fix(conformance): reset the pg_dump archive between cases - #211
Merged
Merged
Conversation
PgDumpRenderer caches one pg_dump archive per host|port|database for the life of the process, and 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. On PostgreSQL 16 the 90-case renderer corpus scored 16 with pg_dump against 68 without it. With the archive reset per case it scores 82. Nothing was being hidden by this: across the full suite neither renderer produces a failure outside the baseline, and no case that passed before stops passing. The stale archive only ever lost cases, which is why it survived — the suite stayed green because every case it broke was already recorded as a known failure. The baseline stays pinned to the built-in renderer rather than dropping to the 8 hard cases that fail with pg_dump. It 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. A run that does have it now reports those cases as baselined-but-passing, which the runner already treats as informational for the same reason it does version-specific differences. The README's fidelity table was measured before this bug and is corrected here, with the server version stated: the numbers move with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
5 tasks
jasdeepkhalsa
added a commit
that referenced
this pull request
Sep 15, 2026
## 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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Summary
PgDumpRenderercaches onepg_dumparchive perhost|port|databasefor the life of the process, and its ownreset()exists for "a process that reconnects". The conformance runner is exactly that: every case drops and recreatespgconf_before,pgconf_afterandpgconf_testunder 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:
pg_dumpThe built-in row not moving is the control — the change only touches the
pg_dumppath.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:
pg_dumpThe 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.jsontherefore 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 thepg_dumpresult would report 14 new failures on any machine withoutpg_dumpinstalled, which includes most contributors.The trade is that a run which does have
pg_dumpnow 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 ALLcopies the named NOT NULL constraints that release introduced, andpg_dumpolder than the server is not used at all so those runs score as built-in.Test plan
Failed (no diff): 0, zero new failures, baseline unchanged at 37phpunit.xmlpinsDBDIFF_PG_DUMP_RENDERER=off, andPgDumpRendererPostgresTestalready callsreset()and enables the renderer only for itselfStacked on #210 only by adjacency in the README; that PR does not touch the table.
🤖 Generated with Claude Code