checksum: chunk verifier comparing source and shadow in one snapshot (CO-1)

[Kiran01bm]

Adds checksum.Verifier: the per-chunk comparison of a source table with its copied shadow that the cutover gate will demand, reporting every chunk whose rows differ up to the copier's landed watermark.

Why

Nothing yet compares what the copier wrote with what the source holds. The gate (CO-1) needs a comparison that is exact — both sides read at the same instant, through the types the shadow declares (D7), so a widened or converted column is not a false mismatch — and that holds no snapshot longer than one chunk, so a long table does not pin xmin for the whole pass. Landing the comparison on its own keeps the divergence policy, the repair primitive, and the VerifiedShadow / CleanWatermark constructors, which decide what a difference means, in their own review.

What

• pkg/checksum:
  • NewVerifier(target, shadow, lock, opts) takes the same three proofs as the copier and applies the same refusals (ST-6 shadow-proof checks, LK-1 lock-session checks, LK-2 timeout floor); the shadow is the copier.Shadow shape schemachange.BuiltShadow satisfies.
  • Verify(ctx, pool, through) reads the shadow's column types once, cuts its own chunks from the live source with a copier.Chunker, and digests each chunk in one read-only REPEATABLE READ transaction: LOCK TABLE source, shadow IN ACCESS SHARE MODE first so the snapshot is taken with both relations held, then Confirm on the lock session and the relation-OID check, then the two reads. The chunk that straddles the watermark is clamped to it; nothing above is compared. Every transaction runs under the lock session's Bind context, and a lost lock is reported over whatever the read returned.
  • The digest is one frozen statement per side, differing only in the table: count(*) and md5(string_agg(md5(ROW(col::shadow_type, …)::text), '' ORDER BY pk)) over pk BETWEEN $1::bigint AND $2::bigint, every function pg_catalog-qualified and the transaction's search_path set to pg_catalog alone (CO-9). Only the copy columns are compared; hashing generated columns present on both sides is a follow-up.
  • Report{Through, Chunks, Rows, Mismatches} with Mismatch{Chunk, Source, Shadow Digest}; Report.Clean(). A report is not a proof: the constructors of VerifiedShadow and CleanWatermark stay private and unwired.
• Docs in the same PR: SAFETY.md pkg/checksum row, copy-and-swap-design.md package map and D7 "where enforced", invariants.md CO-1 / CO-9 / LK-1 "enforced today", architecture.md package table.
• Tests on real PostgreSQL: a complete copy reports clean with the source row count; one changed shadow row is located to exactly its chunk with equal row counts and differing hashes; a missing and an extra shadow row are two mismatches whose row counts say which way each differs; an integer → numeric(10,2) change compares clean; a watermark inside a chunk clamps it and ignores a difference above; a shadowing search_path with decoy md5 and format_type changes no answer; a pool whose sessions start at extra_float_digits = 0 still finds a float that differs beyond the fifteenth digit; a write issued on the verify transaction's own connection between a chunk's two digests is refused read_only_sql_transaction; a replaced or dropped relation and a gone, rival, lost, or mid-pass-lost lock refuse fail-closed. Removing the casts fails the type and search_path tests; unqualifying md5 and dropping the local search_path fails the search_path test.

Before / after

Before                                        After
┌────────┐  copy   ┌────────┐                 ┌────────┐  copy   ┌────────┐
│ source │────────▶│ shadow │                 │ source │────────▶│ shadow │
└────────┘         └────────┘                 └───┬────┘         └───┬────┘
                                                  │  one REPEATABLE READ tx per chunk,
   nothing compares them; the shadow's            │  ACCESS SHARE on both, lock confirmed,
   fidelity is asserted, not measured             │  OIDs checked, then:
                                                  ▼                  ▼
                                            count, md5(rows::shadow types) ── equal? ──▶ Report
                                                  [pk BETWEEN lo AND hi]      no ──▶ Mismatch{chunk, digests}
                                                  … up to the landed watermark

References

• docs/copy-and-swap-design.md D7 (checksum through the destination types) and the package map.
• docs/invariants.md CO-1, CO-9, LK-1.
• Stack: copier: parallel chunked copy step under the table lock (CO-4, LK-3) #128 (copier, merged), progress: copy operation and engine-measured work source (v4) #131 (progress, merged); independent of copier: fill the copy step's progress counters from the copier #132.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 1/2: adversarial correctness review of 8eae0a8d. I read pkg/checksum against the copier's guard, the chunker's tiling, and CO-1 / CO-9 / LK-1 / ST-6. Probes and mutations ran on real PostgreSQL, using the PR's own fixture.

1 blocking, 3 non-blocking.

The guard and the tiling hold. The chunker's last chunk is open above, so every pass reaches the watermark, including on an empty source. LOCK TABLE comes before the first snapshot-taking query, so the snapshot is taken with both relations held. I found no type where the explicit ::shadow_type cast and the copier's assignment cast both succeed but give different values (by reading the length-coercion rules, not by test). Of 15 mutations, 9 are caught. The survivors are below.

Blocking

1. A float difference compares clean when the database or role sets extra_float_digits <= 0. guard.go:66-68

The digest hashes each row's text rendering (ROW(…)::text). For float4, float8 and the geometric types, that rendering follows the session's extra_float_digits. At 0 or below, PostgreSQL rounds to 15 significant digits, so 0.3 and 0.1+0.2 render the same, and a shadow row that differs from its source hashes equal.

That setting is legal at the database or role level (ALTER DATABASE … SET extra_float_digits = 0), and setVerifySession pins the timeouts and search_path but not this. So the comparison's resolution is whatever the caller's configuration says. The PR registers the Verifier as CO-1's enforcement today, so this is a confidently clean answer on the gate's comparison.

Measured: the default session finds the changed row. A pool on a database with extra_float_digits = 0 reports Clean() == true.

Fix, one line, no other test changes:

	budgets := "SET LOCAL lock_timeout = " + strconv.FormatInt(opts.LockTimeout.Milliseconds(), 10) +
		"; SET LOCAL statement_timeout = " + strconv.FormatInt(opts.StatementTimeout.Milliseconds(), 10) +
		"; SET LOCAL extra_float_digits = 3" +
		"; " + dbconn.LocalSearchPath("pg_catalog")

This is the only lossy output setting I found. DateStyle, IntervalStyle, TimeZone and bytea_output change the spelling but never merge two distinct values, and both sides share the session.

Test that fails on 8eae0a8 and passes with the fix

// A float that differs in its last significant digit is a mismatch whatever
// extra_float_digits the database or role configures: the digest renders
// rows as text, and at extra_float_digits <= 0 that text rounds to 15
// digits, so 0.3 and 0.1+0.2 would hash the same.
func TestVerifierHashesFloatsAtFullPrecision(t *testing.T) {
	f := newVerifierFixture(t)
	f.exec(t, `CREATE TABLE %s.readings (id bigint PRIMARY KEY, v double precision NOT NULL, note text)`)
	f.exec(t, `INSERT INTO %s.readings SELECT n, 0.3, 'r' FROM generate_series(1, 2500) n`)
	target := f.prove(t, "readings")
	lock := f.lock(t, "readings")
	shadow := f.build(t, lock, target, `ALTER TABLE %s DROP COLUMN note`)
	f.copy(t, target, shadow, lock)
	f.exec(t, "UPDATE "+f.shadowName(shadow)+" SET v = 0.1::float8 + 0.2::float8 WHERE id = 7")
	f.exec(t, `DO $$ BEGIN EXECUTE format('ALTER DATABASE %I SET extra_float_digits = 0', current_database()); END $$`)
	pool, err := dbconn.NewPool(t.Context(), f.cfg)
	require.NoError(t, err)
	t.Cleanup(pool.Close)

	report, err := f.verify(t, pool, target, shadow, lock, copier.NewWatermark(math.MaxInt64))
	require.NoError(t, err)
	require.Len(t, report.Mismatches, 1)
	assert.Equal(t, chunk(t, math.MinInt64, 1000), report.Mismatches[0].Chunk)
}

--- FAIL: TestVerifierHashesFloatsAtFullPrecision (1.88s) on 8eae0a8d ("[]" should have 1 item(s), but has 0). --- PASS: TestVerifierHashesFloatsAtFullPrecision (1.89s) with the fix.

Non-blocking

2. The missing/extra-row test cannot tell source rows from shadow rows. verifier_integration_test.go:205,218

The one missing and one extra shadow row cancel out: 999 + 1001 + 500 and 1000 + 1000 + 500 are both 2500. So assert.Equal(t, int64(rows), report.Rows, "the report counts source rows") holds even when verifier.go:217 adds shadow.Rows, and that mutation survives the package. Deleting two rows makes the counts differ.

Test change that passes on 8eae0a8 and fails with report.Rows += shadow.Rows

	f.exec(t, "DELETE FROM "+f.shadowName(shadow)+" WHERE id IN (5, 6)")
	// …
	assert.Equal(t, int64(998), missing.Shadow.Rows)

ok at 8eae0a8d. --- FAIL: TestVerifierCountsMissingAndExtraShadowRows (2.36s) under the mutation.

3. The transaction's search_path layer is not pinned by any test (CO-9 test obligation).

Every function is pg_catalog-qualified, so the md5 and format_type decoys pass even with dbconn.LocalSearchPath("pg_catalog") removed from guard.go:68, and that mutation survives. What the local path actually protects is the operators that cannot be qualified: BETWEEN at digest.go:102, and the = joins in relationOIDsSQL.

A decoy <= operator turns the survivor into a false clean that the existing assertion catches.

Test change that passes on 8eae0a8 and fails without the local search_path

Add to TestVerifierIgnoresTheSessionSearchPath, before NewCatalogShadowingPool:

	f.exec(t, `CREATE FUNCTION %s.int8le(bigint, bigint) RETURNS boolean LANGUAGE sql IMMUTABLE AS 'SELECT false'`)
	f.exec(t, `CREATE OPERATOR %s.<= (LEFTARG = bigint, RIGHTARG = bigint, FUNCTION = %s.int8le)`)

ok at 8eae0a8d. Without the local path: --- FAIL: TestVerifierIgnoresTheSessionSearchPath (1.96s) ("[]" should have 1 item(s), but has 0). Both sides then match no rows and compare clean.

4. Four guard properties survive mutation. Each is untested rather than wrong:

• pgx.RepeatableRead → ReadCommitted (guard.go:27). The single-snapshot property needs a write landing between the two digests to show.
• Dropping SET LOCAL ROLE (guard.go:72). The superuser fixture reads everything either way.
• Dropping AccessMode: pgx.ReadOnly.
• Dropping the missing-copy-column refusal in shadowColumnTypes (digest.go:68).

Invariants: this extends CO-1 (the comparison exists, and the gate stays unwired), CO-9 and LK-1, and upholds ST-6. CO-1's Enforced today line is accurate once finding 1 is fixed.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 2/2: OSS adoption and integration ease, at 8eae0a8d. These are lenses, not correctness findings. 0 blocking, 3 non-blocking.

1. Progress: a pass has no progress.WorkSource. The copier fills the tracker's copy counters while Run is in progress. Verify reports nothing until it returns a Report. A schema change orchestrator like SchemaBot renders what the engine reports and never projects it. So the checksum phase on a large table would show no movement between start and finish. A rows-hashed / rows-to-through source, shaped like the copier's, would let importers render it with no extra code.

2. Throughput: a pass is serial. Each chunk costs about ten round trips: the boundary query, begin, the budgets, the role, the lock, Confirm, the OID check, two digests and the commit. Chunks are also strictly sequential, while the copier runs workers. Chunk sizing grows to DefaultMaxChunkRows, so the round trips amortize. But a verify pass on a large table will take a multiple of the copy's wall time. A Workers option later is natural, since each chunk transaction is already independent and short-lived.

3. Portability: md5() and FIPS (not run here; no FIPS build available). On PostgreSQL 14+ built against OpenSSL, md5() goes through OpenSSL, and an OpenSSL in FIPS mode refuses MD5. The digest then errors. That is fail-closed, but it means the verifier cannot run on FIPS hosts. pg_catalog.sha256(pg_catalog.convert_to(…, 'UTF8')) is the portable choice. It is worth deciding before VerifiedShadow freezes the digest's shape.

The API is otherwise easy to adopt. It takes the same three proofs as the copier, refusals are errors.Is-able ErrInvariantViolation values, ErrNothingLanded is distinct, and a mismatch is data, not an error.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving 8eae0a8d with 1 blocking finding in the 1/2 comment. Pin extra_float_digits in setVerifySession, or a database or role at <= 0 compares a float difference clean. The fix is one line, and the comment includes a test that fails today. The 2/2 comment has three non-blocking integration notes.

This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/134, follow-up commit

Verdict: the blocking finding is fixed in the follow-up commit with the suggested test; three of the four non-blocking correctness findings are fixed (the fourth is three parts fixed, one part rejected); the three integration-lens notes are tracked in the delivery plan rather than taken here.

