Conversation
📝 WalkthroughWalkthroughA new integration TAP test and schedule entry are added. The test creates a 2-node cluster, forces a spock.node_id collision on n2, asserts subscription creation fails in both directions with uniqueness/duplicate diagnostics, verifies rollback and no sync on n2, restores IDs, and tears down the cluster. ChangesNode ID Collision Integration Test
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
|
danolivo
added
skip-test-nightly
![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
🤖 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 `@tests/tap/t/024_node_id_collision.pl`:
- Around line 115-118: The test's regex in the like assertion is too permissive
and may match unrelated "already exists" errors; update the regex used in the
like($sub_output, ...) assertions (the one that currently checks for /duplicate
key|unique constraint|already exists|node.*exists/i) to explicitly require the
primary-key collision signature by including node_pkey (for example add
|node_pkey to the alternation or tighten the node.*exists branch to mention
node_pkey), and make the same change in the second occurrence around lines
131-134 so the test only passes for the expected PK-collision message.
🪄 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: 2e704a00-0c9e-4a27-8ae9-62bad78bbd34
📒 Files selected for processing (2)
tests/tap/scheduletests/tap/t/024_node_id_collision.pl
Comment on lines
+115
to
+118
| like($sub_output, | ||
| qr/duplicate key|unique constraint|already exists|node.*exists/i, | ||
| "failure mode is a uniqueness / duplicate diagnostic " . | ||
| "(sees: " . substr($sub_output, 0, 120) . "...)"); |
There was a problem hiding this comment.
Tighten error matching to node_pkey to prevent false positives.
These assertions currently accept generic messages like already exists, so unrelated failures can still pass. Match the expected PK-collision signature (or include node_pkey explicitly) to keep this test diagnostic-focused.
Suggested diff
like($sub_output,
- qr/duplicate key|unique constraint|already exists|node.*exists/i,
+ qr/duplicate key value violates unique constraint\s+"(?:spock\.)?node_pkey"/i,
"failure mode is a uniqueness / duplicate diagnostic " .
"(sees: " . substr($sub_output, 0, 120) . "...)");
like($sub_output2,
- qr/duplicate key|unique constraint|already exists|node.*exists/i,
+ qr/duplicate key value violates unique constraint\s+"(?:spock\.)?node_pkey"/i,
"reverse failure mode is also a uniqueness diagnostic " .
"(sees: " . substr($sub_output2, 0, 120) . "...)");Also applies to: 131-134
🤖 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/tap/t/024_node_id_collision.pl` around lines 115 - 118, The test's
regex in the like assertion is too permissive and may match unrelated "already
exists" errors; update the regex used in the like($sub_output, ...) assertions
(the one that currently checks for /duplicate key|unique constraint|already
exists|node.*exists/i) to explicitly require the primary-key collision signature
by including node_pkey (for example add |node_pkey to the alternation or tighten
the node.*exists branch to mention node_pkey), and make the same change in the
second occurrence around lines 131-134 so the test only passes for the expected
PK-collision message.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/tap/t/024_node_id_collision.pl (2)
115-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten error matching to
node_pkeyto prevent false positives.The regex pattern is too permissive and may match unrelated failures. The expected error is specifically "duplicate key value violates unique constraint "node_pkey"". Tighten the pattern to:
like($sub_output, - qr/duplicate key|unique constraint|already exists|node.*exists/i, + qr/duplicate key value violates unique constraint\s+"(?:spock\.)?node_pkey"/i, "failure mode is a uniqueness / duplicate diagnostic " . "(sees: " . substr($sub_output, 0, 120) . "...)");🤖 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/tap/t/024_node_id_collision.pl` around lines 115 - 118, The current test's regex in the like assertion is too broad and may match unrelated errors; update the pattern used in the like($sub_output, qr/... , ...) call to specifically look for the node_pkey unique constraint (e.g., require node_pkey or the phrase "duplicate key value violates unique constraint \"node_pkey\"") so the test only passes for the intended uniqueness collision; modify the regex in the like assertion referencing $sub_output to include node_pkey (or the full expected phrase) instead of the generic duplicate/unique/exists alternatives.
131-134:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten error matching to
node_pkeyto prevent false positives.Same issue as the previous direction. Update the regex to specifically match the node_pkey constraint:
like($sub_output2, - qr/duplicate key|unique constraint|already exists|node.*exists/i, + qr/duplicate key value violates unique constraint\s+"(?:spock\.)?node_pkey"/i, "reverse failure mode is also a uniqueness diagnostic " . "(sees: " . substr($sub_output2, 0, 120) . "...)");🤖 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/tap/t/024_node_id_collision.pl` around lines 131 - 134, The test's error regex is too broad and may match unrelated uniqueness messages; update the assertion that checks $sub_output2 to specifically look for the node_pkey constraint (e.g., replace the current qr/duplicate key|unique constraint|already exists|node.*exists/i with a pattern that matches "node_pkey" or "constraint .*node_pkey" case-insensitively) so the test only accepts the expected node primary-key collision diagnostic.
🧹 Nitpick comments (1)
tests/tap/t/024_node_id_collision.pl (1)
78-81: ⚡ Quick winConsider using
psql_or_bailfor DDL/DML instead ofscalar_query.
scalar_queryis typically used for SELECT statements that return a single value. For DDL and DML operations like CREATE TABLE and INSERT, other Spock TAP tests usepsql_or_bail(). While this might work, consider aligning with the established pattern:psql_or_bail(1, "CREATE TABLE test (id serial PRIMARY KEY, x integer)"); psql_or_bail(1, "INSERT INTO test (x) VALUES (42)");Additionally, the INSERT syntax
(VALUES (42))has extra parentheses that, while valid, are non-idiomatic. Standard syntax isVALUES (42).🤖 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/tap/t/024_node_id_collision.pl` around lines 78 - 81, Replace the DDL/DML usage of scalar_query with psql_or_bail: instead of calling scalar_query(1, "...CREATE TABLE...; INSERT...;"), call psql_or_bail(1, "CREATE TABLE test (id serial PRIMARY KEY, x integer)") and psql_or_bail(1, "INSERT INTO test (x) VALUES (42)"); also remove the extra parentheses around VALUES so the INSERT uses the standard VALUES (42) form; update occurrences of scalar_query in this test to these two psql_or_bail calls.
🤖 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.
Duplicate comments:
In `@tests/tap/t/024_node_id_collision.pl`:
- Around line 115-118: The current test's regex in the like assertion is too
broad and may match unrelated errors; update the pattern used in the
like($sub_output, qr/... , ...) call to specifically look for the node_pkey
unique constraint (e.g., require node_pkey or the phrase "duplicate key value
violates unique constraint \"node_pkey\"") so the test only passes for the
intended uniqueness collision; modify the regex in the like assertion
referencing $sub_output to include node_pkey (or the full expected phrase)
instead of the generic duplicate/unique/exists alternatives.
- Around line 131-134: The test's error regex is too broad and may match
unrelated uniqueness messages; update the assertion that checks $sub_output2 to
specifically look for the node_pkey constraint (e.g., replace the current
qr/duplicate key|unique constraint|already exists|node.*exists/i with a pattern
that matches "node_pkey" or "constraint .*node_pkey" case-insensitively) so the
test only accepts the expected node primary-key collision diagnostic.
---
Nitpick comments:
In `@tests/tap/t/024_node_id_collision.pl`:
- Around line 78-81: Replace the DDL/DML usage of scalar_query with
psql_or_bail: instead of calling scalar_query(1, "...CREATE TABLE...;
INSERT...;"), call psql_or_bail(1, "CREATE TABLE test (id serial PRIMARY KEY, x
integer)") and psql_or_bail(1, "INSERT INTO test (x) VALUES (42)"); also remove
the extra parentheses around VALUES so the INSERT uses the standard VALUES (42)
form; update occurrences of scalar_query in this test to these two psql_or_bail
calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b559a07-3b75-4f06-b060-f688c5a60571
📒 Files selected for processing (2)
tests/tap/scheduletests/tap/t/024_node_id_collision.pl
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/tap/schedule
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
Add a TAP test that exercises Spock's behaviour when two independently-created nodes end up sharing a
node_id. The id is currently generated locally ashash_any(name) & 0xffffatnode_createtime, with no cluster-wide coordination — so this collision is reachable in practice (cluster splits, geo-migrations, DR rehydration). The test documents what happens today, before any negotiation protocol lands.What's covered
The test creates a 2-node cluster (
node_createonly, no cross-wire), then tampers with n2's catalog to give it n1's id. Tampering navigates two FK constraints:node_interface.if_nodeidandreplication_set.set_nodeidfollow viaON UPDATE CASCADE;local_node.node_idhas no cascade and is updated manually underSET session_replication_role = replicato bypass the FK trigger. The PRIMARY KEY onspock.node(node_id)is enforced by the unique index, so this works only because we tamper pre-attach when each catalog still holds a single row.With the colliding state in place,
sub_createis attempted from both directions. The test asserts:duplicate key value violates unique constraint "node_pkey"(the existing PK catches the collision whencreate_node(origin)atsrc/spock_functions.c:503tries to insert the remote-rep row);spock.nodecatalogs still hold exactly one row (the failed transactions rolled back atomically);spock.subscriptionis empty on both sides (no orphan subscription rows pointing at a colliding id);sync_structure=true,sync_data=true— the PK trip has to happen before the copy phase or n2 would be left with an orphaned populated table).Out of scope
The cluster-merge case where a colliding id sits on a third-party peer (one the joining node hasn't subscribed to directly) produces silent misattribution rather than a PK trip. That requires a 3-node forwarding scenario and is left for the upcoming negotiation-protocol work.