Skip to content
Permalink

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: DBDiff/DBDiff
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: 2ae3d86
Choose a base ref
...
head repository: DBDiff/DBDiff
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: d99f75f
Choose a head ref
  • 5 commits
  • 6 files changed
  • 3 contributors

Commits on Sep 14, 2026

  1. fix(conformance): reset the pg_dump archive between cases (#211)

    ## Summary
    
    `PgDumpRenderer` caches one `pg_dump` archive per `host|port|database`
    for the life of the process, and its own `reset()` exists for "a process
    that reconnects". The conformance runner is exactly that: every case
    drops and recreates `pgconf_before`, `pgconf_after` and `pgconf_test`
    under those same three names, so from the **second case onward the
    renderer answered from the first case's dump**.
    
    The effect was to make PostgreSQL's own tooling look far worse than the
    hand-written renderer it is meant to exceed:
    
    | PostgreSQL 16, 90-case renderer corpus | before | after |
    |---|---|---|
    | built-in | 68 / 90 | 68 / 90 |
    | with `pg_dump` | 16 / 90 | **82 / 90** |
    
    The built-in row not moving is the control — the change only touches the
    `pg_dump` path.
    
    ## Nothing was being hidden
    
    Worth stating, because the alternative would have been serious. Across
    the full suite, in both renderer modes, there are **no failures outside
    the baseline and no case that passed before stops passing**:
    
    | renderer | passed | new failures | baselined but passing |
    |---|---|---|---|
    | built-in | 291 | 0 | 3 |
    | with `pg_dump` | 305 | 0 | 17 |
    
    The stale archive only ever *lost* cases, which is why it survived
    unnoticed: the suite stayed green because every case it broke was
    already recorded as a known failure. `known-failures.json` therefore
    needs no additions.
    
    ## Why the baseline stays where it is
    
    It stays pinned to the built-in renderer rather than dropping to the 8
    hard cases that fail with `pg_dump`. That is the figure every
    environment can meet — pruning to the `pg_dump` result would report 14
    new failures on any machine without `pg_dump` installed, which includes
    most contributors.
    
    The trade is that a run which *does* have `pg_dump` now reports 17 cases
    as baselined-but-passing. The runner already treats that as
    informational rather than fatal, for the same reason it does
    version-specific differences: *"a case that fails on one version can
    legitimately pass on another. Failing the run for that would mean no
    single baseline could ever satisfy the whole matrix."* Renderer
    availability is the same class of variation.
    
    If the noise is unwelcome, the alternatives are a per-renderer baseline
    or pinning the conformance job to one renderer — both more machinery
    than the problem currently justifies.
    
    ## README
    
    The fidelity table was measured before this bug and is corrected here,
    with the server version stated because the numbers move with it: the
    built-in renderer reproduces 67 on PostgreSQL 18, where `LIKE ...
    INCLUDING ALL` copies the named NOT NULL constraints that release
    introduced, and `pg_dump` older than the server is not used at all so
    those runs score as built-in.
    
    ## Test plan
    
    - [x] 90-case corpus re-measured both renderer modes on PostgreSQL 16
    and 18
    - [x] PostgreSQL 18 scores 67 in *both* modes, confirming the renderer
    correctly declines when older than the server (client 17.11 vs server
    18) rather than silently misrendering
    - [x] Full suite both modes: `Failed (no diff): 0`, zero new failures,
    baseline unchanged at 37
    - [x] Checked no other harness shares the bug — `phpunit.xml` pins
    `DBDIFF_PG_DUMP_RENDERER=off`, and `PgDumpRendererPostgresTest` already
    calls `reset()` and enables the renderer only for itself
    
    Stacked on #210 only by adjacency in the README; that PR does not touch
    the table.
    
    🤖 Generated with [Claude Code](https://claude.com/claude-code)
    jasdeepkhalsa authored Sep 14, 2026
    Configuration menu
    Copy the full SHA
    1bbbbff View commit details
    Browse the repository at this point in the history

Commits on Sep 15, 2026

  1. fix(postgres): keep a partitioned table's primary key inline

    pg_dump writes a partitioned parent's primary key as
    
        ALTER TABLE ONLY parent ADD CONSTRAINT parent_pkey PRIMARY KEY (...)
    
    and ONLY means the index reaches the partitions that exist when it runs
    and no others. In pg_dump's own output order the key precedes every
    CREATE TABLE ... PARTITION OF, so they inherit it and the migration is
    correct. Reorder the statements — create every table before adding any
    constraint, which is a reasonable way to apply a fix set and is what
    SupaForge's ordering does — and the partitions never receive the key. The
    migration reports success having silently dropped it.
    
    Rendering DDL whose correctness depends on the order it happens to be
    written in is the underlying fault, so partitioned tables now use the
    built-in renderer, which emits the key inside CREATE TABLE where the
    order cannot matter. This is what the README already claimed; it stopped
    being true when pg_dump rendering was introduced and began taking
    precedence for every table.
    
    Reached 3.0.0-rc.10. Every partitioned table with a primary key diffed by
    that release, on a machine with pg_dump, loses its partition keys when
    applied by a consumer that reorders.
    
    Why the suite missed it: the conformance runner applies DBDiff's
    statements in DBDiff's order, which is the one order in which this bug is
    invisible. The corpus does cover partitioned tables with primary keys,
    and those cases passed. Nothing asserted that the DDL survives being
    reordered, so that is what the new test does — it replays the statements
    with every CREATE TABLE moved first and checks the partitions still have
    their keys. It fails without this change with "readings_2025 lost its
    primary key".
    
    Costs one corpus case, hard_part_expr_key, which pg_dump reproduced and
    the built-in renderer does not; it was already a known failure, so the
    baseline is unchanged. The README's figure moves from 82 to 81 and now
    says why the exception exists.
    
    Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
    jasdeepkhalsa and claude committed Sep 15, 2026
    Configuration menu
    Copy the full SHA
    85c74dc View commit details
    Browse the repository at this point in the history
  2. fix(postgres): keep a partitioned table's primary key inline (#212)

    ## The bug
    
    `pg_dump` writes a partitioned parent's primary key as
    
    ```sql
    ALTER TABLE ONLY public.readings ADD CONSTRAINT readings_pkey PRIMARY KEY (taken_on, id);
    ```
    
    `ONLY` means the index reaches the partitions that exist **at the moment
    it runs** and no others. In `pg_dump`'s own output order the key
    precedes every `CREATE TABLE ... PARTITION OF`, so they inherit it and
    the migration is correct.
    
    Reorder the statements and it silently isn't. Grouping a fix set by
    object kind — every table created before any constraint is added — is a
    reasonable way to apply a migration, and SupaForge's `orderStatements`
    does exactly that (`CREATE_TABLE` phase 40 before `ALTER_TABLE` phase
    50). The partitions are then created before the key is added, never
    receive it, and the migration reports success having dropped it.
    
    **This reached 3.0.0-rc.10.** Every partitioned table with a primary key
    diffed by that release, on a machine with `pg_dump`, loses its partition
    keys when applied by a consumer that reorders. It surfaced as a
    SupaForge e2e failure whose fingerprint showed the partitions missing
    `readings_2025_pkey` / `readings_2026_pkey`, their indexes, and the
    `inh` attach links.
    
    Introduced by `4e9c912` ("render DDL with pg_dump when it is
    available"), which put `PgDumpRenderer::tableDDL()` first in
    `getCreateStatement` for **every** table. rc.9 was immune because the
    built-in renderer emits the key inline.
    
    ## The fix
    
    Partition metadata is read before `pg_dump` is tried, so partitioned
    tables use the built-in renderer and get the key inside `CREATE TABLE`,
    where the statement order cannot matter.
    
    The real fault is DDL whose correctness depends on the order it happens
    to be written in. This is also what the README already claimed —
    "Partitions always use the built-in renderer" — which stopped being true
    when `pg_dump` rendering began taking precedence.
    
    ## Why the suite missed it
    
    The corpus **does** cover partitioned tables with primary keys
    (`obj_partitioned_range`, `obj_partition_index`,
    `hard_part_index_propagated`), and those cases passed throughout.
    
    The conformance runner applies DBDiff's statements in DBDiff's own order
    — the one order in which this bug is invisible. Nothing asserted that
    the emitted DDL survives being reordered.
    
    So that is what the new test does.
    `PartitionedTableRendererPostgresTest` has two cases, both verified to
    fail without this change:
    
    - the parent's key is inline and the DDL contains no `ALTER TABLE ONLY`
    - the statements are replayed with every `CREATE TABLE` moved first, and
    each partition must still have its primary key — without the fix this
    fails with `readings_2025 lost its primary key`
    
    The second encodes the property rather than the symptom, and is the one
    that would have caught the original regression.
    
    I also considered and **disproved** two other explanations before
    finding this: that the stale `pg_dump` archive cache (#211) was hiding
    it, and that single-transaction application in the runner was masking
    it. Both were tested; neither holds.
    
    ## Cost
    
    One corpus case: `hard_part_expr_key`, an expression partition key that
    `pg_dump` reproduced and the built-in renderer does not. It was already
    in `known-failures.json`, so the baseline is unchanged and the suite
    reports no new failures. Measured back to back, twice each way, to be
    sure of the delta.
    
    | PostgreSQL 16, 90-case corpus | before | after |
    |---|---|---|
    | built-in | 68 / 90 | 68 / 90 |
    | with `pg_dump` | 82 / 90 | **81 / 90** |
    
    The README's figure and the reason for the exception are updated.
    
    ## Test plan
    
    - [x] New test fails without the fix (both cases), passes with it
    - [x] Verified the fix is targeted, not a blanket disable: an ordinary
    table still renders via `pg_dump`, a partitioned parent does not
    - [x] Full conformance suite, both renderer modes: `Failed (no diff):
    0`, **zero new failures** (304 with `pg_dump`, 291 built-in)
    - [x] 782 unit, 68 Postgres, 16 SQLite pass
    - [x] Corpus delta measured back to back in one session rather than from
    separate runs, after an initial single measurement proved to be an
    artifact of leftover scratch databases
    
    🤖 Generated with [Claude Code](https://claude.com/claude-code)
    jasdeepkhalsa authored Sep 15, 2026
    Configuration menu
    Copy the full SHA
    dd4a88c View commit details
    Browse the repository at this point in the history
  3. fix(postgres): reproduce a serial column's sequence ownership

    A serial column's sequence belongs to that column. PostgreSQL records
    the link as a dependency, drops the sequence when the column goes, and a
    schema reader uses it to tell an owned sequence from a standalone one.
    
    pg_dump writes the link as its own table-of-contents entry, SEQUENCE
    OWNED BY, and wantsType did not ask for it. The SEQUENCE and DEFAULT
    entries alone reproduce what the column does but not what it owns, so a
    table copied by DBDiff held a sequence owned by nothing.
    
    Nothing looked at sequence ownership until sequences became a modelled
    object kind, so the copy being subtly wrong did not show. Once they were
    modelled, a diff between the original and the copy found a standalone
    sequence on one side only and generated a DROP for it — which PostgreSQL
    refuses, because the column default still depends on it:
    
        cannot drop sequence shipments_id_seq because other objects depend on it
    
    The migration then rolls back entirely, so a schema DBDiff had itself
    created could not be diffed again. Reached 3.0.0-rc.10 and rc.11.
    
    Asserted as a round trip rather than as text, because the text was never
    the point: reproduce the table into an empty database, and the sequence
    has to be as invisible to the object-kind reader on the copy as it is on
    the original. The test fails without this change.
    
    TABLE DATA and SEQUENCE SET stay excluded — this renders schema, and a
    schema diff neither copies rows nor moves a sequence's current value.
    
    Verified through SupaForge, whose suite is where this surfaced: its
    lifecycle e2e file goes from 31 of 32 to 32 of 32, and its full suite to
    1225 passing. Conformance is unchanged at 304 with pg_dump and 291
    without, no new failures on either path.
    
    Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
    jasdeepkhalsa and claude committed Sep 15, 2026
    Configuration menu
    Copy the full SHA
    1fbd6c7 View commit details
    Browse the repository at this point in the history
  4. fix(postgres): reproduce a serial column's sequence ownership (#213)

    ## The bug
    
    A `serial` column's sequence belongs to that column. PostgreSQL records
    the link as a dependency, drops the sequence when the column goes, and a
    schema reader uses it to tell an owned sequence from a standalone one.
    
    `pg_dump` writes that link as its own table-of-contents entry —
    `SEQUENCE OWNED BY` — and `wantsType` never asked for it:
    
    ```
    216; 1259 TABLE public shipments
    215; 1259 SEQUENCE public shipments_id_seq
    3423; 0 0 SEQUENCE OWNED BY public shipments_id_seq     <-- filtered out
    3267; 2604 DEFAULT public shipments id
    ```
    
    `SEQUENCE` and `DEFAULT` reproduce what the column *does* but not what
    it *owns*, so a table copied by DBDiff held a sequence owned by nothing.
    Measured directly:
    
    | | `shipments_id_seq` ownership |
    |---|---|
    | original | `a` (owned by the column) |
    | DBDiff's copy | **`UNOWNED`** |
    
    Nothing read sequence ownership until sequences became a modelled object
    kind in #209, so the copy being subtly wrong never showed. Once they
    were modelled, a diff between the original and the copy found a
    standalone sequence on one side only and generated a `DROP` for it —
    which PostgreSQL refuses, because the column default still depends on
    it:
    
    ```
    1 error(s):
      ✗ [schema] schema-drop-sequence-4: cannot drop sequence shipments_id_seq because other objects depend on it
    
      Nothing was written.
    ```
    
    The migration rolls back entirely, so **a schema DBDiff had itself
    created could not be diffed again**. This reached rc.10 and rc.11.
    
    ## The fix
    
    Add `SEQUENCE OWNED BY` to the allowlist. It is part of a table's own
    DDL in exactly the way `DEFAULT` is.
    
    `TABLE DATA` and `SEQUENCE SET` stay excluded: this renders schema, and
    a schema diff neither copies rows nor moves a sequence's current value.
    
    ## Why it escaped
    
    `PostgresObjectKindsTest` covers serial, identity and standalone
    sequences — but only on a **freshly created** schema, never on one
    DBDiff had itself reproduced. The defect needs a second diff against
    DBDiff's own output to appear, and nothing asserted that round trip.
    
    That is the same shape as #212: both bugs are invisible in a single pass
    and only surface on the second. So the test asserts the round trip
    rather than the text — reproduce the table into an empty database, and
    the sequence must be as invisible to the object-kind reader on the copy
    as it is on the original. It fails without this change.
    
    ## Verification
    
    Found and confirmed through SupaForge's own suite, where it surfaced.
    Its e2e lifecycle file is order-dependent in a useful way: the failure
    needs an earlier test to have synced a table with a serial column first,
    which is why the case passes in isolation and fails in the file.
    
    | DBDiff | whole `lifecycle.test.ts` |
    |---|---|
    | rc.9 | 32 / 32 |
    | rc.10, rc.11 | 31 pass, 1 fail |
    | **this branch** | **32 / 32** |
    
    Run against this branch's source via a shim over the released binary,
    with the rc.11 pins otherwise untouched so only the DBDiff code
    differed. SupaForge's full suite: **1225 passing, 3 skipped, 59/59
    files**.
    
    - [x] New test fails without the fix, passes with it
    - [x] 782 unit, 69 Postgres, 16 SQLite pass
    - [x] Conformance unchanged — 304 with `pg_dump`, 291 without, `Failed
    (no diff): 0`, no new failures on either path
    
    ## Note on the release
    
    rc.11 is currently the `latest` dist-tag and carries this, so anything
    installing `@dbdiff/cli` today hits it when re-diffing a schema
    containing a `serial` column. Worth an rc.12 once this lands.
    
    🤖 Generated with [Claude Code](https://claude.com/claude-code)
    jasdeepkhalsa authored Sep 15, 2026
    Configuration menu
    Copy the full SHA
    d99f75f View commit details
    Browse the repository at this point in the history
Loading