Skip to content

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

Merged
jasdeepkhalsa merged 2 commits into
masterfrom
fix/conformance-renderer-cache
Sep 14, 2026
Merged

jasdeepkhalsa merged 2 commits into
masterfrom
fix/conformance-renderer-cache

Conversation

@jasdeepkhalsa

Copy link
Copy Markdown
Member

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

  • 90-case corpus re-measured both renderer modes on PostgreSQL 16 and 18
  • 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
  • Full suite both modes: Failed (no diff): 0, zero new failures, baseline unchanged at 37
  • 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

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>
@github-actions github-actions Bot added bug php Pull requests that update php code postgres Related to Postgres labels Sep 14, 2026
@sonarqubecloud

Copy link
Copy Markdown

@jasdeepkhalsa
jasdeepkhalsa merged commit 1bbbbff into master Sep 14, 2026
69 checks passed
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug php Pull requests that update php code postgres Related to Postgres

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant