Conversation
Fixes 'publication "ace_mtree_pub" does not exist' (SQLSTATE 42704) during 'ace mtree table-diff' on Spock-enabled clusters. Two root causes addressed: 1. Init order: the replication slot was being created before the publication was committed to WAL. The slot's consistent point therefore preceded the publication, and pgoutput's get_publication_oid failed during change-callback replay. MtreeInit is now split into three phases per node: Phase A commits schema + metadata table + publication and captures the publication commit LSN; Phase B creates the slot (its consistent point is now strictly after the publication); Phase C persists slot/start_lsn/pub_commit_lsn. 2. Spock replication of ace_cdc_metadata: under DDL replication the metadata table was auto-added to Spock's default repset, so each node's node-local start_lsn leaked across the cluster and overwrote peer LSNs. ExcludeMetadataFromSpockRepsets enumerates spock.tables and removes the metadata table from every repset it landed in; all metadata writes (init, periodic flush, on-shutdown flush, final update) now wrap their write in spock.repair_mode to defend against the per-node-init race where a peer's repset still contains the table. spock.tables surfaces every Spock-known relation with NULL set_name for tables not in any repset, so the enumeration filters AND set_name IS NOT NULL. CAUTION applied at every SetSpockRepairMode site: repair_mode is session-scoped, not transaction-scoped — SQL ROLLBACK does NOT reset it. Callers must reach Set(false) on every code path or the pooled connection returns poisoned. A new pub_commit_lsn column is added to ace_cdc_metadata (additive, ALTER TABLE IF NOT EXISTS). processReplicationStream refuses to open a stream whose start_lsn is older than pub_commit_lsn, returning an actionable error instead of silently rewinding to a pre-publication LSN. Empty pub_commit_lsn (legacy metadata rows) skips the check with a warning. Existing broken installs are not auto-repaired; users must run 'ace mtree teardown' + 'ace mtree init' + 'ace mtree build' after upgrade. '--skip-cdc' remains a valid interim workaround for pre-upgrade clusters. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add tests/integration/cdc_init_ordering_test.go covering the three invariants the fix preserves: - slot's confirmed_flush_lsn >= ace_cdc_metadata.pub_commit_lsn on every node (Bug 1: slot consistent point must not precede publication commit). - ace_cdc_metadata is removed from every Spock repset on every node; the test seeds it into 'default' between two init calls so the assertion bites on pre-fix code (Bug 2). - processReplicationStream returns the actionable guard error, not raw SQLSTATE 42704, when start_lsn precedes pub_commit_lsn. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds publication commit LSN tracking to PostgreSQL CDC initialization, enforces ordering checks to prevent stale start LSN values, isolates metadata table updates node-locally via Spock repair mode, and validates invariants through integration tests. ChangesCDC Publication-Commit Ordering and Isolation
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
danolivo
changed the title
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 6 critical (6 false positives) |
🟢 Metrics 17 complexity · 7 duplication
Metric Results Complexity 17 Duplication 7
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codacy flagged the test body as 61 LOC (limit 50). Pull the two-node fan-out loops and the sentinel-propagation block into named helpers: seedMetadataInDefaultRepset, assertMetadataNotInAnyRepset, assertSentinelDoesNotPropagate, plus mtreeTestNodes() to centralise the (name, pool) pair list. The test body is now 21 LOC and reads as arrange → act → assert. The six "SQL injection" findings Codacy raised on the same file are AI-classified false positives: the %s substitution is config.Cfg.MTree.Schema (a config-loaded identifier sanitised at load time), and the line at seedMetadataInDefaultRepset's spock.repset_add_table call is in fact parameterised via $1 — Codacy misread the surrounding fmt.Sprintf. No code change needed for those. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
db/queries/templates.go (1)
376-387:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSplit this multi-statement template into separate
Execcalls.The template emits
CREATE TABLE ...; ALTER TABLE ...as a single SQL string. pgx v5 uses the extended protocol by default, which does not support multiple statements in a singleExec()call. This will fail at runtime. Either split into two separate templates with twoExeccalls, or explicitly configure the connection to useQueryExecModeSimpleProtocol(not found in the codebase).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/queries/templates.go` around lines 376 - 387, The template CreateCDCMetadataTable currently emits two SQL statements ("CREATE TABLE ...;" and "ALTER TABLE ...;") in one string which fails under pgx v5 extended protocol; split this into two separate templates and Exec calls: keep the CREATE TABLE statement in CreateCDCMetadataTable (or a renamed createAceCDCMetadataTable) and move the ALTER TABLE ... ADD COLUMN IF NOT EXISTS pub_commit_lsn into a new template (e.g., AddPubCommitLsnToAceCDCMetadata) and call Exec for each template separately, referencing {{aceSchema}} and ace_cdc_metadata in both so they run sequentially at migration time.
🧹 Nitpick comments (1)
internal/infra/cdc/setup.go (1)
90-99: 🏗️ Heavy liftConsider constraining the API signature to enforce session pinning.
All current call sites correctly use pinned transactions (
txfrompool.Begin()), but the function signature accepts a genericqueries.DBQuerier. While the wrapper atdb/queries/queries.go:3007documents thatSet(true)andSet(false)must pair on the same session, the generic API allows future callers to pass a pool handle and inadvertently separate the toggles across sessions, poisoning one connection.Consider adding compile-time enforcement (e.g., a type wrapper or interface constraint) to make session-pinning explicit and prevent misuse.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/infra/cdc/setup.go` around lines 90 - 99, SetSpockRepairMode currently accepts a generic queries.DBQuerier which lets callers pass a pool handle and accidentally toggle repair mode on different sessions; change the API to require a session-bound type (e.g., accept the transaction/session interface used by pool.Begin() such as a queries.DBTx or a new interface like queries.SessionQuerier) so SetSpockRepairMode(ctx context.Context, session queries.SessionQuerier, on bool) enforces compile-time session pinning; update all callers to pass the tx returned from Begin() and call queries.CheckSpockInstalled and queries.SetSpockRepairMode on that session so Set(true)/Set(false) always occur on the same connection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/consistency/mtree/merkle.go`:
- Around line 866-870: The call to queries.CurrentWalInsertLSN that sets
pubCommitLSN is happening while Phase A's transaction (tx) is still open; move
the WAL LSN capture so it occurs after Phase A commits and before Phase B starts
so the read reflects the commit LSN. Concretely: finish and commit the Phase A
transaction first, then call queries.CurrentWalInsertLSN (using an appropriate
context/connection outside the committed tx or a new read-only tx) to set
pubCommitLSN, and only then proceed to Phase B logic (the code that relies on
pubCommitLSN and uses tx/slot-guard checks).
In `@internal/infra/cdc/listen.go`:
- Around line 467-489: After calling SetSpockRepairMode(processingCtx, tx, true)
you must immediately schedule cleanup so session-scoped repair_mode is always
disabled; add defer SetSpockRepairMode(processingCtx, tx, false) right after the
successful true call (before calling queries.UpdateCDCMetadata or committing tx)
so any early return or error path will run the disable. Update or remove the
later explicit SetSpockRepairMode(..., false) calls as needed to avoid
double-disabling or handle their errors gracefully; ensure the deferred call
runs regardless of conn.Close or tx.Commit failures and continues to return the
connection to the pool in a neutral state.
---
Outside diff comments:
In `@db/queries/templates.go`:
- Around line 376-387: The template CreateCDCMetadataTable currently emits two
SQL statements ("CREATE TABLE ...;" and "ALTER TABLE ...;") in one string which
fails under pgx v5 extended protocol; split this into two separate templates and
Exec calls: keep the CREATE TABLE statement in CreateCDCMetadataTable (or a
renamed createAceCDCMetadataTable) and move the ALTER TABLE ... ADD COLUMN IF
NOT EXISTS pub_commit_lsn into a new template (e.g.,
AddPubCommitLsnToAceCDCMetadata) and call Exec for each template separately,
referencing {{aceSchema}} and ace_cdc_metadata in both so they run sequentially
at migration time.
---
Nitpick comments:
In `@internal/infra/cdc/setup.go`:
- Around line 90-99: SetSpockRepairMode currently accepts a generic
queries.DBQuerier which lets callers pass a pool handle and accidentally toggle
repair mode on different sessions; change the API to require a session-bound
type (e.g., accept the transaction/session interface used by pool.Begin() such
as a queries.DBTx or a new interface like queries.SessionQuerier) so
SetSpockRepairMode(ctx context.Context, session queries.SessionQuerier, on bool)
enforces compile-time session pinning; update all callers to pass the tx
returned from Begin() and call queries.CheckSpockInstalled and
queries.SetSpockRepairMode on that session so Set(true)/Set(false) always occur
on the same connection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5811b76d-c669-4f27-abcf-7fa388daaab6
📒 Files selected for processing (7)
db/queries/queries.godb/queries/templates.gointernal/consistency/mtree/merkle.gointernal/infra/cdc/listen.gointernal/infra/cdc/setup.gotests/integration/cdc_busy_table_test.gotests/integration/cdc_init_ordering_test.go
In processReplicationStream's non-continuous tail, the previous pattern called SetSpockRepairMode(tx, true), did the metadata write, then SetSpockRepairMode(tx, false) before commit. Any error between the two — UpdateCDCMetadata, the Set(false) itself, or Commit — returned early and left the underlying session GUC set. The pooled connection went back to the pool poisoned; the next borrower's writes silently did not replicate. Switch to the borrowed-conn pattern: pool.Acquire returns a *pgxpool.Conn, Set(true) runs against it, defer Set(false) on the same conn runs before Release. The tx is opened on the same conn so the SESSION GUC applies to UpdateCDCMetadata. Tx commit/rollback ends the tx but the conn is still acquired when the defer fires, so the reset reaches a live session. If the reset itself fails, log and continue — Release will then drop the conn rather than return it to the pool, also acceptable. A deferred reset against the tx (as one might first try) does not work: pgx.Tx is closed at commit time, so the SQL call inside the defer would return "tx is closed" and the session would stay set. Two more sites have the same shape and the same bug: - flushMetadata in this file (called from periodic and on-shutdown) - MtreeInit Phase C in internal/consistency/mtree/merkle.go This commit fixes only the site Codacy flagged; the other two should be migrated to the same pattern in a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Diagnosis and ordering invariant look sound; tests cover the right invariants. Notes inline. PR-level nit: the body says "two commits (fix-first, then test)" but the branch has four; either update the description or rebase back to two for a bisect-clean history.
| // start_lsn is the local slot's progress; cross-node propagation | ||
| // triggers SQLSTATE 42704 on peers. See SetSpockRepairMode's | ||
| // CAUTION on session-scoped state — Set(false) below MUST run. | ||
| if err := SetSpockRepairMode(ctx, tx, true); err != nil { |
There was a problem hiding this comment.
Same pool-poisoning hole that Site 1 just fixed. If UpdateCDCMetadata errors, Set(false) never runs and the conn returns to the pool with repair_mode on. flushMetadata is called from the periodic and shutdown paths, so this is the hottest place to leave broken. Please switch to the borrowed-*pgxpool.Conn + defer Set(false) before Release pattern in this PR.
|
|
||
| if err = queries.CreateCDCMetadataTable(m.Ctx, tx); err != nil { | ||
| return fmt.Errorf("failed to create cdc metadata table: %w", err) | ||
| if err := queries.InitCDCMetadata(m.Ctx, tx, |
There was a problem hiding this comment.
Same hole as flushMetadata (listen.go:556). If InitCDCMetadata errors, Set(false) on line 899 is skipped and the pool conn returns poisoned. Please port to the borrowed-conn pattern in this PR rather than a follow-up.
| logger.Error("failed to parse pub_commit_lsn %s: %v", pubCommitLSNStr, err) | ||
| return fmt.Errorf("failed to parse pub_commit_lsn %s: %w", pubCommitLSNStr, err) | ||
| } | ||
| if startLSN < pubCommitLSN { |
There was a problem hiding this comment.
Guard catches a peer start_lsn older than pub_commit_lsn (the customer's case). A peer LSN newer than pub_commit_lsn passes through. Repset exclusion + spock.repair_mode are the real defense; this is a tripwire for partial-state-on-upgrade. A comment line saying so would stop future readers from treating it as a general leak detector.
| // | ||
| // No-op when Spock is not installed. | ||
| func SetSpockRepairMode(ctx context.Context, db queries.DBQuerier, on bool) error { | ||
| spockInstalled, err := queries.CheckSpockInstalled(ctx, db) |
There was a problem hiding this comment.
CheckSpockInstalled fires on every SetSpockRepairMode call (~12 RTTs/min in continuous CDC) for a value that's fixed for the Postgres lifetime. Cache it on the pool, or hoist the check into processReplicationStream and pass a bool down. Not blocking.
| ), sentinel, config.Cfg.MTree.CDC.PublicationName) | ||
| require.NoError(t, err, "write sentinel start_lsn on n1") | ||
|
|
||
| deadline := time.Now().Add(3 * time.Second) |
There was a problem hiding this comment.
3 s is tight for Spock apply lag on slow CI. Bump to 8-10 s, or pull from a test-tunable constant alongside the other CDC-timing knobs. Otherwise this will flake periodically.
| EnumerateMetadataRepsets: template.Must(template.New("enumerateMetadataRepsets").Funcs(aceTemplateFuncs).Parse(` | ||
| SELECT set_name | ||
| FROM spock.tables | ||
| WHERE nspname = $1 AND relname = 'ace_cdc_metadata' AND set_name IS NOT NULL |
There was a problem hiding this comment.
Schema is parameterized via $1 but the relname 'ace_cdc_metadata' is hardcoded. A shared const cdcMetadataTableName referenced from both the Go side and this template would prevent drift if the table is ever renamed. Minor, not blocking.
2 participants
Summary
Fixes
publication "ace_mtree_pub" does not exist(SQLSTATE 42704) duringace mtree table-diffon Spock-enabled clusters. Customer-reported (QubeRT via Ahsan). The interim workaround was--skip-cdc; this PR removes the need for it on new init runs.Root causes
Two interacting defects in
MtreeInit:pgoutput'sget_publication_oidfailed during change-callback replay.ace_cdc_metadata. Under DDL replication the metadata table was auto-added to Spock's default repset; each node's node-localstart_lsnleaked cross-node and overwrote peer LSNs.Either bug in isolation is harmless. Together they steer
processReplicationStreaminto a silent rewind to the slot's pre-publication LSN → 42704.What changed
MtreeInitis split into three phases per node. Phase A (tx1) creates schema/helpers/metadata table, removes it from every Spock repset, creates the publication, and captures the publication's commit LSN. Phase B creates the replication slot (consistent point now strictly after the publication). Phase C (tx2) persistsslot_name/start_lsn/pub_commit_lsntoace_cdc_metadata, wrapped inspock.repair_mode(true).pub_commit_lsncolumn onace_cdc_metadata(additive,ALTER TABLE IF NOT EXISTS).processReplicationStream: refuses to open a stream whosestart_lsnis older thanpub_commit_lsn, with an actionable error pointing atmtree teardown+mtree init. The previous silent rewind toslot.confirmed_flush_lsnis gone.ExcludeMetadataFromSpockRepsets,SetSpockRepairMode,CurrentWalInsertLSN.spock.tablesis filteredWHERE set_name IS NOT NULLbecause that view surfaces every Spock-known relation, NULL-set_namefor tables not in any repset — pgx would otherwise crash withcannot scan NULL into *string.tests/integration/cdc_init_ordering_test.gocovers the three invariants the fix preserves: slot ordering, repset exclusion, runtime guard.Reviewer notes
spock.repair_modeis session-scoped, not transaction-scoped. SQL ROLLBACK does NOT reset it. EverySet(true)must reachSet(false)on every code path, including error paths — otherwise the pooled connection is returned poisoned and the next borrower's writes silently don't replicate. Every site that toggles repair_mode now carries a CAUTION block making this explicit. A follow-up patch to usedefer Set(false)on a borrowed*pgxpool.Connwould harden this further but is out of scope here.ace mtree teardown+ace mtree init+ace mtree buildafter upgrade.--skip-cdcremains a valid workaround pre-upgrade.CheckSpockInstalledruns on everySetSpockRepairModecall; in continuous CDC mode that's ~12 round-trips/min for a value that's fixed for a Postgres lifetime. Easy follow-up to cache on the pool.git bisect-clean.🤖 Generated with Claude Code