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>
📝 WalkthroughWalkthroughAdds pub_commit_lsn to CDC metadata, enforces a publication-commit ordering guard during replication stream startup, excludes and isolates metadata writes from Spock repsets via session-scoped repair mode on dedicated connections, and adds integration tests validating ordering and Spock exclusions. 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 docstrings
🧪 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
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 18 |
| Duplication | 5 |
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
ibrarahmad
reviewed
May 16, 2026
Contributor
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.
danolivo
pushed a commit
that referenced
this pull request
May 19, 2026
…ming) Round of fixes for the human review comments on PR #119: - flushMetadata (listen.go:542): same pool-poisoning hole as Site 1. Port to the borrowed-*pgxpool.Conn pattern so Set(false) is deferred on the live session before Release. This site fires every 10 s on the periodic-flush path AND on shutdown, so it was the highest-risk remaining leak. - MtreeInit Phase C (merkle.go:884): same hole. Borrow the conn, defer Set(false) on it, run the InitCDCMetadata write through a tx opened on the same conn. - Guard scope comment (listen.go:103): clarify that the pub_commit_lsn guard is a tripwire for partial-state-on-upgrade and for cross-node leaks whose LSN lands below local pub_commit_lsn — NOT a general leak detector. The actual defenses against cross-node leak are repset exclusion + repair_mode wrapping every metadata write. Reader-of-the-future shouldn't mistake this for an end-to-end check. - Test timing (cdc_init_ordering_test.go:186): bump the sentinel-propagation window from 3 s to 10 s. 3 s was tight under load on slow CI; 10 s gives Spock apply latency real headroom without making the test materially slower in the common case. Two non-blocking comments deliberately deferred to follow-ups: - CheckSpockInstalled cache on the pool (~12 RTTs/min savings) - Hoist 'ace_cdc_metadata' relname into a shared constant CodeRabbit's pubCommitLSN-timing finding remains skipped: the invariant chain captured_mid_tx < commit_lsn(Phase A) <= slot.consistent_point holds for freshly-created slots, which is the only state Phase B produces. The hypothetical "broken slot whose consistent_point sits between captured and commit" cannot arise from this code path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/infra/cdc/listen.go`:
- Around line 568-575: The deferred reset of spock.repair_mode in flushMetadata
currently uses the caller's cancelable ctx which can be canceled before defer
runs; change the defer to use a non-cancelable context (like the pattern in
processReplicationStream) so the reset always runs: keep the initial call
SetSpockRepairMode(ctx, c, true) as-is, then create a non-cancelable context
(e.g. ctxNoCancel := context.WithoutCancel(ctx)) and call
SetSpockRepairMode(ctxNoCancel, c, false) inside the defer; reference
SetSpockRepairMode and flushMetadata to locate and update the defer.
🪄 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: 582e9095-53ab-40a8-a71b-ea48d3c87de7
📒 Files selected for processing (3)
internal/consistency/mtree/merkle.gointernal/infra/cdc/listen.gotests/integration/cdc_init_ordering_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/cdc_init_ordering_test.go
- internal/consistency/mtree/merkle.go
processReplicationStream's non-continuous tail leaked repair_mode when any step between Set(true) and Commit returned early — the session GUC persists across SQL ROLLBACK, so the pooled connection went back poisoned and the next borrower's writes silently did not replicate. Switch to pool.Acquire: defer Set(false) on the borrowed *pgxpool.Conn runs before Release, on every path. flushMetadata and MtreeInit Phase C share the shape; tracked separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
danolivo
force-pushed
the
ace-186
branch
2 times, most recently
from
6bb524b to
dbd3724
Compare
- flushMetadata (listen.go) and MtreeInit Phase C (merkle.go): port to the borrowed-*pgxpool.Conn pattern from Site 1, so defer Set(false) runs on the live session before Release. flushMetadata is on the periodic+shutdown path and was the highest-leak remaining site. - Guard scope (listen.go): clarify the pub_commit_lsn check is a tripwire for partial-state-on-upgrade and narrow cross-node leaks, not a general leak detector — the real defense is repset exclusion plus repair_mode wrapping every metadata write. - Test timing (cdc_init_ordering_test.go): bump assertSentinelDoesNotPropagate's window from 3 s to 10 s for slow CI.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/cdc_init_ordering_test.go (1)
184-186:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale comment: mentions "3 s window" but code uses 10 seconds.
The comment references the original 3-second timeout, but the deadline was bumped to 10 seconds per prior review feedback.
📝 Suggested fix
// assertSentinelDoesNotPropagate writes a sentinel start_lsn on n1 and -// verifies it never appears on n2 within a 3 s window. Under bug 2 the +// verifies it never appears on n2 within a 10 s window. Under bug 2 the // sentinel would propagate via Spock apply within seconds.🤖 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 `@tests/integration/cdc_init_ordering_test.go` around lines 184 - 186, Update the stale comment above assertSentinelDoesNotPropagate to reflect the current 10 second timeout (instead of "3 s window"); locate the comment that mentions "3 s window" in the assertSentinelDoesNotPropagate test and change it to state "10 s window" or "10-second window" so it matches the test's actual deadline.
🤖 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.
Outside diff comments:
In `@tests/integration/cdc_init_ordering_test.go`:
- Around line 184-186: Update the stale comment above
assertSentinelDoesNotPropagate to reflect the current 10 second timeout (instead
of "3 s window"); locate the comment that mentions "3 s window" in the
assertSentinelDoesNotPropagate test and change it to state "10 s window" or
"10-second window" so it matches the test's actual deadline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2d4b8b7-a44a-434f-9397-23eb7b69ae3e
📒 Files selected for processing (6)
db/queries/queries.godb/queries/templates.gointernal/consistency/mtree/merkle.gointernal/infra/cdc/listen.gointernal/infra/cdc/setup.gotests/integration/cdc_init_ordering_test.go
ibrarahmad
approved these changes
May 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes
publication "ace_mtree_pub" does not exist(SQLSTATE 42704) duringace mtree table-diffon Spock-enabled clusters. Customer-reported. 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.