docs: say what the diff actually covers - #210
Merged
Merged
Conversation
The feature list stopped at "tables, views, triggers, stored procedures/functions, enum types, and data", which has been understated since composite types, domains, materialized views, sequences and row level security became part of the diff. Each now has a section under How Does the Diff Work?, alongside the kinds already documented there, with the details that decide whether a migration is right: a sequence is altered in place rather than recreated, a matview carries its indexes, and a table's RLS flags are diffed separately from its policies. One passage was wrong rather than incomplete. The destructive linter's warnings were described as losing "definitions rather than data", which is true of a view or a routine but not of the kinds added since: dropping a materialized view discards the result set it holds, and dropping a sequence loses its current value. Both are named now, as is a dropped policy widening who can see which rows. Two claims had drifted independently of that work. Partitions were said to be handled "correctly" by the built-in renderer while sub-partitioning and expression partition keys are still among the cases it cannot reproduce. And the CI matrix was given as "5 PHP x 4 MySQL = 20 jobs, plus dedicated jobs" — the PostgreSQL matrix alone is 25, so the real shape is now a table read off the workflow. The Supabase section says that row level security is diffed, since that is how Supabase enforces per-row access, and that the managed schemas are not, since DBDiff reads public. The pg_dump fidelity table is deliberately left alone: the numbers are stale, but re-measuring them needs a harness fix first, and half of a corrected table would read as pg_dump making things worse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
4 tasks
jasdeepkhalsa
added a commit
that referenced
this pull request
Sep 14, 2026
## 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)
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
Documentation catch-up after #209, plus two claims that had drifted on their own.
Understated. The feature list stopped at "tables, views, triggers, stored procedures/functions, enum types, and data". Composite types, domains, materialized views, sequences and row level security are all diffed now, so each gets a section under How Does the Diff Work? beside the kinds already documented — including the details that decide whether a migration is correct: a sequence is altered in place rather than recreated (recreating resets the counter), a materialized view carries its indexes, and a table's RLS flags are diffed separately from its policies.
Wrong, not merely incomplete. The destructive linter's warnings were described as losing "definitions rather than data". True of a view or a routine; not of the kinds added since. Dropping a materialized view discards the result set it holds until refreshed, and dropping a sequence loses its current value so a recreated one restarts. Both are now named, as is a dropped policy widening who can see which rows.
Drifted independently of that work. Partitions were said to be handled "correctly" by the built-in renderer, while sub-partitioning and expression partition keys are still among the cases it cannot reproduce. And the CI matrix was given as "5 PHP × 4 MySQL = 20 jobs, plus dedicated jobs for…" — the PostgreSQL matrix alone is 25 jobs, so it is now a table read off
tests.yml.Supabase. Says that row level security is diffed, since that is how Supabase enforces per-row access, and that
auth/storageare not, since DBDiff readspublic.Deliberately not changed
The
pg_dumpfidelity table (built-in 52 / 90,with pg_dump 66 / 90) is stale, and is left as-is rather than half-corrected.Re-measuring it turned up a harness bug:
PgDumpRenderercaches its archive keyed onhost|port|database, andreset()exists for exactly this case, butrun-conformance.phpreusespgconf_before/pgconf_after/pgconf_testfor all 90+ cases and never calls it — so from the second case onward thepg_dumppath can be served a previous case's dump. On PostgreSQL 16 the 90-case corpus gives 68/90 with the renderer forced off and 16/90 with it on, which is not a plausible verdict onpg_dump.Writing the measurable half in would have produced
built-in 68 / pg_dump 66— implyingpg_dumphurts fidelity, which is not supported. The table is better stale than misleading until the harness is fixed and both rows can be re-measured. Tracked separately.Test plan
DiffSorterordering, the linter list againstDestructiveLinter's rule map, the matrix againsttests.yml, and the partitioning caveat against the cases still inknown-failures.json🤖 Generated with Claude Code