Skip to content

fix(postgres): keep a partitioned table's primary key inline #212

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jasdeepkhalsa merged 1 commit into from
Sep 15, 2026
+279 −15
Merged

fix(postgres): keep a partitioned table's primary key inline #212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand Down
35 changes: 25 additions & 10 deletions src/DB/Adapters/PostgresAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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'];
Expand Down
240 changes: 240 additions & 0 deletions tests/PartitionedTableRendererPostgresTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,240 @@
<?php

use DBDiff\DB\Adapters\PostgresAdapter;
use DBDiff\DB\Support\PgDumpRenderer;
use Illuminate\Database\Capsule\Manager as Capsule;
use Illuminate\Database\Events\StatementPrepared;
use Illuminate\Events\Dispatcher;
use PHPUnit\Framework\TestCase;

/**
* A partitioned table's DDL has to survive being reordered.
*
* DBDiff emits one statement per difference and says nothing about how they are
* applied. Consumers reorder: grouping a fix set by object kind — every table
* created before any constraint is added — is a reasonable thing to do, and
* SupaForge does exactly that. DDL whose correctness depends on DBDiff's own
* output order is therefore DDL that will sometimes be applied wrongly.
*
* pg_dump renders 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 existing when it runs and no
* others. In pg_dump's own order the key precedes every CREATE TABLE ...
* PARTITION OF, so the partitions inherit it. Move the table creations first and
* the partitions never receive it — the migration applies cleanly and the
* primary keys are gone. That reached a release: every partitioned table diffed
* by 3.0.0-rc.10 on a machine with pg_dump lost its partition keys when applied
* by a consumer that reorders.
*
* The conformance suite could not see it, because it applies DBDiff's statements
* in DBDiff's order. These cases fix both halves: the rendering is asserted
* directly, and the result is asserted after a deliberate reordering.
*
* Skips when pdo_pgsql, DB_HOST_POSTGRES, or pg_dump is absent — without
* pg_dump the built-in renderer runs anyway, which is the behaviour being
* required here, so there would be nothing to prove.
*/
class PartitionedTableRendererPostgresTest extends TestCase
{
private string $database = 'dbdiff_partitioned_renderer';
private $connection;
private $capsule;
private PostgresAdapter $adapter;
private $adminDb;

private static function commandExists(string $bin): bool
{
exec('command -v ' . escapeshellarg($bin), $out, $code);
return $code === 0;
}

private function connect(string $host, string $port, string $user, string $pass, string $db)
{
$capsule = new Capsule;
$dispatcher = new Dispatcher();
$dispatcher->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');
}
}
}
}
1 change: 1 addition & 0 deletions tests/phpunit.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
<file>./BulkSchemaPostgresTest.php</file>
<file>./PostgresObjectKindsTest.php</file>
<file>./PgDumpRendererPostgresTest.php</file>
<file>./PartitionedTableRendererPostgresTest.php</file>
<file>./RoutineOverloadPostgresTest.php</file>
</testsuite>
<testsuite name="SQLite">
Expand Down
Loading