Conversation
Extend PG test to connect to all three nodes. The new test sets up n3 → n1/n2 logical replication, verifies origin tracking through GetNodeOriginNames, then runs a full diff + preserve-origin repair cycle confirming both origins and timestamps are preserved.
📝 WalkthroughWalkthroughAdded a third PostgreSQL node ( Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 8 |
| Duplication | -4 |
TIP This summary will be updated as you push new changes. Give us feedback
![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)
tests/integration/native_pg_test.go (1)
609-617:⚠️ Potential issue | 🟡 MinorUse
SET LOCALto constrainsession_replication_roleto this transaction.
SET session_replication_role = 'replica'withoutLOCALis session-scoped; withpgxpool, the connection returns to the pool still in replica mode afterCommit, potentially affecting subsequent operations. UseSET LOCALand add a rollback defer for failure paths.Proposed fix
tx, err := state.n2Pool.Begin(ctx) require.NoError(t, err) +defer tx.Rollback(ctx) // ignored after successful commit -_, err = tx.Exec(ctx, "SET session_replication_role = 'replica'") +_, err = tx.Exec(ctx, "SET LOCAL session_replication_role = 'replica'") require.NoError(t, err) for _, id := range sampleIDs { _, err = tx.Exec(ctx, fmt.Sprintf("DELETE FROM %s WHERE id = $1", qualifiedTableName), id) require.NoError(t, err) } require.NoError(t, tx.Commit(ctx))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/native_pg_test.go` around lines 609 - 617, The transaction sets session_replication_role globally which can leak into pooled connections; change the statement executed on the tx returned by state.n2Pool.Begin(ctx) to use "SET LOCAL session_replication_role = 'replica'" so it only applies to that transaction, and add a defer to rollback the tx on early returns/errors (e.g. defer func(){ _ = tx.Rollback(ctx) }()) before looping over sampleIDs and before committing with tx.Commit(ctx) to ensure the tx is cleaned up on failures; update references in the failing block that use tx, qualifiedTableName, sampleIDs, and tx.Commit accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/CHANGELOG.md`:
- Around line 21-24: Update the CHANGELOG entry to accurately state that ACE
schema names are sanitized in Go before template rendering: change the wording
so it says the schema name is sanitized by the aceSchema() Go function (used by
CreateSchema) prior to being passed into the SQL template (the template still
renders {{.SchemaName}} without calling .Sanitize()); reference aceSchema() in
internal/consistency/mtree/merkle.go and the CreateSchema template in
db/queries/templates.go to ensure the note correctly attributes where
sanitization occurs.
In `@tests/integration/native_pg_test.go`:
- Around line 655-663: The test currently only checks repairedOrigin is
non-empty; change it to assert the preserved origin equals the expected
normalized origin. For each id in sampleIDs call getNativeReplicationOrigin(t,
ctx, state.n2Pool, qualifiedTableName, id), compute the expected origin (e.g.
subN1Name or the exact value from your diff metadata/ originalOrigins[id] after
any normalization you apply), and replace require.NotEmpty with require.Equal(t,
expectedOrigin, repairedOrigin, ...) so the test verifies the exact preserved
origin rather than any non-empty value. Ensure you reference repairedOrigin,
originalOrigins, subN1Name, getNativeReplicationOrigin, qualifiedTableName, and
state.n2Pool when locating code to change.
---
Outside diff comments:
In `@tests/integration/native_pg_test.go`:
- Around line 609-617: The transaction sets session_replication_role globally
which can leak into pooled connections; change the statement executed on the tx
returned by state.n2Pool.Begin(ctx) to use "SET LOCAL session_replication_role =
'replica'" so it only applies to that transaction, and add a defer to rollback
the tx on early returns/errors (e.g. defer func(){ _ = tx.Rollback(ctx) }())
before looping over sampleIDs and before committing with tx.Commit(ctx) to
ensure the tx is cleaned up on failures; update references in the failing block
that use tx, qualifiedTableName, sampleIDs, and tx.Commit accordingly.
🪄 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: 4ade17d0-bcbf-49ce-a4f7-5fd3e4368771
📒 Files selected for processing (3)
docs/CHANGELOG.mdtests/integration/docker-compose-native.yamltests/integration/native_pg_test.go
| ### Changed | ||
| - ACE schema name in SQL templates is now quoted with `pgx.Identifier.Sanitize()` | ||
| to prevent SQL breakage with non-simple schema names (e.g., mixed case, | ||
| hyphens). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify whether ACE schema creation/template paths sanitize schema identifiers.
rg -n -C3 'CreateSchema\(|aceSchema\(\)|pgx\.Identifier|\.Sanitize\(\)' --type goRepository: pgEdge/ace
Length of output: 50366
🏁 Script executed:
fd -e sql -o -e tmpl -o -e tpl | head -20Repository: pgEdge/ace
Length of output: 170
🏁 Script executed:
find . -name "*.sql" -o -name "*.tmpl" -o -name "*template*" | grep -v node_modules | head -30Repository: pgEdge/ace
Length of output: 102
🏁 Script executed:
rg -l "CREATE SCHEMA|CreateSchema" --type sql --type tmpl --max-count 20Repository: pgEdge/ace
Length of output: 82
🏁 Script executed:
cat -n db/queries/templates.go | head -100Repository: pgEdge/ace
Length of output: 5019
🏁 Script executed:
rg -n "CreateSchema|GetCreateSchema" db/queries/Repository: pgEdge/ace
Length of output: 512
🏁 Script executed:
sed -n '1510,1520p' db/queries/templates.goRepository: pgEdge/ace
Length of output: 585
Clarify the scope of identifier sanitization in the release notes.
The CHANGELOG claim is inaccurate. The ACE schema name is sanitized via the aceSchema() Go function before being passed to CreateSchema, not within the SQL template itself. The CreateSchema SQL template (line 1510 in db/queries/templates.go) renders {{.SchemaName}} without calling .Sanitize(). While the final schema name is quoted (because m.aceSchema() at line 819 in internal/consistency/mtree/merkle.go returns a pre-sanitized string), the sanitization happens in Go code, not "in SQL templates" as the release note suggests. Revise line 22 to clarify that the schema name is now sanitized before SQL template rendering, not within the templates themselves.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/CHANGELOG.md` around lines 21 - 24, Update the CHANGELOG entry to
accurately state that ACE schema names are sanitized in Go before template
rendering: change the wording so it says the schema name is sanitized by the
aceSchema() Go function (used by CreateSchema) prior to being passed into the
SQL template (the template still renders {{.SchemaName}} without calling
.Sanitize()); reference aceSchema() in internal/consistency/mtree/merkle.go and
the CreateSchema template in db/queries/templates.go to ensure the note
correctly attributes where sanitization occurs.
| // --- Verify origins are preserved --- | ||
| // After preserve-origin repair, repaired rows on n2 should have an | ||
| // ACE-created origin that encodes the original source (n3 via n1). | ||
| for _, id := range sampleIDs { | ||
| repairedOrigin := getNativeReplicationOrigin(t, ctx, state.n2Pool, qualifiedTableName, id) | ||
| require.NotEmpty(t, repairedOrigin, | ||
| "Row %d should have a replication origin after preserve-origin repair", id) | ||
| log.Printf("Row %d: n1 origin=%s, repaired n2 origin=%s", id, originalOrigins[id], repairedOrigin) | ||
| } |
There was a problem hiding this comment.
Assert the preserved origin, not just that some origin exists.
This passes if repair writes any non-empty replication origin, so it does not verify the PR’s preserve-origin invariant. Compare repairedOrigin to the expected normalized origin, such as subN1Name or the exact value emitted in the diff metadata, instead of only logging originalOrigins[id].
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/native_pg_test.go` around lines 655 - 663, The test
currently only checks repairedOrigin is non-empty; change it to assert the
preserved origin equals the expected normalized origin. For each id in sampleIDs
call getNativeReplicationOrigin(t, ctx, state.n2Pool, qualifiedTableName, id),
compute the expected origin (e.g. subN1Name or the exact value from your diff
metadata/ originalOrigins[id] after any normalization you apply), and replace
require.NotEmpty with require.Equal(t, expectedOrigin, repairedOrigin, ...) so
the test verifies the exact preserved origin rather than any non-empty value.
Ensure you reference repairedOrigin, originalOrigins, subN1Name,
getNativeReplicationOrigin, qualifiedTableName, and state.n2Pool when locating
code to change.
1 participant
Also update the native PG test to verify origin tracking