[Kiran01bm]

The drain that runs inside the swap transaction now renders binary values as text in the same format as the copy, the catch-up, and the verifier.

Why

A copy-and-swap copies a table into a new one, keeps it up to date with live changes, and at the swap applies the last few changes inside the same transaction that swaps the tables. That last step could write binary data converted to text in a different format than the rest of the copy, depending on how the database or role was configured. The new table then disagreed with itself, and nothing checks the table after the swap. This PR makes the last step use the same format as everything else and leaves the caller's own setting as it found it.

What

• FlushIn pins chunksql.Rendering (extra_float_digits = 3, bytea_output = 'hex'), the pin Flush, the copier, and the verifier's digest already use, instead of extra_float_digits alone.
• It reads the caller's bytea_output alongside extra_float_digits and restores both, transaction-local, in one statement before returning.
• Integration tests: a bytea → text and a float8 → text conversion drained by FlushIn land the copier's text under a caller set to escape / extra_float_digits = 0, the caller's settings are restored, and the connection keeps its own settings after the commit.
• docs/limitations.md names the swap's drain among the writers that pin both settings.

Before / after

One source column body bytea is retyped with ALTER COLUMN body TYPE text USING body::text, and the database is configured with bytea_output = 'escape'. Row 1 is copied by the chunk copy and row 2 changes late, so the swap's drain writes it; both hold the bytes \x01ff. Only the drain's output changes; the copy, the catch-up flush, and the verifier already wrote hex and are unchanged.

Before
  row 1, chunk copy:  pins hex           ->  body::text  ->  stored "\x01ff"
  row 2, swap drain:  keeps the caller's
                      escape setting      ->  body::text  ->  stored "\001\377"
  swap commits; no verifier runs after it: the new table holds the same bytes
  as two different texts

After
  row 1, chunk copy:  pins hex           ->  body::text  ->  stored "\x01ff"
  row 2, swap drain:  pins hex           ->  body::text  ->  stored "\x01ff"
                      then puts the caller's escape setting back
  swap commits: both rows hold the same text, as the source bytes are equal

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-30T00:25:06.322218Z We couldn’t complete this review. Comment @codex review to try again.	3ec0c05	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

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

[JashLal]

🤖 Correctness review on Jash's behalf at head 3ec0c05.

Verdict: no concrete blocker found in the full diff; surrounding-source and local-test verification are incomplete. FlushIn now captures both caller output settings, uses the shared rendering settings for the drain, and restores bytea_output with a parameterized transaction-local set_config. The integration test checks actual copied and drained text under an escape-configured caller, then checks that the caller's setting is restored. That targets the data inconsistency described in the PR rather than merely asserting a configuration statement ran.

1. (Optional) In pkg/applier/flush_in_integration_test.go, set a nondefault extra_float_digits value in the same new test and assert both settings after FlushIn. This would cover the complete two-setting save/restore contract together.

Verification: read the description, full diff, visible issue comments and check results. All 16 checks succeeded, including PostgreSQL 14–18. No local build or tests ran. Exact-head surrounding-source API access failed to connect; the shared Rendering constant, unchanged error/transaction paths, repository instructions outside the diff and inline review threads were not independently verified. Generated by Codex.

[JashLal]

🤖 Product review at 3ec0c05.

OSS adoption: Binary-to-text conversions become consistent across copy and final drain even when a role uses escape output. The fix requires no new flag or configuration change, and the integration test captures the observable stored-value difference.

SchemaBot integration: FlushIn keeps its existing API and restores the caller's bytea_output on the successful path, so callers can use the same transaction without compensating for this setting. Downstream transaction/error handling was not independently verified. Generated by Codex.

[JashLal]

🤖 Approving on behalf of Jash after the review at 3ec0c05. Findings and verification limitations remain as recorded above. This approval follows the authorized workflow and does not resolve them. No local build or tests ran. Generated by Codex.

[aparajon]

🤖 Review 1/2 of 3ec0c05: adversarial correctness, 0 blocking, 2 non-blocking.

Inspected the full diff against the merge base (pkg/applier/flush_in.go, pkg/applier/flush_in_integration_test.go), the shared pin it now uses (chunksql.Rendering), the catch-up flush's own pin (setFlushSession), and the only caller of FlushIn, the swap's drain (flushBatch). Ran the FlushIn tests against PostgreSQL 16, mutation-tested every term of the change, and wrote two probe tests.

The change is correct as written. pinFlushSettings now reads extra_float_digits and bytea_output in one statement (L102-L116) and pins both with chunksql.Rendering, the same constant the copier, the verifier, and setFlushSession use, so a bytea converted to text by the drain renders as the copy rendered it (\x01ff, not \001\377). The caller's values are put back at L87-L92, after the second resolution check and only on the success path; every failure leaves the transaction for the caller to roll back, which the doc comment already says. A transaction-local set_config(..., true) with a bind parameter is the right restore: SET LOCAL cannot take one, and the value read back is already one of the two valid enum strings. The empty batch still touches nothing.

Invariants. CO-9 (decision reads resolve to the real catalog): upholds. The // INV: CO-9 pin of the search path (L111) is untouched and still runs in the same statement as the rendering pin. CO-6 and CO-4 (the drain converges or refuses): upholds, no control flow moved. No registry entry names the rendering pin, so nothing in docs/invariants.md needs an edit.

Non-blocking

1. The restore's transaction scope is not pinned: a session-scoped restore passes every test

The restore uses set_config('bytea_output', $1, true), so the caller's value stays transaction-local and the connection returns to its own setting at commit. Changing the true to false makes the restore session-scoped: after the caller commits, the pooled connection keeps whatever the caller had in the transaction. That differs from the connection's own setting exactly when the caller set it with SET LOCAL, as the new test does. The new test reads current_setting before the commit (L93), where both scopes read escape, so the mutant survives. Latent today: the one caller, the swap's drain, commits and then releases its connection, but FlushIn is exported and the doc comment promises the caller's settings are put back.

Test case: passes on 3ec0c05, fails with the restore made session-scoped

package applier_test

import (
	"testing"

	"github.com/jackc/pgx/v5"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
)

// The caller's bytea_output is the caller's own for the transaction only:
// once it commits, the connection reads the setting it had before.
func TestFlushInLeavesTheConnectionsBytea(t *testing.T) {
	f := newFlushFixture(t)
	f.createOrders(t, 10)
	p := f.prepare(t, "orders")
	conn, err := f.pool.Acquire(t.Context())
	require.NoError(t, err)
	defer conn.Release()
	var before string
	require.NoError(t, conn.QueryRow(t.Context(), `SELECT current_setting('bytea_output')`).Scan(&before))
	tx, err := conn.BeginTx(t.Context(), pgx.TxOptions{AccessMode: pgx.ReadWrite})
	require.NoError(t, err)
	_, err = tx.Exec(t.Context(), `SET LOCAL bytea_output = 'escape'`)
	require.NoError(t, err)
	_, err = p.flush.FlushIn(t.Context(), tx, batch(image(5, col("qty", "500"), col("note", "n"))))
	require.NoError(t, err)
	require.NoError(t, tx.Commit(t.Context()))
	var after string
	require.NoError(t, conn.QueryRow(t.Context(), `SELECT current_setting('bytea_output')`).Scan(&after))
	assert.Equal(t, before, after, "the connection's own bytea_output outlives the transaction")
}

--- FAIL: TestFlushInLeavesTheConnectionsBytea (0.72s) with expected: "hex", actual : "escape" under the true to false mutant; passes on 3ec0c05.

2. FlushIn has no test that pins extra_float_digits, so the shared constant could lose that half unnoticed

The PR moves the drain from an inline SET LOCAL extra_float_digits = 3 to chunksql.Rendering. The new test covers the bytea_output half. Nothing runs a float through FlushIn: pinning only bytea_output in pinFlushSettings (mutant M8 below) leaves every FlushIn test green, and the float agreement test in flusher_conversion_integration_test.go exercises Flush, not FlushIn. This gap predates the PR, which is why it is not a finding against the change, but the swap drain is the one write no verifier follows, and the PR is the natural place to close it. It also answers the optional note in the earlier review comment about covering both settings together.

Test case: passes on 3ec0c05, fails when the drain stops pinning extra_float_digits

package applier_test

import (
	"testing"

	"github.com/jackc/pgx/v5"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"

	"github.com/block/pg-sprite/pkg/applier"
	"github.com/block/pg-sprite/pkg/copier"
)

func TestFlushInConvertsAFloatToTextAsTheCopyDoes(t *testing.T) {
	f := newFlushFixture(t)
	f.exec(t, `
		CREATE TABLE %s.readings (
			id bigint PRIMARY KEY,
			value float8,
			note text
		)`)
	f.exec(t, `INSERT INTO %s.readings VALUES (1, 0.1::float8 + 0.2::float8, 'n'), (2, 0.1::float8 + 0.2::float8, 'n')`)
	target := f.prove(t, "readings")
	lock := f.lock(t, "readings")
	shadow := f.build(t, lock, target, `ALTER TABLE %s ALTER COLUMN value TYPE text USING value::text`)
	c, err := copier.NewCopier(target, shadow, lock, copier.Watermark{}, copier.Options{Workers: 1})
	require.NoError(t, err)
	require.NoError(t, c.Run(t.Context(), f.poolWithRuntimeParam(t, "extra_float_digits", "0")))
	copied, _ := f.text(t, shadow.ShadowTable(), "value", 1)
	flush, err := applier.NewFlusher(target, shadow, lock, applier.Options{})
	require.NoError(t, err)
	tx, err := f.pool.BeginTx(t.Context(), pgx.TxOptions{AccessMode: pgx.ReadWrite})
	require.NoError(t, err)
	_, err = tx.Exec(t.Context(), `SET LOCAL extra_float_digits = 0`)
	require.NoError(t, err)
	_, err = flush.FlushIn(t.Context(), tx, batch(image(2, col("value", "0.30000000000000004"), col("note", "n"))))
	require.NoError(t, err)
	var digits string
	require.NoError(t, tx.QueryRow(t.Context(), `SELECT current_setting('extra_float_digits')`).Scan(&digits))
	assert.Equal(t, "0", digits)
	require.NoError(t, tx.Commit(t.Context()))
	drained, _ := f.text(t, shadow.ShadowTable(), "value", 2)
	require.NotNil(t, drained)
	assert.Equal(t, "0.30000000000000004", *copied)
	assert.Equal(t, *copied, *drained, "the same source value converted by the copy and by the drain")
}

--- FAIL: TestFlushInConvertsAFloatToTextAsTheCopyDoes (0.55s) with expected: "0.30000000000000004", actual : "0.3" when the pin is reduced to SET LOCAL bytea_output = 'hex'; passes on 3ec0c05.

Verified

• Pin and restore order. Read before pin, pinned before the guards and the write, restored after the second confirmResolution; a restore that ran before the write fails the new test.
• Test fails without the fix. With the drain pinning extra_float_digits only (the pre-PR behavior), TestFlushInConvertsAByteaToTextAsTheCopyDoes fails: expected: "\\x01ff", actual : "\\001\\377", message "the same source value converted by the copy and by the drain".
• Serial reruns. go test ./pkg/applier/ -run 'Flush|Drain' passed four times in a row. One earlier run, made at the same time as a pkg/schemachange run against the same server, failed once and did not reproduce.
• CI. All 16 checks on the head are green, PostgreSQL 14 through 18 included.

Mutant	Caught by
pin extra_float_digits only (pre-PR drain)	TestFlushInConvertsAByteaToTextAsTheCopyDoes
no bytea_output restore	TestFlushInConvertsAByteaToTextAsTheCopyDoes
restore bytea_output as the constant hex	TestFlushInConvertsAByteaToTextAsTheCopyDoes
restore bytea_output before the write	TestFlushInConvertsAByteaToTextAsTheCopyDoes
no extra_float_digits restore	TestFlushInWritesOnlyWhenTheCallerCommits
restore session-scoped (true to false)	survives (finding 1)
M8: pin bytea_output only, drop extra_float_digits	survives (finding 2)

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

[aparajon]

🤖 Review 2/2 of 3ec0c05: OSS adoption and SchemaBot integration lenses, 0 blocking, 2 non-blocking.

OSS adoption. The fix needs no flag, option, or configuration: an adopter whose database or role sets bytea_output = 'escape' gets the same stored text from the swap's drain as from the copy, and the one place that wrote differently is closed. FlushIn's signature and error taxonomy are unchanged, and its doc comment (L13-L31) now says which settings it pins and which it restores, so an importer reading the godoc learns the contract without reading the body. The restore is transaction-local, so a pooler in transaction mode sees nothing leak between transactions. Two gaps below, both small.

SchemaBot integration. A caller that opens its own transaction, sets its own session GUCs, and calls FlushIn on the success path gets its extra_float_digits and bytea_output back, so it needs no compensation. On a failure the caller rolls back, which FlushIn already documents and which discards the pinned values with the transaction. The drain is the one caller in this module (flushBatch), and an importer that wraps FlushIn in its own swap does not need to change anything.

Non-blocking

1. The limitations page still lists three writers, not four

docs/limitations.md#L75-L79 says the float4/float8 to text and bytea to text casts follow extra_float_digits and bytea_output, and that "the copy, the verifier, and the catch-up each pin both settings" so such a conversion lands the same text "from all three". The swap's drain is now a fourth writer that pins both, and it is the one an adopter would worry about most, because the paragraph just above (L65-L72) says no comparison follows it. Adding "and the swap's drain" and changing "all three" to "all of them" keeps the page true to the code.

2. The restore costs one more statement under the swap's lock

FlushIn runs while the swap holds its lock, and the restore adds one round trip per batch (L90) next to the existing SET LOCAL extra_float_digits one (L87); the drain flushes up to two batches. One SELECT set_config('extra_float_digits', $1, true), set_config('bytea_output', $2, true) would restore both in one statement, and would also stop the two restores using different forms (SET LOCAL with a formatted integer, and set_config with a parameter). I have not measured the difference; on a same-region target it is small, and an importer whose database is a network hop away holds the swap's lock for it. Not worth blocking on, and the current form is correct.

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

[aparajon]

🤖 Stamping with comments: 0 blocking, 4 non-blocking (see the two review comments above).

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

[Kiran01bm]

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

All 5 findings are fixed in the follow-up commit; nothing is deferred or rejected.

#	Finding	Status	Explanation
C3-F1	A session-scoped restore (true → false) passed every test	fixed	TestFlushInLeavesTheConnectionsOwnOutputSettings sets both settings with SET LOCAL, commits, and asserts the connection reads its own values; fails if either restore is made session-scoped.
C3-F2	No FlushIn test pins extra_float_digits	fixed	TestFlushInConvertsAFloatToTextAsTheCopyDoes drains float8 → text under extra_float_digits = 0; fails with 0.3 when the drain pins bytea_output only, and when the caller's value is not put back.
C1-F1	Cover a nondefault extra_float_digits and assert both settings after FlushIn	fixed	Covered by the two tests above: the float test restores 0, and the connection test asserts both settings together.
C4-F1	docs/limitations.md lists three writers that pin both settings, not four	fixed	The page now names the swap's final drain; the chunksql.Rendering comment lists it too.
C4-F2	The restore costs an extra round trip under the swap's lock, in two different forms	fixed	One SELECT set_config('extra_float_digits', $1, true), set_config('bytea_output', $2, true) restores both; the integer parse it no longer needs is gone.
C2-F1	Downstream transaction and error handling not independently verified	no action	Every failure leaves the transaction for the caller to roll back, as FlushIn documents; TestFlushInLeavesTheTransactionAbortedWhenTheBatchIsDeferred pins it.
Approvals	Verified-correct notes: pin and restore order, the regression test failing without the fix, CI on 14–18	no action	Confirmations, no findings.

Source: block/pg-sprite#251, review comments 6515359367, 6515359689, 6567760477 and 6567761385 and reviews 5708258200 and 5731560446 at head 3ec0c058; fixes in the follow-up commit