Origin tracking test for native Postgres#114
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.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 9 seconds. ⌛ 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)
📝 WalkthroughWalkthroughAdded a third native PostgreSQL node to integration tests, introduced a data-sync helper, and refactored test setup and preserve-origin verification to operate across a 3-node logical replication topology. 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 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 | 14 |
| 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: 1
🧹 Nitpick comments (3)
tests/integration/native_pg_test.go (1)
609-617: Minor:SET session_replication_roleleaks to the pool after commit.
SET(withoutLOCAL) inside a transaction persists at session scope even afterCOMMIT, so the connection returned tostate.n2Poolhere stays inreplicarole. In this test everything that follows is read-only, and this is the lastt.RuninTestNativePG, so there's no concrete impact today — but it's a subtle footgun if the test grows or the pool is reused elsewhere. PreferSET LOCAL, or explicitlyRESET session_replication_rolebefore commit.♻️ Proposed fix
tx, err := state.n2Pool.Begin(ctx) require.NoError(t, err) - _, 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 at session scope which leaks into the pooled connection; change the test code around state.n2Pool.Begin / tx to avoid persisting the role by using a transaction-local setting or resetting it before returning the connection: replace the "SET session_replication_role = 'replica'" call with "SET LOCAL session_replication_role = 'replica'" (or alternatively execute "RESET session_replication_role" on tx before tx.Commit) so the pool connection returned by tx.Commit does not remain in replica mode.tests/integration/docker-compose-native.yaml (1)
44-57: Optional: deduplicate the three near-identical service blocks via YAML anchors.All three services share image, env, command, and ports — a YAML anchor (
&native-pg+<<: *native-pg) would remove the copy-paste and ensure n3 stays in sync with any future change to n1/n2. Purely a readability/maintenance win; not required.♻️ Sketch
services: native-n1: &native-pg image: postgres:17 environment: POSTGRES_USER: postgres POSTGRES_PASSWORD: password POSTGRES_DB: testdb command: - "postgres" - "-c" - "track_commit_timestamp=on" - "-c" - "wal_level=logical" ports: - "5432" native-n2: <<: *native-pg native-n3: <<: *native-pg🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/docker-compose-native.yaml` around lines 44 - 57, Create a YAML anchor for the shared Postgres service config and apply it to native-n1, native-n2, and native-n3 to avoid duplication: extract the common keys (image, environment, command, ports) into an anchor (e.g., &native-pg) and replace each service body with an alias merge (<<: *native-pg) so native-n1, native-n2, and native-n3 all inherit the same configuration.tests/integration/test_env_test.go (1)
398-414: Optional: give each count query a bounded context.
assertEventuallyonly checks its 30 s deadline between polls, so if acount(*)call ever stalls (e.g., a stuck connection during cleanup of a prior subtest) this gate can block well past 30 s. Since the helper exists specifically to de-flake tests, a per-attempt timeout would make the worst case deterministic. Not required — current implementation is fine for the healthy path.♻️ Proposed tweak
func (e *testEnv) awaitDataSync(t *testing.T, qualifiedTableName string) { t.Helper() - ctx := context.Background() assertEventually(t, 30*time.Second, func() error { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() var n1Count, n2Count int if err := e.N1Pool.QueryRow(ctx, fmt.Sprintf("SELECT count(*) FROM %s", qualifiedTableName)).Scan(&n1Count); err != nil { return fmt.Errorf("counting rows on n1: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_env_test.go` around lines 398 - 414, awaitDataSync's per-poll DB queries (N1Pool.QueryRow and N2Pool.QueryRow) can block longer than the assertEventually poll period; wrap each QueryRow call in a short per-attempt context (e.g., ctxAttempt, cancel := context.WithTimeout(ctx, 2*time.Second) with defer cancel()) and use that ctxAttempt for QueryRow/Scan so each attempt times out deterministically; ensure you propagate or wrap context deadline errors in the returned errors so assertEventually sees the failure and continues polling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/merkle_tree_test.go`:
- Line 571: The comment above the test misstates behavior: although it says
"this test only uses n1", the test later calls newTableDiffTask(ServiceN1,
ServiceN2) after CDC events, so it can be affected by prior subtest writes to
n2; either update the comment to explicitly explain why you intentionally
omitted awaitDataSync (e.g., to test CDC latency or to allow eventual
consistency) or add the same awaitDataSync(...) gate used in other subtests
before building the merkle tree on ServiceN1 to ensure no bleed from previous
cleanup/replication; locate the test around newTableDiffTask and the surrounding
comment and apply the chosen change (update comment text or insert awaitDataSync
referencing ServiceN1 and ServiceN2).
---
Nitpick comments:
In `@tests/integration/docker-compose-native.yaml`:
- Around line 44-57: Create a YAML anchor for the shared Postgres service config
and apply it to native-n1, native-n2, and native-n3 to avoid duplication:
extract the common keys (image, environment, command, ports) into an anchor
(e.g., &native-pg) and replace each service body with an alias merge (<<:
*native-pg) so native-n1, native-n2, and native-n3 all inherit the same
configuration.
In `@tests/integration/native_pg_test.go`:
- Around line 609-617: The transaction sets session_replication_role at session
scope which leaks into the pooled connection; change the test code around
state.n2Pool.Begin / tx to avoid persisting the role by using a
transaction-local setting or resetting it before returning the connection:
replace the "SET session_replication_role = 'replica'" call with "SET LOCAL
session_replication_role = 'replica'" (or alternatively execute "RESET
session_replication_role" on tx before tx.Commit) so the pool connection
returned by tx.Commit does not remain in replica mode.
In `@tests/integration/test_env_test.go`:
- Around line 398-414: awaitDataSync's per-poll DB queries (N1Pool.QueryRow and
N2Pool.QueryRow) can block longer than the assertEventually poll period; wrap
each QueryRow call in a short per-attempt context (e.g., ctxAttempt, cancel :=
context.WithTimeout(ctx, 2*time.Second) with defer cancel()) and use that
ctxAttempt for QueryRow/Scan so each attempt times out deterministically; ensure
you propagate or wrap context deadline errors in the returned errors so
assertEventually sees the failure and continues polling.
🪄 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: d8c39172-b83e-4311-b0c1-2c4f46a65938
📒 Files selected for processing (4)
tests/integration/docker-compose-native.yamltests/integration/merkle_tree_test.gotests/integration/native_pg_test.gotests/integration/test_env_test.go
mason-sharp
force-pushed
the
task/native-pg-origin-test
branch
from
23c4880 to
1f13996
Compare
Sequential merkle tree subtests share a table whose cleanup (repair via spock) may still be replicating when the next subtest starts, causing spurious failures. awaitDataSync polls until n1 and n2 row counts match before each data-mutating subtest. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> (cherry picked from commit 2760bb0e7bac8c8e0c74f474f0986b3720514759)
mason-sharp
force-pushed
the
task/native-pg-origin-test
branch
from
1f13996 to
8ba45ca
Compare
2 participants
Also make tests less flakey