From 85c74dcf9140538846bcc574ba5fac62c4dd62ac Mon Sep 17 00:00:00 2001 From: Jasdeep Khalsa Date: Tue, 15 Sep 2026 08:20:02 +0000 Subject: [PATCH 1/2] fix(postgres): keep a partitioned table's primary key inline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- README.md | 18 +- src/DB/Adapters/PostgresAdapter.php | 35 ++- .../PartitionedTableRendererPostgresTest.php | 240 ++++++++++++++++++ tests/phpunit.xml | 1 + 4 files changed, 279 insertions(+), 15 deletions(-) create mode 100644 tests/PartitionedTableRendererPostgresTest.php diff --git a/README.md b/README.md index 01af714..99e8964 100644 --- a/README.md +++ b/README.md @@ -75,7 +75,7 @@ fingerprints: | renderer | reproduces | |---|---| | built-in | 68 / 90 | -| with `pg_dump` | **82 / 90** | +| with `pg_dump` | **81 / 90** | Measured on PostgreSQL 16. The figures move by a case or two with the server: the built-in renderer reproduces 67 on PostgreSQL 18, where `LIKE ... INCLUDING @@ -85,10 +85,18 @@ not used at all, so those runs score as built-in. Nothing is required. `pg_dump` is not bundled — the released binaries are static PHP and cannot carry it — so when it is absent, or older than the -server, DBDiff falls back to its built-in renderer and says why. Partitions -always use the built-in renderer, which reproduces the common range, list and -hash forms; sub-partitioning and expression partition keys are among the cases -it does not yet reproduce. +server, DBDiff falls back to its built-in renderer and says why. + +Partitioned tables always use the built-in renderer, whatever is installed, and +that is deliberate rather than a limitation of the integration. `pg_dump` writes +a partitioned parent's primary key as `ALTER TABLE ONLY parent ADD CONSTRAINT`, +which reaches the partitions existing at the moment it runs and no others — +correct in `pg_dump`'s own output order, and silently wrong as soon as anything +reorders the statements, which a tool applying a migration grouped by object +kind reasonably does. The built-in renderer puts the key inside `CREATE TABLE`, +where the partitions inherit it however the statements are ordered. It costs one +case in the table above — an expression partition key — and buys DDL that does +not depend on being applied in the order it was written. A migration produced this way records it, so two machines emitting different SQL is explainable from the file: diff --git a/src/DB/Adapters/PostgresAdapter.php b/src/DB/Adapters/PostgresAdapter.php index 2ab1393..0803e89 100644 --- a/src/DB/Adapters/PostgresAdapter.php +++ b/src/DB/Adapters/PostgresAdapter.php @@ -64,16 +64,6 @@ public function getTableSchema(Connection $connection, string $table): array { } public function getCreateStatement(Connection $connection, string $table): string { - // pg_dump is the reference implementation and reproduces 90 of the 90 - // cases in the shared conformance corpus; the renderer below manages 52. - // It is used whenever it is present and new enough for the server, and - // returns null rather than throwing when it is not, so a machine - // without it keeps working on the hand-written path. - $viaPgDump = PgDumpRenderer::tableDDL($connection, $table); - if ($viaPgDump !== null) { - return $viaPgDump; - } - $partition = PostgresSchemaHelper::partitionMeta($connection, $table); // A partition is declared against its parent, which supplies the columns, @@ -84,6 +74,31 @@ public function getCreateStatement(Connection $connection, string $table): strin return "CREATE TABLE \"$table\" PARTITION OF \"{$partition['parent']}\" {$partition['bound']}"; } + // pg_dump is the reference implementation and reproduces more of the + // shared conformance corpus than the renderer below. It is used whenever + // it is present and new enough for the server, and returns null rather + // than throwing when it is not, so a machine without it keeps working on + // the hand-written path. + // + // A partitioned parent is the exception. pg_dump renders its primary key + // as `ALTER TABLE ONLY parent ADD CONSTRAINT ... PRIMARY KEY`, and ONLY + // means the index reaches the partitions that exist when it runs and no + // others. That is correct in pg_dump's own output order, where the key + // precedes every CREATE TABLE ... PARTITION OF, and silently wrong the + // moment anything reorders the statements — which a consumer applying a + // fix set grouped by object kind legitimately does, creating all the + // tables before any constraint. The partitions then never receive the + // key, and the migration reports success having lost it. + // + // The renderer below emits the key inline in CREATE TABLE, where the + // partitions inherit it however the statements are ordered. + if ($partition['partition_by'] === null) { + $viaPgDump = PgDumpRenderer::tableDDL($connection, $table); + if ($viaPgDump !== null) { + return $viaPgDump; + } + } + $bulk = $this->getBulkTableSchema($connection, [$table]); $schema = $bulk[$table] ?? ['columns' => [], 'keys' => [], 'constraints' => []]; $columns = $schema['columns']; diff --git a/tests/PartitionedTableRendererPostgresTest.php b/tests/PartitionedTableRendererPostgresTest.php new file mode 100644 index 0000000..3a5d2f1 --- /dev/null +++ b/tests/PartitionedTableRendererPostgresTest.php @@ -0,0 +1,240 @@ +listen(StatementPrepared::class, function ($event) { + $event->statement->setFetchMode(PDO::FETCH_ASSOC); + }); + $capsule->setEventDispatcher($dispatcher); + $capsule->addConnection([ + 'driver' => 'pgsql', + 'host' => $host, + 'port' => $port, + 'database' => $db, + 'username' => $user, + 'password' => $pass, + 'charset' => 'utf8', + 'schema' => 'public', + ], 'partitioned_' . $db); + $this->capsule = $capsule; + + return $capsule->getConnection('partitioned_' . $db); + } + + protected function setUp(): void + { + if (!extension_loaded('pdo_pgsql')) { + $this->markTestSkipped('pdo_pgsql extension not loaded.'); + } + $host = getenv('DB_HOST_POSTGRES') ?: null; + if (!$host) { + $this->markTestSkipped('DB_HOST_POSTGRES env var not set.'); + } + if (!self::commandExists('pg_dump') || !self::commandExists('pg_restore')) { + $this->markTestSkipped('pg_dump/pg_restore not on PATH — the built-in renderer runs regardless.'); + } + + // The suite pins the renderer off; this case is about what happens when + // it is available, so it opts back in. + putenv('DBDIFF_PG_DUMP_RENDERER='); + + $port = '5432'; + $user = 'dbdiff'; + $pass = 'rootpass'; + + $this->adminDb = new PDO( + "pgsql:host=$host;port=$port;dbname=diff1", $user, $pass, + [PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION] + ); + $this->dropDb(); + $this->adminDb->exec("CREATE DATABASE {$this->database}"); + + $this->connection = $this->connect($host, $port, $user, $pass, $this->database); + $this->adapter = new PostgresAdapter(); + + $this->connection->unprepared(<<<'SQL' + CREATE TABLE readings ( + id bigint GENERATED BY DEFAULT AS IDENTITY, + taken_on date NOT NULL, + value numeric(10,2), + PRIMARY KEY (taken_on, id) + ) PARTITION BY RANGE (taken_on); + CREATE TABLE readings_2025 PARTITION OF readings + FOR VALUES FROM ('2025-01-01') TO ('2026-01-01'); + CREATE TABLE readings_2026 PARTITION OF readings + FOR VALUES FROM ('2026-01-01') TO ('2027-01-01'); +SQL); + + // The availability probe dumps the database, and that archive is cached. + // The tables above were created after it, so without this the renderer + // would look for them in a snapshot taken before they existed. + PgDumpRenderer::reset(); + } + + protected function tearDown(): void + { + PgDumpRenderer::reset(); + putenv('DBDIFF_PG_DUMP_RENDERER=off'); + if ($this->connection) { + $this->connection->disconnect(); + $this->connection = null; + } + $this->capsule = null; + if ($this->adminDb) { + $this->dropDb(); + } + } + + private function dropDb(): void + { + try { + $this->adminDb->exec("DROP DATABASE IF EXISTS {$this->database} WITH (FORCE)"); + } catch (PDOException $e) { + $this->adminDb->exec("DROP DATABASE IF EXISTS {$this->database}"); + } + } + + /** + * The key has to be part of CREATE TABLE, not a later ALTER, so that a + * partition created afterwards inherits it no matter when the statements run. + */ + public function testPartitionedParentRendersItsPrimaryKeyInline(): void + { + $ddl = $this->adapter->getCreateStatement($this->connection, 'readings'); + + $this->assertStringContainsString('PARTITION BY RANGE', $ddl); + $this->assertMatchesRegularExpression( + '/CREATE\s+TABLE.*PRIMARY\s+KEY/s', + $ddl, + 'the primary key must be inline in CREATE TABLE' + ); + $this->assertStringNotContainsStringIgnoringCase( + 'ALTER TABLE ONLY', + $ddl, + 'ALTER TABLE ONLY reaches only the partitions that already exist, ' + . 'so the key is lost whenever the statements are reordered' + ); + } + + /** + * The property that matters, asserted the way a consumer breaks it: create + * every table first, then everything else. + */ + public function testPartitionsKeepTheKeyWhenTablesAreCreatedFirst(): void + { + $statements = []; + foreach (['readings', 'readings_2025', 'readings_2026'] as $table) { + foreach (explode(";\n", $this->adapter->getCreateStatement($this->connection, $table)) as $stmt) { + $stmt = trim(rtrim(trim($stmt), ';')); + if ($stmt !== '') { + $statements[] = $stmt; + } + } + } + + // Group by kind the way a consumer ordering a fix set would: tables + // before anything that alters them. Relative order within each group is + // kept, so the parent still precedes its partitions. + $creates = array_values(array_filter( + $statements, + fn(string $s): bool => (bool) preg_match('/^\s*CREATE\s+TABLE/i', $s) + )); + $rest = array_values(array_filter( + $statements, + fn(string $s): bool => !preg_match('/^\s*CREATE\s+TABLE/i', $s) + )); + + $this->adminDb->exec('DROP DATABASE IF EXISTS ' . $this->database . '_replay'); + $this->adminDb->exec('CREATE DATABASE ' . $this->database . '_replay'); + $replay = $this->connect( + getenv('DB_HOST_POSTGRES'), '5432', 'dbdiff', 'rootpass', $this->database . '_replay' + ); + + try { + foreach (array_merge($creates, $rest) as $stmt) { + $replay->unprepared($stmt); + } + + foreach (['readings_2025', 'readings_2026'] as $partition) { + $rows = $replay->select( + "SELECT con.conname + FROM pg_constraint con + JOIN pg_class c ON c.oid = con.conrelid + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'public' + AND c.relname = ? + AND con.contype = 'p'", + [$partition] + ); + $this->assertCount( + 1, + $rows, + "$partition lost its primary key: the parent's key did not reach it, " + . 'which is what ALTER TABLE ONLY does once the partitions already exist' + ); + } + } finally { + $replay->disconnect(); + $this->capsule = null; + try { + $this->adminDb->exec( + 'DROP DATABASE IF EXISTS ' . $this->database . '_replay WITH (FORCE)' + ); + } catch (PDOException $e) { + $this->adminDb->exec('DROP DATABASE IF EXISTS ' . $this->database . '_replay'); + } + } + } +} diff --git a/tests/phpunit.xml b/tests/phpunit.xml index e30104c..d9b592e 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -40,6 +40,7 @@ ./BulkSchemaPostgresTest.php ./PostgresObjectKindsTest.php ./PgDumpRendererPostgresTest.php + ./PartitionedTableRendererPostgresTest.php ./RoutineOverloadPostgresTest.php From 1fbd6c7b8b66bb178d36cdd24a0112a90aebe13b Mon Sep 17 00:00:00 2001 From: Jasdeep Khalsa Date: Tue, 15 Sep 2026 13:00:00 +0100 Subject: [PATCH 2/2] fix(postgres): reproduce a serial column's sequence ownership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/DB/Support/PgDumpRenderer.php | 20 +++++++- tests/PgDumpRendererPostgresTest.php | 77 ++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/src/DB/Support/PgDumpRenderer.php b/src/DB/Support/PgDumpRenderer.php index ca76830..60dd487 100644 --- a/src/DB/Support/PgDumpRenderer.php +++ b/src/DB/Support/PgDumpRenderer.php @@ -316,12 +316,30 @@ private static function entriesFor(Connection $connection, string $table): array * * TABLE ATTACH and INDEX ATTACH are excluded for the same reason * partitions are skipped entirely — see tableDDL. + * + * SEQUENCE OWNED BY is included, and leaving it out was a real defect. A + * serial column's sequence belongs to that column: PostgreSQL records the + * link with a dependency, drops the sequence when the column goes, and a + * schema reader can tell from it that the sequence is not a standalone + * object. Emitting SEQUENCE and DEFAULT without it reproduced the column's + * behaviour but not that link, so the copy held a sequence owned by nothing. + * The next diff between the two then found a standalone sequence on one side + * only and set about dropping it — which PostgreSQL refuses, because the + * column default still depends on it: + * + * cannot drop sequence shipments_id_seq because other objects depend on it + * + * TABLE DATA and SEQUENCE SET stay out: this renders schema, and a schema + * diff neither copies rows nor moves a sequence's current value. */ private static function wantsType(string $type): bool { return in_array( $type, - ['TABLE', 'SEQUENCE', 'INDEX', 'CONSTRAINT', 'FK CONSTRAINT', 'DEFAULT', 'COMMENT'], + [ + 'TABLE', 'SEQUENCE', 'SEQUENCE OWNED BY', 'INDEX', + 'CONSTRAINT', 'FK CONSTRAINT', 'DEFAULT', 'COMMENT', + ], true ); } diff --git a/tests/PgDumpRendererPostgresTest.php b/tests/PgDumpRendererPostgresTest.php index 5f99ec3..23d5c13 100644 --- a/tests/PgDumpRendererPostgresTest.php +++ b/tests/PgDumpRendererPostgresTest.php @@ -258,4 +258,81 @@ public function testDeclinesWhenPgDumpIsOlderThanTheServer(): void @unlink($stub); } } + + /** + * A serial column's sequence has to come back owned by that column. + * + * PostgreSQL records the link as a dependency, and it is what makes the + * sequence part of the column rather than an object in its own right. The + * SEQUENCE and DEFAULT entries alone reproduce the column's behaviour but + * not the link, which leaves the copy holding a sequence owned by nothing. + * + * That was invisible until sequences became a modelled object kind. A diff + * between the original and the copy then found a standalone sequence on one + * side only and generated a DROP for it, which PostgreSQL refuses: + * + * cannot drop sequence t_id_seq because other objects depend on it + * + * So the property asserted here is the round trip, not the text: reproduce + * the table, and the sequence must be as invisible to the object-kind reader + * on the copy as it is on the original. + */ + public function testAReproducedSerialColumnKeepsItsSequenceOwnership(): void + { + $this->connection->statement('CREATE TABLE t (id serial PRIMARY KEY, n text)'); + + $ddl = $this->adapter->getCreateStatement($this->connection, 't'); + $this->assertCameFromPgDump($ddl); + $this->assertStringContainsString( + 'OWNED BY', + $ddl, + 'without ALTER SEQUENCE ... OWNED BY the copy holds an unowned sequence' + ); + + // The original does not report it: it belongs to the column. + $this->assertSame( + [], + \DBDiff\DB\Support\PostgresObjectKinds::sequences($this->connection), + 'a serial column\'s sequence is not a standalone object' + ); + + $replayDb = $this->database . '_replay'; + $host = getenv('DB_HOST_POSTGRES') ?: getenv('DB_HOST'); + $port = getenv('DB_PORT_POSTGRES') ?: '5432'; + $user = getenv('DB_USER_POSTGRES') ?: 'dbdiff'; + $pass = getenv('DB_PASSWORD_POSTGRES') ?: 'rootpass'; + + $admin = $this->connect($host, $port, $user, $pass, 'postgres'); + $admin->statement("DROP DATABASE IF EXISTS $replayDb"); + $admin->statement("CREATE DATABASE $replayDb"); + $admin->disconnect(); + + $replay = $this->connect($host, $port, $user, $pass, $replayDb); + try { + foreach (explode(";\n", $ddl) as $stmt) { + $stmt = trim(rtrim(trim($stmt), ';')); + if ($stmt !== '') { + $replay->statement($stmt); + } + } + + // And neither does the copy — which is the whole point. A mismatch + // here is a DROP SEQUENCE in the next migration. + $this->assertSame( + [], + \DBDiff\DB\Support\PostgresObjectKinds::sequences($replay), + 'the reproduced sequence is unowned, so it reads as standalone' + ); + } finally { + $replay->disconnect(); + $admin = $this->connect($host, $port, $user, $pass, 'postgres'); + $admin->statement( + "SELECT pg_terminate_backend(pid) FROM pg_stat_activity + WHERE datname = ? AND pid <> pg_backend_pid()", + [$replayDb] + ); + $admin->statement("DROP DATABASE IF EXISTS $replayDb"); + $admin->disconnect(); + } + } }