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 1 commit into
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 (9)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds new logging GUCs and public logging APIs, emits JSON change logs for replicated DML and DDL in the apply worker, routes selected debug messages through new macros, and adds TAP coverage for the new behavior. ChangesApply 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)
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 | 2 medium |
🟢 Metrics 42 complexity · 0 duplication
Metric Results Complexity 42 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
ibrarahmad
force-pushed
the
SPOC-551
branch
2 times, most recently
from
a374f5e to
66ba3cd
Compare
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
rasifr
left a comment
rasifr
left a comment
There was a problem hiding this comment.
-
Although I still favor single GUC, even accepting two separate GUCs, the new SPOCK_DEBUG1/SPOCK_DEBUG2 macros are unnecessary complexity. Every existing ereport(DEBUG1/2, …) is already gated by log_min_messages — that is exactly the mechanism Postgres provides for controlling what debug output reaches the log. Wrapping that with a spock-specific level macro that duplicates the concept adds indirection without adding capability.
If the goal is to promote spock debug messages to LOG without touching log_min_messages globally, the GUC alone can do that at the call site with a simple helper function. -
spock_jsonb_utils.c already provides spock_tuple_to_json_cstring() — the same conceptual operation this PR implements again in append_row_json. The right approach is to extend spock_jsonb_utils.c to cover the additional requirements (such as PK-only filtering etc) rather than introduce a parallel set of JSON helpers in a new file.
-
The removal of the entire Win32 exec_cmd path (~324 lines) and the PG_VERSION_NUM guard change from >= 180000 to >= 170000 for build_exclude_extension_string have no connection to the GUC feature being added here. These should be in a separate PR
OK, now I understand what you meant.
@ibrarahmad I agree with Asif.
I think he just needs to rebase, that was already removed. |
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.
|
Gave this a rebase because of the extra windows code removal changes |
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