Skip to content

docs: say what the diff actually covers - #210

Merged
jasdeepkhalsa merged 1 commit into
masterfrom
docs/object-kinds-readme
Sep 14, 2026
Merged

jasdeepkhalsa merged 1 commit into
masterfrom
docs/object-kinds-readme

Conversation

@jasdeepkhalsa

Copy link
Copy Markdown
Member

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/storage are not, since DBDiff reads public.

Deliberately not changed

The pg_dump fidelity 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: PgDumpRenderer caches its archive keyed on host|port|database, and reset() exists for exactly this case, but run-conformance.php reuses pgconf_before/pgconf_after/pgconf_test for all 90+ cases and never calls it — so from the second case onward the pg_dump path 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 on pg_dump.

Writing the measurable half in would have produced built-in 68 / pg_dump 66 — implying pg_dump hurts 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

  • Docs only — no code paths touched
  • Every claim checked against the code or the workflow it describes, not against memory: the per-kind sections against the adapter and DiffSorter ordering, the linter list against DestructiveLinter's rule map, the matrix against tests.yml, and the partitioning caveat against the cases still in known-failures.json

🤖 Generated with Claude Code

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>
@github-actions github-actions Bot added bug enhancement mysql Related to Mysql postgres Related to Postgres labels Sep 14, 2026
@sonarqubecloud

Copy link
Copy Markdown

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

Labels

bug enhancement mysql Related to Mysql postgres Related to Postgres

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant