Skip to content

storage: pin session rendering GUCs on Postgres source connections - #38745

Closed
ohbadiah wants to merge 1 commit into
MaterializeInc:mainfrom
ohbadiah:nickmcavoy/ss-362-pin-session-rendering-gucs-datestyle-intervalstyle-timezone
Closed

storage: pin session rendering GUCs on Postgres source connections#38745
ohbadiah wants to merge 1 commit into
MaterializeInc:mainfrom
ohbadiah:nickmcavoy/ss-362-pin-session-rendering-gucs-datestyle-intervalstyle-timezone

Conversation

@ohbadiah

@ohbadiah ohbadiah commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Pin the PostgreSQL session settings that control text rendering on every connection a Postgres source makes, and fail the connection if the server does not honor them.

Motivation

Source ingestion parses the text PostgreSQL renders, both in COPY snapshots and in the before-images of the replication stream, and a retraction only cancels its insertion if both were rendered identically. That rendering follows DateStyle, IntervalStyle, TimeZone, and extra_float_digits, which a source connection inherits from server or database defaults the customer can change at any point in a source's life. A change picked up by a fresh walsender, after a managed failover, a parameter-group apply, or pg_terminate_backend, renders the before-image differently from the stored row, and full-row reads fail with Non-positive multiplicity in DistinctBy, the signature of incident 1161. SS-376 confirmed the repro on the TimeZone leg with a timestamptz inside a TEXT COLUMNS composite.

Change

mz_postgres_util::Config gains a PINNED_SESSION_SETTINGS constant (DateStyle=ISO, IntervalStyle=postgres, TimeZone=UTC, extra_float_digits=3). Config::new appends them to the startup options string, which already carries wal_sender_timeout and so is known to reach the walsender. After each connect, the settings are read back with current_setting over the simple-query protocol, and a mismatch fails the connection naming the setting and quoting what the server reported. That turns a pooler configured to strip startup options into a loud connection error instead of a silent rendering difference.

PostgresConnection::config in storage-types is the only constructor of this Config, so snapshot COPY, replication, and purification connections all get the pins. No call site changes.

Pins are instant-preserving for rows ingested before the pin: a timestamptz rendered as 17:15:00+02 under Europe/Berlin and as 15:15:00+00 under UTC parses to the same datum, so pre-pin rows still retract cleanly. Any positive extra_float_digits selects shortest round-trip float output.

Tests

  • test/pg-cdc/session-guc-change.td is the SS-376 repro extended to one leg per pinned setting. Each leg changes the setting at the database level, terminates the walsender so the next change is decoded by a fresh session, updates a row, and reads the table back through operators with multiplicity checks. It ends with an assertion on the exact rendered composite text and resets the database-level settings, which outlive the file. Without the pins each leg fails with the DistinctBy error: verified by running with progressively more pins enabled, so every pin has a leg that fails when it alone is removed. With all four pins the file passes, and so does the rest of the pg-cdc suite.

Suggested review order

  1. src/postgres-util/src/tunnel.rs: the constant and its doc comment, then Config::new, then verify_pinned_session_settings.
  2. test/pg-cdc/session-guc-change.td.

Release note

This release will pin DateStyle, IntervalStyle, TimeZone, and extra_float_digits on PostgreSQL source connections, so upstream session-default changes can no longer corrupt ingested rows. Connections through a pooler that strips startup options now fail with a clear error.

Closes: SS-362
Closes: SS-376

🤖 Generated with Claude Code

Set DateStyle, IntervalStyle, TimeZone, and extra_float_digits in the
startup options of every connection made through mz_postgres_util::Config
and verify them after connecting. Before-image text otherwise follows
upstream session defaults that can change mid-life, breaking retraction
symmetry for TEXT COLUMNS values.

Closes: SS-362
Closes: SS-376

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@ohbadiah
ohbadiah requested a review from a team as a code owner September 10, 2026 18:37
@def-

def- commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

QA LLM Review

1. HIGH -- Pinning the GUCs breaks retraction symmetry for rows already ingested by existing sources

src/postgres-util/src/tunnel.rs:127

Applying the pins to every connection immediately changes the text rendering used for before-images of sources that were snapshotted before the pins existed. For a source with TEXT COLUMNS covering a timestamptz (or any type whose rendering follows one of the four GUCs) on an upstream whose effective setting differs from the pinned value, the first UPDATE/DELETE of a pre-existing row after this ships retracts a row form Materialize never stored, producing exactly the negative-accumulation corruption this PR is meant to eliminate.

Details

The PR description states pre-pin rows still retract cleanly because the pins are instant-preserving. That holds for CastType::Natural columns, where the text is parsed into a datum, but not for TEXT COLUMNS, where the cast is StorageScalarExpr::Column(i) (src/sql/src/pure/postgres.rs:626) and the rendered string is stored verbatim (src/storage/src/source/postgres/replication.rs:1095decode_utf8_text). TEXT COLUMNS on a timestamptz is the motivating repro.

Concretely, upstream timezone = Europe/Berlin, table with TEXT COLUMNS (audit) where audit is a composite containing a timestamptz:

  • Snapshot before this change stores ("2026-07-22 17:15:00+02", ...).
  • After this change, an UPDATE of that row emits -1 ("2026-07-22 15:15:00+00", ...) and +1 for the new form.
  • The collection is left with the stored Berlin row at +1, the never-ingested UTC row at -1, and the new row at +1. Any full-row read then errors with Non-positive multiplicity in DistinctBy, and the phantom row is permanent.

The new test/pg-cdc/session-guc-change.td demonstrates the mechanism in the safe direction: its final assertion reads back "2026-07-22 15:15:00+00" while the upstream database-level timezone is Europe/Berlin, i.e. the stored text tracks the connection's GUCs, not the datum. CI cannot catch the migration case because the postgres mzcompose service runs with no TZ, so its server default is already UTC and the TimeZone pin is a no-op there; the other three pins equal PostgreSQL's own defaults on PG 12+, so only upstreams with explicitly non-default settings are exposed for those.

Suggested fix: make the rendering settings a per-source property rather than a global connection property, so a source only changes rendering at a re-snapshot boundary. Recording the pinned set on PostgresSourceConnection at plan time (absent for catalog entries created before this change, meaning "inherit upstream", present and pinned for new sources) keeps existing sources bit-identical and gives new sources the fix. If the pins must apply to all sources at once, the rollout needs to force a re-snapshot of sources with TEXT COLUMNS on rendering-dependent types, and a dyncfg to disable the pins so an environment that hits this can be recovered without a code change. That same switch also covers the second unconditional break in this diff: an existing source reached through a pooler that strips startup options goes from working to permanently stalled on verify_pinned_session_settings, with no way to opt out.

Note that single-version testdrive cannot reproduce this, since with the pins in place the snapshot is pinned too. A platform check that ingests under a non-UTC upstream default in the old version and updates after the upgrade is the shape that would cover it.

@ohbadiah

Copy link
Copy Markdown
Contributor Author

Closing. The QA finding is correct: TEXT COLUMNS store rendered text verbatim, so a connection-level pin changes the retraction form for every existing source with a non-default upstream, and the first UPDATE after upgrade leaves a permanent phantom row. Any global pin, including DateStyle alone, has this shape. If the pin is ever pursued it has to be a per-source property fixed at CREATE time. The four-leg repro in this PR stands on its own and can be lifted into that work.

@ohbadiah ohbadiah closed this Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants