Merged
Conversation
Initialize debug_query_string to NULL at apply worker startup, since it is not a regular backend and has no client query string. Also clear debug_query_string in execute_sql_command_error_cb after the errcontext call, which already includes the SQL statement, to prevent it from appearing a second time in the LOG output. (cherry picked from commit dab4464)
When spock_read_insert returns NULL (relation not found), the exception log entry's local_tuple may still hold a pointer from a previous operation. Clear it before calling log_insert_exception to prevent get_tuple_origin from dereferencing freed memory. (cherry picked from commit 5fc70f5)
…ple fix Phase 1 verifies SUB_DISABLE and skip_lsn recovery when a table is dropped on the subscriber side. Phase 2 is a regression test for the dangling local_tuple bug: a conflict transaction sets exception_log->local_tuple in MessageContext; after commit MessageContext is reset and the pointer becomes dangling. A subsequent missing-relation exception in handle_insert() would dereference the freed pointer (SIGSEGV) without the fix. The test verifies graceful SUB_DISABLE with no signal 11 in the logs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> (cherry picked from commit 351c34f)
Use "local" for the origin when the row is local Display 0 for local origin in log table (cherry picked from commit e51bf26)
|
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 (1)
📝 WalkthroughWalkthroughAdds Spock 5.0.6 release notes; fixes cleanup of dangling Changes
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/tap/t/016_sub_disable_missing_relation.pl (2)
46-46: Consider making the log path more robust.The path
"../logs/00${p2}.log"assumes the test runs fromtests/tap/t/. This works with the current test harness but could break if the test is run from a different directory.Consider using the
$config->{log_dir}fromget_test_config()for consistency:my $pg_log_n2 = "$config->{log_dir}/00${p2}.log";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tap/t/016_sub_disable_missing_relation.pl` at line 46, The hardcoded relative log path in variable $pg_log_n2 is fragile; instead use the test config's log directory from get_test_config() (the $config hash) to build the path so it doesn't depend on current working directory—update the assignment of $pg_log_n2 to reference $config->{log_dir} (use the existing $config from get_test_config()) and construct the filename with "00${p2}.log".
67-67: Hardcoded sleeps may cause test flakiness.Multiple
sleep(N)calls throughout the test (lines 67, 109, 113, 164, 224) could lead to flakiness under load or slow CI environments. Consider replacing with polling loops where possible, similar to the existingwait_for_sub_statuspattern.For example, line 67's
sleep(3)after insert could poll until the row count changes:# Instead of: sleep(3); for (1..30) { last if scalar_query(2, "SELECT count(*) FROM test_missing_rel") eq '1'; sleep(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tap/t/016_sub_disable_missing_relation.pl` at line 67, Replace the hardcoded sleep(3) after the insert with a polling loop that repeatedly calls scalar_query to check the row count of test_missing_rel (e.g., scalar_query(2, "SELECT count(*) FROM test_missing_rel") eq '1') with a short sleep between iterations and a reasonable timeout; mirror the existing wait_for_sub_status pattern to avoid flakiness under load. Locate the sleep(3) in the test script and implement the loop, failing the test if the timeout is reached.
🤖 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/016_sub_disable_missing_relation.pl`:
- Line 46: The hardcoded relative log path in variable $pg_log_n2 is fragile;
instead use the test config's log directory from get_test_config() (the $config
hash) to build the path so it doesn't depend on current working directory—update
the assignment of $pg_log_n2 to reference $config->{log_dir} (use the existing
$config from get_test_config()) and construct the filename with "00${p2}.log".
- Line 67: Replace the hardcoded sleep(3) after the insert with a polling loop
that repeatedly calls scalar_query to check the row count of test_missing_rel
(e.g., scalar_query(2, "SELECT count(*) FROM test_missing_rel") eq '1') with a
short sleep between iterations and a reasonable timeout; mirror the existing
wait_for_sub_status pattern to avoid flakiness under load. Locate the sleep(3)
in the test script and implement the loop, failing the test if the timeout is
reached.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 738df693-4bc2-4a06-964a-5205e4b58dbe
📒 Files selected for processing (7)
docs/spock_release_notes.mdsrc/spock_apply.csrc/spock_conflict.ctests/tap/scheduletests/tap/t/013_origin_change_restore.pltests/tap/t/016_sub_disable_missing_relation.pltests/tap/t/SpockTest.pm
In the near future, trim down the schedule and run others nightly instead.
rasifr
approved these changes
Mar 13, 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
3 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.
Bring in changes from v5_STABLE into main, including release notes