#	Finding	Status	Explanation
C1-F1	A float difference compares clean when the database or role sets extra_float_digits <= 0 (blocking)	✅ Fixed	setVerifySession now pins SET LOCAL extra_float_digits = 3 alongside the timeouts and search_path, and its comment says why: the digest hashes each row's text rendering, and for float4, float8, and the geometric types that rendering follows the setting. The suggested TestVerifierHashesFloatsAtFullPrecision is in, adapted to the fixture (readable DDL, a SHOW extra_float_digits guard on the second pool so the test proves the database setting actually reached the session it reads on, and a cleanup that resets the database setting so a shared PG_DSN database is left as it was). It fails on 8eae0a8d and passes with the pin. The design-doc row for pkg/checksum names the pin.
C1-F2	The missing/extra-row test cannot tell source rows from shadow rows	✅ Fixed	Two shadow rows are deleted (id IN (5, 6)) and the first mismatch asserts Shadow.Rows == 998, so the source and shadow totals differ (2500 vs 2499) and report.Rows += shadow.Rows now fails the test; confirmed by running that mutation.
C1-F3	The transaction's search_path layer is not pinned by any test (CO-9)	✅ Fixed	TestVerifierIgnoresTheSessionSearchPath adds the suggested decoy: a bigint <= operator in the shadowing schema backed by a function that is never true, which would empty both sides of the key range and compare nothing clean. Removing LocalSearchPath("pg_catalog") from the guard now fails the test; confirmed by running that mutation. The CO-9 Enforced line in docs/invariants.md names the operator decoy as the layer only the local search_path can pin.
C1-F4	Four guard properties survive mutation: RepeatableRead, SET LOCAL ROLE, AccessMode: ReadOnly, the missing-copy-column refusal	✅ Fixed (3 of 4) / ❌ Rejected (ReadOnly)	RepeatableRead → fixed: TestVerifierDigestsBothSidesInOneSnapshot reads on a pool whose pgx query tracer, when the source digest of the first chunk completes, updates a shadow row in that chunk from another connection; the pass compares clean and the next pass finds the row. ReadCommitted fails it. SET LOCAL ROLE → fixed: TestVerifierReadsAsTheTableOwner builds on a throwaway NOLOGIN owner (the copier's owned-fixture shape), passes clean, then revokes the owner's own SELECT on the source; the pass ends 42501 because LOCK TABLE … ACCESS SHARE is checked against the owner, not the superuser behind the pool. Dropping the SET LOCAL ROLE fails it. Missing copy column → fixed: TestVerifierRefusesAShadowMissingACopyColumn drops qty from the shadow (same relation, same OID) and asserts the (ST-6) refusal naming the column. Dropping the check in shadowColumnTypes fails it. ReadOnly → rejected: every statement the guarded transaction runs is SET LOCAL, LOCK TABLE … ACCESS SHARE, or a SELECT, all of which a read-only transaction permits, so there is no statement the verifier issues whose outcome the access mode changes and nothing a test can observe without a tracer that rewrites the verifier's own SQL. It stays as a declaration that the pass writes nothing; a test that only passes by asserting on the BEGIN text would make log text a test surface.
C2-F1	Progress: a pass has no progress.WorkSource	⏳ Deferred	Already planned as part of the next checksum PR (cs6-repair-policy, row 6b of the copy-and-swap PR plan: "checksum WorkSource filler (chunks verified)"), where the pass gains the repair loop whose progress is also worth rendering. Not taken here so this PR stays the comparison alone.
C2-F2	Throughput: a pass is serial; a Workers option is natural	⏳ Deferred	Agreed on the shape. Tracked as a new plan row (cs6-verify-workers, 6e) behind 6b, since parallel progress needs the WorkSource to be visible; the key tests are named there (same chunks as a serial pass, ascending mismatch order, lock loss cancels every worker).
C2-F3	Portability: md5() fails under an OpenSSL in FIPS mode; pg_catalog.sha256(pg_catalog.convert_to(…, 'UTF8')) is the portable choice	⏳ Deferred	Agreed; the fail-closed error on a FIPS host is not an acceptable OSS answer. Tracked as its own XS plan row (cs6-digest-sha256, 6d). Deferred rather than taken in this PR because it is a D7 decision change (design text, the frozen-SQL oracle, and the CO-9 decoy move together), and because the "freeze" concern does not bind: a Digest.Hash lives only in an in-memory Report, nothing persists or compares it across runs, so VerifiedShadow in 6b does not fix the algorithm and the swap has no compatibility surface whenever it lands.
R1	Approving review restating C1-F1 as the one blocking item	✅ Fixed	Same change as C1-F1.

Tests added or changed in the follow-up commit: TestVerifierHashesFloatsAtFullPrecision, TestVerifierDigestsBothSidesInOneSnapshot, TestVerifierReadsAsTheTableOwner, TestVerifierRefusesAShadowMissingACopyColumn (new); TestVerifierCountsMissingAndExtraShadowRows, TestVerifierIgnoresTheSessionSearchPath (hardened). Each new or hardened test was run against the mutation it exists to catch and fails under it. go test -race ./pkg/checksum/ and golangci-lint run ./pkg/checksum/... are green.

Source: block/pg-sprite#134, review comments 5922753567 and 5922753763 and review 5373824351 at head 8eae0a8d

[aparajon]

🤖 Delta re-review of da5303c, scoped to the findings on 8eae0a8d.

0 blocking, 2 non-blocking.

Finding 1 (blocking) is fixed. setVerifySession now sets SET LOCAL extra_float_digits = 3 along with the timeouts and search_path. So the digest's text rendering no longer depends on the database or role setting. TestVerifierHashesFloatsAtFullPrecision reproduces the original case: a pool whose session reports extra_float_digits = 0, and a 0.3 vs 0.1+0.2 row. It finds the one mismatch.

The survivors are now pinned. I reran the earlier survivors against the new head on real PostgreSQL. The full pkg/checksum package passes unmutated (247s).

Mutation	Result
drop the extra_float_digits pin	caught by TestVerifierHashesFloatsAtFullPrecision
report.Rows += shadow.Rows	caught by TestVerifierCountsMissingAndExtraShadowRows (two deleted rows now break the tie)
drop the search_path pin	caught by TestVerifierIgnoresTheSessionSearchPath (the decoy <= operator)
RepeatableRead → ReadCommitted	caught by TestVerifierDigestsBothSidesInOneSnapshot
drop SET LOCAL ROLE	caught by TestVerifierReadsAsTheTableOwner
drop the missing-copy-column refusal	caught by TestVerifierRefusesAShadowMissingACopyColumn
drop AccessMode: pgx.ReadOnly	survives

Non-blocking

1. The float test changes a database setting that other packages share. verifier_integration_test.go:265

ALTER DATABASE … SET extra_float_digits = 0 applies to every new session on that database, not just this test's pool. That is harmless with a per-test testcontainer. Under make test, though, PG_DSN points every package in ./... at the same postgres database, and those packages run in parallel. While this test runs, any other package that opens a connection gets rounded float text. The cleanup's RESET also removes any value the database had set before the test.

Setting the parameter on the test pool alone tests the same property without touching shared state. For example, copy f.cfg and set ConnConfig.RuntimeParams["extra_float_digits"] = "0". A startup parameter is the same thing a database or role default delivers: a session-level value that SET LOCAL has to override. The existing SHOW extra_float_digits assertion still proves the value reached the session.

2. Read-only mode is still unpinned. Dropping AccessMode: pgx.ReadOnly survives, as noted last time. A test that tries a write inside the verify transaction would pin it. This is optional, since the guard never writes.

Invariants: unchanged from the first review. This extends CO-1 and CO-9 (the decoy operator is now named in CO-9), and LK-1, and upholds ST-6.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving da5303c: the extra_float_digits finding is fixed and every guard property now has a test. Two non-blocking notes are in the delta comment.

This review was generated by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Response to the re-review of da5303c (#134 (comment)). Both non-blocking findings are fixed in <FIX_SHA>.

#	Finding	Status	Fix
NB-1	TestVerifierHashesFloatsAtFullPrecision sets extra_float_digits with ALTER DATABASE, which reaches every later session on the server (other packages under make test on a shared PG_DSN) and clobbers any pre-existing value.	Fixed	The test now opens the pass's pool with extra_float_digits = 0 as a connection parameter (pgxpool.ParseConfig → ConnConfig.RuntimeParams), via a new poolWithRuntimeParam fixture helper. The SHOW extra_float_digits == "0" assertion on that pool stays, so the test still proves the session the pass reads on starts at 0 and the pass's own SET LOCAL overrides it. No ALTER DATABASE, no cleanup, nothing left on the server. — <FIX_SHA>
NB-2	Mutation "drop AccessMode: pgx.ReadOnly" survives: nothing writes on the verify transaction's own connection.	Fixed	New TestVerifierTransactionRefusesWrites: the query hook now hands the connection it fired on, and the test issues an UPDATE on the shadow on that connection, between a chunk's source and shadow digests. It asserts the server refuses it with SQLSTATE 25006 (read_only_sql_transaction), the pass fails (aborted transaction), and the shadow row is unchanged. Verified the mutant is killed: with AccessMode: pgx.ReadOnly removed from begin, the write lands and the test fails at require.Error. — <FIX_SHA>

Mutation table after this commit: every row in the re-review's table, including "drop AccessMode: pgx.ReadOnly", now has a failing test. The existing TestVerifierDigestsBothSidesInOneSnapshot keeps writing from a separate connection on purpose — that is the property it tests (one snapshot per chunk, CO-1); the new test covers the same-connection case.

Verification: go test -race ./pkg/checksum/ green on PG16; make lint 0 issues. No production code changed; invariants CO-1 / CO-9 / LK-1 / ST-6 unchanged.

---