Add spock.log_verbosity and spock.apply_change_logging GUCs#477
Add spock.log_verbosity and spock.apply_change_logging GUCs#477ibrarahmad wants to merge 2 commits into
Conversation
log_verbosity (normal|verbose) promotes spock DEBUG1/DEBUG2 to LOG. apply_change_logging (none|key_only|verbose) emits per-change JSON from the apply worker: action/schema/table/pk/origin/commit_ts in key_only, plus old/new row data in verbose. DDL is logged in both non-none modes. Both GUCs are PGC_SUSET (SET / SIGHUP reload). TAP coverage in tests/tap/t/044_apply_change_logging.pl - 24/24.
📝 WalkthroughWalkthroughAdds two GUCs and debug-routing macros, implements JSON serialization and public logging APIs, emits structured change logs before apply/DDL execution, updates debug sites and sync exec_cmd code, and adds TAP tests validating logging behavior. ChangesSpock Apply Worker Change Logging
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/spock_change_log.csrc/spock_change_log.c:16:10: fatal error: 'postgres.h' file not found ... [truncated 698 characters] ... /infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" src/spock_apply.csrc/spock_apply.c:12:10: fatal error: 'postgres.h' file not found ... [truncated 683 characters] ... "/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" src/spock_sync.csrc/spock_sync.c:13:10: fatal error: 'postgres.h' file not found ... [truncated 680 characters] ... "/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" 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
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 31 complexity · 0 duplication
Metric Results Complexity 31 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![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 (1)
tests/tap/t/044_apply_change_logging.pl (1)
20-30: ⚡ Quick winAdd a test case for skipped DML in
TRANSDISCARD/SUB_DISABLEmodes.Current coverage is strong for normal apply flow, but it doesn’t assert that skipped DML in exception replay modes does not emit apply-change records. Adding this would close a high-risk gap in logging correctness.
🤖 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/044_apply_change_logging.pl` around lines 20 - 30, Add a new test in the same TAP file that verifies skipped DML under the TRANSDISCARD and SUB_DISABLE replay modes does not emit apply-change JSON log records: set apply_change_logging to key_only (and repeat for verbose), configure publisher/subscriber, perform DML on the publisher that will be skipped on the subscriber by triggering TRANSDISCARD and SUB_DISABLE replay behavior, capture the subscriber server logs (same log-capture helpers used elsewhere in this test file), and assert that no apply-change JSON lines (those containing action/schema/table/pk and, for verbose, new row data) are present for the skipped DML; name the test case clearly (e.g., "skipped DML in TRANSDISCARD/SUB_DISABLE produces no apply_change_logging records") so it’s easy to find.
🤖 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 `@src/spock_apply.c`:
- Around line 1414-1416: The apply-change log is emitted even when DML was
intentionally skipped under exception-replay modes TRANSDISCARD/SUB_DISABLE;
update the three spots that call spock_log_apply_change (the INSERT block with
newtup, the UPDATE block with oldtup/newtup, and the DELETE block with oldtup)
to only call spock_log_apply_change when the operation both did not fail and was
actually applied — i.e., change the guard from if (!failed) to if (!failed &&
!<skip-dml-flag>), where <skip-dml-flag> checks the current exception replay
mode (TRANSDISCARD or SUB_DISABLE) or otherwise detects that DML was skipped;
use the existing symbols for those modes so the check reflects the code’s
exception replay state and ensures no “applied change” log is produced for
skipped operations.
---
Nitpick comments:
In `@tests/tap/t/044_apply_change_logging.pl`:
- Around line 20-30: Add a new test in the same TAP file that verifies skipped
DML under the TRANSDISCARD and SUB_DISABLE replay modes does not emit
apply-change JSON log records: set apply_change_logging to key_only (and repeat
for verbose), configure publisher/subscriber, perform DML on the publisher that
will be skipped on the subscriber by triggering TRANSDISCARD and SUB_DISABLE
replay behavior, capture the subscriber server logs (same log-capture helpers
used elsewhere in this test file), and assert that no apply-change JSON lines
(those containing action/schema/table/pk and, for verbose, new row data) are
present for the skipped DML; name the test case clearly (e.g., "skipped DML in
TRANSDISCARD/SUB_DISABLE produces no apply_change_logging records") so it’s easy
to find.
🪄 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: ebbbe842-5466-4b4d-86ca-e2caf912cda8
📒 Files selected for processing (10)
include/spock.hinclude/spock_change_log.hsrc/spock.csrc/spock_apply.csrc/spock_change_log.csrc/spock_common.csrc/spock_dependency.csrc/spock_sync.ctests/tap/scheduletests/tap/t/044_apply_change_logging.pl
mason-sharp
changed the title
Move spock_log_apply_change() for INSERT/UPDATE/DELETE ahead of the apply attempt and drop the !failed guard, so every IUD that reaches the apply path is logged regardless of success, failure, or skip under TRANSDISCARD/SUB_DISABLE; replace the local append_json_string helper with PostgreSQL's escape_json from utils/json.h. Resolve conflicts in src/spock_sync.c and tests/tap/schedule against main, and fix the test's log-path lookup to honour TESTLOGDIR.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/spock_sync.c (1)
16-16:⚠️ Potential issue | 🔴 CriticalFix WIN32 build break:
<sys/wait.h>and UNIX-onlyexec_cmd()appear unguarded insrc/spock_sync.c.
src/spock_sync.cincludes#include <sys/wait.h>in the main include block (no nearby#ifdef WIN32guards shown), but<sys/wait.h>isn’t available on Windows.- The only
exec_cmd()implementation visible here is UNIX-oriented (pid_t, fork/exec-style); no WIN32-specificexec_cmdalternative (exec_cmd_win32/CreateProcess) is present in the shown code.Run on Windows will likely fail at the header include and/or during compilation of the UNIX-only code.
🤖 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 `@src/spock_sync.c` at line 16, The file includes the UNIX-only header <sys/wait.h> and contains a UNIX-style exec_cmd implementation (uses pid_t/fork/exec) unguarded, breaking WIN32 builds; wrap the include and the UNIX exec_cmd implementation in proper platform guards (e.g., `#if` !defined(_WIN32) / `#endif`) so <sys/wait.h> and the fork/exec-based exec_cmd are only compiled on POSIX, and provide either a Win32 alternative declaration or call to a Windows-specific implementation (e.g., exec_cmd_win32/CreateProcess) for _WIN32 builds so compilation on Windows succeeds; update references to exec_cmd to use the platform-appropriate implementation or a common wrapper function name where needed.
🤖 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.
Outside diff comments:
In `@src/spock_sync.c`:
- Line 16: The file includes the UNIX-only header <sys/wait.h> and contains a
UNIX-style exec_cmd implementation (uses pid_t/fork/exec) unguarded, breaking
WIN32 builds; wrap the include and the UNIX exec_cmd implementation in proper
platform guards (e.g., `#if` !defined(_WIN32) / `#endif`) so <sys/wait.h> and the
fork/exec-based exec_cmd are only compiled on POSIX, and provide either a Win32
alternative declaration or call to a Windows-specific implementation (e.g.,
exec_cmd_win32/CreateProcess) for _WIN32 builds so compilation on Windows
succeeds; update references to exec_cmd to use the platform-appropriate
implementation or a common wrapper function name where needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09849b26-cd7c-4c9b-93d5-be6a02003286
📒 Files selected for processing (5)
src/spock_apply.csrc/spock_change_log.csrc/spock_sync.ctests/tap/scheduletests/tap/t/044_apply_change_logging.pl
✅ Files skipped from review due to trivial changes (1)
- tests/tap/schedule
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/spock_apply.c (1)
2393-2409:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing DDL apply-change logs in replay skip modes.
Line 2395 enters the TRANSDISCARD/SUB_DISABLE skip path and bypasses
handle_sql(), so the DDL log call at Line 2365 never runs. That dropsspock apply changeJSON for queued SQL/DDL in exactly the skipped/replay path this feature is meant to expose.Suggested fix
+/* Extract queued SQL scalar as raw string (shared by execute/skip paths). */ +static char * +extract_queued_sql(QueuedMessage *queued_message) +{ + /* move current JSON scalar validation/extraction logic from handle_sql() here */ +} + static void handle_sql(QueuedMessage *queued_message, bool tx_just_started, char **sql) { - /* Validate the json and extract the SQL string from it. */ - ... + *sql = extract_queued_sql(queued_message); spock_log_apply_ddl(*sql, remote_origin_name); spock_execute_sql_command(*sql, queued_message->role, tx_just_started); } ... if (exception_behaviour == TRANSDISCARD || exception_behaviour == SUB_DISABLE) { - sql = JsonbToCString(NULL, &queued_message->message->root, 0); + sql = extract_queued_sql(queued_message); + spock_log_apply_ddl(sql, remote_origin_name); exception_command_counter++; failed = false; }🤖 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 `@src/spock_apply.c` around lines 2393 - 2409, When exception_behaviour is TRANSDISCARD or SUB_DISABLE the current branch extracts sql with JsonbToCString and skips calling handle_sql(), which prevents emitting the DDL "spock apply change" log; modify the TRANSDISCARD/SUB_DISABLE branch in spock_apply.c so after sql = JsonbToCString(...) and before incrementing exception_command_counter/setting failed you invoke the same logging path used in the normal apply path (i.e. emit the apply-change JSON/log entry for queued_message->message and sql) — mirror the log call that handle_sql() or the code at the earlier log site uses so skipped/replay DDL still gets recorded while keeping the skip behavior for execution. Ensure you reference MyApplyWorker->use_try_block, exception_behaviour, TRANSDISCARD, SUB_DISABLE, JsonbToCString, queued_message, exception_command_counter and failed in the change.src/spock_sync.c (1)
16-16:⚠️ Potential issue | 🔴 CriticalPotential Windows build break: unguarded
<sys/wait.h>+fork()/waitpid()insrc/spock_sync.c🤖 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 `@src/spock_sync.c` at line 16, The unguarded `#include` <sys/wait.h> and use of POSIX APIs (fork(), waitpid()) in src/spock_sync.c will break Windows builds; wrap the include and any code that calls fork()/waitpid() in platform checks (e.g. `#if` !defined(_WIN32) || defined(HAVE_SYS_WAIT_H) ... `#endif`) and provide a Windows-safe alternative or stub for the spock sync path (use CreateProcess/WaitForSingleObject or return a clear "not supported on Windows" error). Specifically, guard the <sys/wait.h> include and all occurrences of fork() and waitpid() so Windows compiles, or add `#ifdef` _WIN32 branches that implement equivalent behavior or disable the feature.
🤖 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.
Outside diff comments:
In `@src/spock_apply.c`:
- Around line 2393-2409: When exception_behaviour is TRANSDISCARD or SUB_DISABLE
the current branch extracts sql with JsonbToCString and skips calling
handle_sql(), which prevents emitting the DDL "spock apply change" log; modify
the TRANSDISCARD/SUB_DISABLE branch in spock_apply.c so after sql =
JsonbToCString(...) and before incrementing exception_command_counter/setting
failed you invoke the same logging path used in the normal apply path (i.e. emit
the apply-change JSON/log entry for queued_message->message and sql) — mirror
the log call that handle_sql() or the code at the earlier log site uses so
skipped/replay DDL still gets recorded while keeping the skip behavior for
execution. Ensure you reference MyApplyWorker->use_try_block,
exception_behaviour, TRANSDISCARD, SUB_DISABLE, JsonbToCString, queued_message,
exception_command_counter and failed in the change.
In `@src/spock_sync.c`:
- Line 16: The unguarded `#include` <sys/wait.h> and use of POSIX APIs (fork(),
waitpid()) in src/spock_sync.c will break Windows builds; wrap the include and
any code that calls fork()/waitpid() in platform checks (e.g. `#if`
!defined(_WIN32) || defined(HAVE_SYS_WAIT_H) ... `#endif`) and provide a
Windows-safe alternative or stub for the spock sync path (use
CreateProcess/WaitForSingleObject or return a clear "not supported on Windows"
error). Specifically, guard the <sys/wait.h> include and all occurrences of
fork() and waitpid() so Windows compiles, or add `#ifdef` _WIN32 branches that
implement equivalent behavior or disable the feature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5980fd5a-34c1-424f-b966-4660e1f83bc9
📒 Files selected for processing (5)
src/spock_apply.csrc/spock_change_log.csrc/spock_sync.ctests/tap/scheduletests/tap/t/044_apply_change_logging.pl
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/tap/t/044_apply_change_logging.pl
4 participants
log_verbosity (normal|verbose) promotes spock DEBUG1/DEBUG2 to LOG. apply_change_logging (none|key_only|verbose) emits per-change JSON from the apply worker: action/schema/table/pk/origin/commit_ts in key_only, plus old/new row data in verbose. DDL is logged in both non-none modes. Both GUCs are PGC_SUSET (SET / SIGHUP reload).
TAP coverage in tests/tap/t/044_apply_change_logging.pl - 24/24.
SPOC-551