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