Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds logging for deletions when the local tuple origin differs, integrates that delete-origin-differs case into conflict reporting and logging, expands tests to cover delete-origin-change scenarios, and increases the TAP test timeout from 15 to 20 minutes. Changes
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/spock_conflict.c (1)
519-531: Consider including local tuple details in the log message for consistency with UPDATE_ORIGIN_DIFFERS.The
errdetailforDELETE_ORIGIN_DIFFERSonly shows remote transaction info, but for origin-change debugging, the local tuple's origin and timestamp are valuable. TheUPDATE_ORIGIN_DIFFERScase (lines 482-490) includes"existing local tuple {%s} xid=%u,origin=%s,timestamp=%s". Sincelocaltup.data,local_tuple_xid,local_origin_str, andlocal_tup_ts_strare already populated, adding them would improve observability.💡 Suggested enhancement to include local tuple info
case SPOCK_CT_DELETE_ORIGIN_DIFFERS: ereport(spock_conflict_log_level, - (errcode(ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION), - errmsg("CONFLICT: remote %s on relation %s replica identity index %s (origin differs). Resolution: %s.", - SpockConflictTypeNames[conflict_type], - qualrelname, idxname, - conflict_resolution_to_string(resolution)), - errdetail("tuple for remote delete in xact origin=%u,timestamp=%s,commit_lsn=%X/%X", - replorigin_session_origin, - timestamptz_to_str(replorigin_session_origin_timestamp), - (uint32) (replorigin_session_origin_lsn << 32), - (uint32) replorigin_session_origin_lsn))); + (errcode(ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION), + errmsg("CONFLICT: remote %s on relation %s replica identity index %s (origin differs). Resolution: %s.", + SpockConflictTypeNames[conflict_type], + qualrelname, idxname, + conflict_resolution_to_string(resolution)), + errdetail("existing local tuple {%s} xid=%u,origin=%s,timestamp=%s; remote delete in xact origin=%u,timestamp=%s,commit_lsn=%X/%X", + localtup.data, local_tuple_xid, + local_origin_str, + local_tup_ts_str, + replorigin_session_origin, + timestamptz_to_str(replorigin_session_origin_timestamp), + (uint32) (replorigin_session_origin_lsn << 32), + (uint32) replorigin_session_origin_lsn))); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_conflict.c` around lines 519 - 531, The DELETE_ORIGIN_DIFFERS log currently only emits remote transaction info; update the errdetail in the SPOCK_CT_DELETE_ORIGIN_DIFFERS case to also include the local tuple details (use localtup.data, local_tuple_xid, local_origin_str, local_tup_ts_str) similar to the UPDATE_ORIGIN_DIFFERS handling so the message shows both existing local tuple {…} xid=%u,origin=%s,timestamp=%s and the remote delete origin fields (replorigin_session_origin, replorigin_session_origin_timestamp, replorigin_session_origin_lsn) for consistent observability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/spock_conflict.c`:
- Around line 519-531: The DELETE_ORIGIN_DIFFERS log currently only emits remote
transaction info; update the errdetail in the SPOCK_CT_DELETE_ORIGIN_DIFFERS
case to also include the local tuple details (use localtup.data,
local_tuple_xid, local_origin_str, local_tup_ts_str) similar to the
UPDATE_ORIGIN_DIFFERS handling so the message shows both existing local tuple
{…} xid=%u,origin=%s,timestamp=%s and the remote delete origin fields
(replorigin_session_origin, replorigin_session_origin_timestamp,
replorigin_session_origin_lsn) for consistent observability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b607150-038a-4fee-9ed3-c5ce975a0c7a
⛔ Files ignored due to path filters (1)
tests/regress/expected/tuple_origin.outis excluded by!**/*.out
📒 Files selected for processing (4)
src/spock_apply_heap.csrc/spock_conflict.ctests/regress/sql/tuple_origin.sqltests/tap/t/013_origin_change_restore.pl
mason-sharp
force-pushed
the
from-v5_STABLE-part2
branch
from
55434b3 to
506481f
Compare
rasifr
force-pushed
the
from-v5_STABLE-part2
branch
from
506481f to
b220182
Compare
Report delete_origin_differs when a replicated DELETE targets a row whose local origin differs from the replication session origin. The new to be logged type shares the same early-return / GUC filtering logic as update_origin_differs (since_sub_creation, none, remote_only_differs). - spock_apply_heap.c: detect origin mismatch in the DELETE-found path - spock_conflict.c: extend same-origin guard to DELETE_ORIGIN_DIFFERS, add switch-case log message, remove stale placeholder comment - tests/regress/sql/tuple_origin.sql: add delete_origin_differs regression test - tests/tap/t/013_origin_change_restore.pl: add DELETE tests for all three log_origin_change modes
mason-sharp
force-pushed
the
feature/SPOC-322/delete-origin-log
branch
from
7311e8d to
99a925b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/tap/t/013_origin_change_restore.pl (1)
219-223: Clarify that Test 5 is the “origin unavailable” case, not the local-origin case.The new comment describes the
pg_restorerow asInvalidRepOriginId, which reads the same as the later locally-written path. Rewording this will keep the test intent aligned with the runtime distinction between “local origin = 0” and “origin unavailable”.✏️ Suggested wording
-# Row 4 was pg_restored to node 2 (predates subscription, origin = InvalidRepOriginId). -# It has never been updated since, so its commit timestamp predates sub creation. +# Row 4 was pg_restored to node 2 before the subscription existed. +# This exercises the pre-existing-data / unavailable-origin path, not the +# locally-written origin=0 path covered later.Based on learnings: In the spock codebase, replication origin value
0means the row was changed locally, whileNULL/"unknown"is the distinct case where the local origin is genuinely unavailable (for example afterpg_restore).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tap/t/013_origin_change_restore.pl` around lines 219 - 223, Update the comment for Test 5 to clarify it exercises the “origin unavailable” case (not the local-origin case): replace the phrase "InvalidRepOriginId" with wording such as "origin unavailable (NULL/unknown, e.g. pg_restore)" so the test intent is clear that this row predates subscription and has no local origin (distinct from local-origin = 0); keep the surrounding context mentioning Test 5 and the ALTER SYSTEM setting spock.log_origin_change = 'since_sub_creation'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/tap/t/013_origin_change_restore.pl`:
- Around line 219-223: Update the comment for Test 5 to clarify it exercises the
“origin unavailable” case (not the local-origin case): replace the phrase
"InvalidRepOriginId" with wording such as "origin unavailable (NULL/unknown,
e.g. pg_restore)" so the test intent is clear that this row predates
subscription and has no local origin (distinct from local-origin = 0); keep the
surrounding context mentioning Test 5 and the ALTER SYSTEM setting
spock.log_origin_change = 'since_sub_creation'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b71abf4-a821-400c-a930-70fb1c35eb2b
⛔ Files ignored due to path filters (1)
tests/regress/expected/tuple_origin.outis excluded by!**/*.out
📒 Files selected for processing (4)
src/spock_apply_heap.csrc/spock_conflict.ctests/regress/sql/tuple_origin.sqltests/tap/t/013_origin_change_restore.pl
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spock_apply_heap.c
mason-sharp
force-pushed
the
feature/SPOC-322/delete-origin-log
branch
from
a7c9d3b to
28f8b29
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/spock_conflict.c`:
- Around line 521-536: The errdetail for case SPOCK_CT_DELETE_ORIGIN_DIFFERS
currently prints remotetup.data which can be NULL here; update the message to
use delete-shaped detail text by checking if remotetup is NULL and, if so,
render the delete key from oldkey (use oldkey.data) or else produce the "remote
delete in xact origin=...,timestamp=...,commit_lsn=..." form used by the other
DELETE branches; adjust the errdetail parameters (remotetup.data vs oldkey.data
and the origin/timestamp/LSN fields like replorigin_session_origin,
replorigin_session_origin_timestamp, replorigin_session_origin_lsn) so the
detail always prints a meaningful remote-delete description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c8adeff-098f-42d9-9163-7512ef3eeece
📒 Files selected for processing (3)
.github/workflows/spockbench.ymlsrc/spock_conflict.ctests/tap/t/013_origin_change_restore.pl
We may want to examine moving some to weekly tests
mason-sharp
force-pushed
the
feature/SPOC-322/delete-origin-log
branch
from
28f8b29 to
ea8b42a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/spock_conflict.c`:
- Around line 354-355: The change added SPOCK_CT_DELETE_ORIGIN_DIFFERS to the
"ignore" path but spock_conflict_log_table() still NULLs local_origin when
local_tuple_origin == InvalidRepOriginId, collapsing the distinction between
"local change" (origin 0) and "unknown origin" (NULL). Update
spock_conflict_log_table() so that when constructing the
spock.resolutions.local_origin value you set it to the replication-local
sentinel (0) when local_tuple_origin == InvalidRepOriginId (or use the constant
representing local origin), and only write NULL when the origin is genuinely
unavailable; keep references to local_tuple_origin, local_origin,
InvalidRepOriginId and SPOCK_CT_DELETE_ORIGIN_DIFFERS to locate the logic to
change.
- Around line 528-535: Several errdetail calls in spock_conflict.c build a
commit_lsn using (uint32)(replorigin_session_origin_lsn << 32) which zeros the
high word; replace those manual bit-shift extracts with the canonical
LSN_FORMAT_ARGS macro. Locate each errdetail invocation in the conflict-handling
function that currently ends with
",commit_lsn=%X/%X",(uint32)(replorigin_session_origin_lsn << 32),(uint32)
replorigin_session_origin_lsn and change the format/arguments to use
LSN_FORMAT_ARGS(replorigin_session_origin_lsn) (i.e. keep the "%X/%X" in the
format string but remove the two uint32 casts and supply
LSN_FORMAT_ARGS(replorigin_session_origin_lsn) instead); do this for all five
occurrences referencing replorigin_session_origin_lsn (the same pattern appears
near the existing calls that also use timestamptz_to_str and errdetail).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 602a280a-3948-4070-8608-59ca5f76fff4
📒 Files selected for processing (3)
.github/workflows/spockbench.ymlsrc/spock_conflict.ctests/tap/t/013_origin_change_restore.pl
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/spockbench.yml
Use "local" for the origin when the row is local Display 0 for local origin in log table
mason-sharp
force-pushed
the
feature/SPOC-322/delete-origin-log
branch
from
ea8b42a to
35b9f22
Compare
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
1 participant
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.
DRAFT: currently uses from-v5_STABLE-part2 as a base to sequence PR order, will change to main later.
Report delete_origin_differs when a replicated DELETE targets a row whose local origin differs from the replication session origin. The new to be logged type shares the same early-return / GUC filtering logic as update_origin_differs (since_sub_creation, none, remote_only_differs).