fix(postgres): keep a column's declared precision, and only that · DBDiff/DBDiff@b6df49b · GitHub
Skip to content

Commit b6df49b

Browse files
jasdeepkhalsaclaude
jasdeepkhalsa
and
claude
committed
fix(postgres): keep a column's declared precision, and only that
ADD COLUMN emitted `timestamptz(6)` for a column declared `timestamptz`. The two behave identically — 6 is the default — but they are different types in the catalog, so a migration generated from the column did not reproduce it, and a consumer that verifies convergence afterwards saw the column as still drifted. Reported as issue #215, where supaforge's `--prove` rejects otherwise-valid migrations on any table carrying a plain `created_at`. The cause is that `information_schema.columns.datetime_precision` reports 6 both for `timestamptz` and for `timestamptz(6)`. It cannot express "no modifier was given", so nothing read from it can tell a precision that was asked for from one that was merely defaulted. `atttypmod` can: -1 means no modifier, and for these types anything else is the precision. It comes from attributeMeta, which was already reading pg_attribute for storage, compression and collation; the merge in assembleColumns now happens before the type is built rather than after. Reproducing it turned up three more faults of the same shape, all fixed here: - `timestamp(0)` lost its precision, because the old test was `datetime_precision > 0` and 0 is a legal precision; - `time(3)` and `timetz(2)` lost theirs entirely, returning from the type alias map before any precision was considered; - `interval day to second(3)` and `interval year to month` flattened to bare `interval`. interval's modifier carries a field range as well as a precision, which no single number can express, so that one takes the server's own `format_type` rendering. Verified against live PostgreSQL, in both directions. Fourteen columns covering every optional-precision type, declared bare and with an explicit modifier: each is now emitted exactly as declared, and applying the generated DDL reproduces every one of the 15 catalog types — where master reproduces 10 and differs on 5. Nine interval and boundary cases round-trip likewise, `interval day to second(3)` included. tests/ColumnTypePrecisionPostgresTest.php covers this end to end, including the apply-and-compare that is the property the issue is really about; all 6 of its tests fail on master. The unit provider for buildColumnType moves to atttypmod and gains the cases above — it previously asserted the bug, since a fabricated row saying `datetime_precision => 6` was expected to produce `timestamp(6)`. The 50 `timestamp(6)` expectations across 25 golden files were asserting the same bug: every fixture behind them declares a bare `TIMESTAMP`. They now expect `timestamp`, and the 21 Postgres comprehensive tests pass against them. Fixes #215 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent a9f8c8a commit b6df49b

29 files changed

Lines changed: 388 additions & 61 deletions

‎src/DB/Adapters/PostgresAdapter.php‎

Lines changed: 41 additions & 6 deletions

‎src/DB/Support/PostgresSchemaHelper.php‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,17 @@ public static function attributeMeta(Connection $connection, array $tables): arr
177177
THEN co.collname END AS explicit_collation,
178178
a.attstorage::text AS att_storage,
179179
t.typstorage::text AS type_storage,
180-
NULLIF(a.attcompression::text, '') AS att_compression
180+
NULLIF(a.attcompression::text, '') AS att_compression,
181+
-- -1 means the column was declared without a type modifier.
182+
-- information_schema cannot express that: a bare timestamptz
183+
-- and a timestamptz(6) both report datetime_precision = 6,
184+
-- so a precision nobody asked for was emitted (issue #215).
185+
a.atttypmod,
186+
-- What the column was actually declared as, rendered by the
187+
-- server. The only reliable source for the types whose
188+
-- modifier is more than a number, such as
189+
-- `interval day to second(3)`.
190+
format_type(a.atttypid, a.atttypmod) AS formatted_type
181191
FROM pg_attribute a
182192
JOIN pg_class c ON c.oid = a.attrelid
183193
JOIN pg_type t ON t.oid = a.atttypid

‎tests/ColumnTypePrecisionPostgresTest.php‎

Lines changed: 241 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,241 @@
1+
<?php
2+
3+
use DBDiff\DB\Adapters\PostgresAdapter;
4+
use Illuminate\Database\Capsule\Manager as Capsule;
5+
use Illuminate\Database\Events\StatementPrepared;
6+
use Illuminate\Events\Dispatcher;
7+
use PHPUnit\Framework\TestCase;
8+
9+
/**
10+
* Column types that take an optional precision, against a live server
11+
* (issue #215).
12+
*
13+
* `information_schema.columns.datetime_precision` reports 6 both for a column
14+
* declared `timestamptz` and for one declared `timestamptz(6)`, so a generator
15+
* that reads it cannot tell a precision that was asked for from one that was
16+
* merely defaulted. Reading it emitted `timestamptz(6)` for a bare column:
17+
* behaviourally identical, a different type in the catalog, and therefore a
18+
* migration that did not reproduce the column it came from.
19+
*
20+
* The same test was wrong in the other direction — it dropped a declared
21+
* `timestamp(0)`, 0 being a legal precision — and `time`/`timetz` lost any
22+
* precision at all. So the assertions here run in both directions: a precision
23+
* that was never declared must not appear, and one that was must survive.
24+
*
25+
* The final test is the one that matters most. It applies the generated column
26+
* DDL and compares `format_type` on both sides, which is the property a
27+
* consumer verifying a migration actually checks — and is what the reported
28+
* failure looked like from the outside.
29+
*
30+
* Skips automatically when pdo_pgsql is missing or DB_HOST_POSTGRES is unset.
31+
*/
32+
class ColumnTypePrecisionPostgresTest extends TestCase
33+
{
34+
private string $db = 'dbdiff_column_precision';
35+
private ?PDO $adminDb = null;
36+
private $connection;
37+
private PostgresAdapter $adapter;
38+
39+
/** Declared type => what the generated DDL must say. */
40+
private const CASES = [
41+
// No modifier given: nothing may be invented.
42+
'c_timestamptz' => ['timestamptz', 'timestamptz'],
43+
'c_timestamp' => ['timestamp', 'timestamp'],
44+
'c_time' => ['time', 'time'],
45+
'c_timetz' => ['timetz', 'timetz'],
46+
'c_interval' => ['interval', 'interval'],
47+
// A modifier given: it must survive, including a zero.
48+
'p_timestamptz' => ['timestamptz(3)', 'timestamptz(3)'],
49+
'p_timestamp' => ['timestamp(0)', 'timestamp(0)'],
50+
'p_time' => ['time(3)', 'time(3)'],
51+
'p_timetz' => ['timetz(2)', 'timetz(2)'],
52+
// An explicit 6 is not the same statement as no modifier, so it stays.
53+
'e_timestamptz' => ['timestamptz(6)', 'timestamptz(6)'],
54+
// interval's modifier carries a field range as well as a precision,
55+
// which no single number can express.
56+
'i_fields' => ['interval day to second', 'interval day to second'],
57+
'i_both' => ['interval day to second(3)', 'interval day to second(3)'],
58+
'i_year' => ['interval year to month', 'interval year to month'],
59+
// Types whose optional modifier already worked, kept as guards.
60+
'n_numeric' => ['numeric', 'numeric'],
61+
'n_numeric_ps' => ['numeric(10,2)', 'numeric(10,2)'],
62+
'v_varchar' => ['varchar', 'varchar'],
63+
'v_varchar_len' => ['varchar(50)', 'varchar(50)'],
64+
];
65+
66+
private function dropScratchDb(): void
67+
{
68+
try {
69+
$this->adminDb->exec("DROP DATABASE IF EXISTS {$this->db} WITH (FORCE)");
70+
} catch (PDOException $e) {
71+
$this->adminDb->exec("DROP DATABASE IF EXISTS {$this->db}");
72+
}
73+
}
74+
75+
protected function setUp(): void
76+
{
77+
if (!extension_loaded('pdo_pgsql')) {
78+
$this->markTestSkipped('pdo_pgsql extension not loaded.');
79+
}
80+
$host = getenv('DB_HOST_POSTGRES') ?: null;
81+
if (!$host) {
82+
$this->markTestSkipped('DB_HOST_POSTGRES env var not set.');
83+
}
84+
85+
// Same convention as PgDumpRendererPostgresTest: CI sets both, and a
86+
// local server on another port needs only the env var.
87+
$port = getenv('DB_PORT_POSTGRES') ?: '5432';
88+
$user = getenv('DB_USER_POSTGRES') ?: 'dbdiff';
89+
$pass = getenv('DB_PASS_POSTGRES') ?: 'rootpass';
90+
91+
$this->adminDb = new PDO(
92+
"pgsql:host=$host;port=$port;dbname=diff1",
93+
$user,
94+
$pass,
95+
[PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]
96+
);
97+
$this->dropScratchDb();
98+
$this->adminDb->exec("CREATE DATABASE {$this->db}");
99+
100+
$capsule = new Capsule;
101+
$dispatcher = new Dispatcher();
102+
$dispatcher->listen(StatementPrepared::class, function ($event) {
103+
$event->statement->setFetchMode(PDO::FETCH_ASSOC);
104+
});
105+
$capsule->setEventDispatcher($dispatcher);
106+
$capsule->addConnection([
107+
'driver' => 'pgsql',
108+
'host' => $host,
109+
'port' => $port,
110+
'database' => $this->db,
111+
'username' => $user,
112+
'password' => $pass,
113+
'charset' => 'utf8',
114+
'schema' => 'public',
115+
], 'precision');
116+
117+
$this->connection = $capsule->getConnection('precision');
118+
$this->adapter = new PostgresAdapter();
119+
120+
$cols = [];
121+
foreach (self::CASES as $name => [$declared, $_expected]) {
122+
$cols[] = "$name $declared";
123+
}
124+
125+
$this->connection->unprepared(
126+
"CREATE TABLE declared (id integer PRIMARY KEY, " . implode(', ', $cols) . ");\n"
127+
. "CREATE TABLE empty_copy (id integer PRIMARY KEY);"
128+
);
129+
}
130+
131+
protected function tearDown(): void
132+
{
133+
if ($this->adminDb) {
134+
$this->dropScratchDb();
135+
$this->adminDb = null;
136+
}
137+
}
138+
139+
/** The generated column DDL, keyed by column name. */
140+
private function columnDdl(): array
141+
{
142+
return $this->adapter->getTableSchema($this->connection, 'declared')['columns'];
143+
}
144+
145+
public function testEmitsNoPrecisionThatWasNeverDeclared(): void
146+
{
147+
$columns = $this->columnDdl();
148+
149+
foreach (['c_timestamptz', 'c_timestamp', 'c_time', 'c_timetz', 'c_interval'] as $name) {
150+
$this->assertSame(
151+
'"' . $name . '" ' . self::CASES[$name][1],
152+
$columns[$name],
153+
"$name was declared without a precision, so none may be emitted"
154+
);
155+
}
156+
}
157+
158+
public function testKeepsAPrecisionThatWasDeclared(): void
159+
{
160+
$columns = $this->columnDdl();
161+
162+
foreach (['p_timestamptz', 'p_timestamp', 'p_time', 'p_timetz', 'e_timestamptz'] as $name) {
163+
$this->assertSame(
164+
'"' . $name . '" ' . self::CASES[$name][1],
165+
$columns[$name],
166+
"$name declared a precision, which must survive"
167+
);
168+
}
169+
}
170+
171+
public function testKeepsAPrecisionOfZero(): void
172+
{
173+
// Guarded on its own because the previous implementation tested
174+
// `datetime_precision > 0`, which silently discarded this one.
175+
$this->assertSame('"p_timestamp" timestamp(0)', $this->columnDdl()['p_timestamp']);
176+
}
177+
178+
public function testKeepsAnIntervalFieldRange(): void
179+
{
180+
$columns = $this->columnDdl();
181+
182+
foreach (['i_fields', 'i_both', 'i_year'] as $name) {
183+
$this->assertSame(
184+
'"' . $name . '" ' . self::CASES[$name][1],
185+
$columns[$name],
186+
"$name carries a field range, not just a precision"
187+
);
188+
}
189+
}
190+
191+
public function testEveryCaseMatchesItsDeclaration(): void
192+
{
193+
$columns = $this->columnDdl();
194+
195+
foreach (self::CASES as $name => [$_declared, $expected]) {
196+
$this->assertSame('"' . $name . '" ' . $expected, $columns[$name], $name);
197+
}
198+
}
199+
200+
/**
201+
* The property the issue was really about: applying the generated DDL has to
202+
* produce the same catalog type, or a consumer that verifies convergence
203+
* rejects an otherwise valid migration.
204+
*/
205+
public function testGeneratedDdlReproducesTheCatalogType(): void
206+
{
207+
$columns = $this->columnDdl();
208+
209+
$adds = [];
210+
foreach (array_keys(self::CASES) as $name) {
211+
$adds[] = "ALTER TABLE empty_copy ADD COLUMN {$columns[$name]};";
212+
}
213+
$this->connection->unprepared(implode("\n", $adds));
214+
215+
$typesOf = function (string $table): array {
216+
$rows = $this->connection->select(
217+
"SELECT a.attname, format_type(a.atttypid, a.atttypmod) AS rendered
218+
FROM pg_attribute a
219+
WHERE a.attrelid = ?::regclass AND a.attnum > 0 AND NOT a.attisdropped
220+
ORDER BY a.attname",
221+
[$table]
222+
);
223+
$out = [];
224+
foreach ($rows as $row) {
225+
$out[$row['attname']] = $row['rendered'];
226+
}
227+
return $out;
228+
};
229+
230+
$source = $typesOf('declared');
231+
$applied = $typesOf('empty_copy');
232+
233+
foreach (array_keys(self::CASES) as $name) {
234+
$this->assertSame(
235+
$source[$name],
236+
$applied[$name] ?? null,
237+
"applying the generated DDL for $name did not reproduce its type"
238+
);
239+
}
240+
}
241+
}

‎tests/Unit/PostgresBulkSchemaTest.php‎

Lines changed: 45 additions & 4 deletions

0 commit comments

Comments
 (0)