Arbitrary improvements (take 2)#464
Conversation
📝 WalkthroughWalkthroughThis PR removes Windows platform support from the Spock extension codebase by eliminating Windows-specific implementations, declarations, and type definitions. Concurrently, it adjusts PostgreSQL version-specific compilation guards to align extension features with their PostgreSQL release requirements. ChangesWindows Support Removal
PostgreSQL Version-Specific Compilation
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| 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.
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)
156-174:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLower version gate to PG17 —
--exclude-extensionships in PG17, not PG18.The
--exclude-extensionpg_dump option was introduced in PostgreSQL 17 (commit 522ed12f, March 2024), not PG18. As written,build_exclude_extension_string()is compiled out on PG17 builds, causing extensions inskip_extension[]to be silently dumped even thoughpg_dump 17supports the flag.Change
PG_VERSION_NUM >= 180000toPG_VERSION_NUM >= 170000at:
- Line 156 (function definition)
- Line 239 (call site in
dump_structure())🤖 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` around lines 156 - 174, The build currently gates build_exclude_extension_string() behind PG_VERSION_NUM >= 180000 but the --exclude-extension option exists in PG17; change the preprocessor condition to PG_VERSION_NUM >= 170000 in both the function definition around build_exclude_extension_string and at its call site in dump_structure() so the function is compiled and used for PG17+ builds; update the `#if/`#endif macro checks referencing PG_VERSION_NUM accordingly.
🧹 Nitpick comments (1)
sql/spock--6.0.0-devel.sql (1)
743-751: 💤 Low valueInconsistent
CREATE OR REPLACE FUNCTIONand stale upgrade-script comment.Every other function declaration in this install script uses plain
CREATE FUNCTION.CREATE OR REPLACEis a no-op in a fresh install (the function cannot pre-exist after\quit), and it cuts against this PR's stated goal of normalizing declarations to a uniform shape. The trailing-- Changed from int to bigintcomment is also a leftover from an upgrade script and is meaningless in the head install file.♻️ Proposed cleanup
-CREATE OR REPLACE FUNCTION spock.get_apply_worker_status( - OUT worker_pid bigint, -- Changed from int to bigint +CREATE FUNCTION spock.get_apply_worker_status( + OUT worker_pid bigint, OUT worker_dboid int, OUT worker_subid bigint, OUT worker_status text ) RETURNS SETOF record AS 'MODULE_PATHNAME', 'get_apply_worker_status' LANGUAGE C STABLE;🤖 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 `@sql/spock--6.0.0-devel.sql` around lines 743 - 751, Replace the non-standard declaration and stale comment for the function spock.get_apply_worker_status: change the header from "CREATE OR REPLACE FUNCTION" to "CREATE FUNCTION" and remove the trailing upgrade-script comment "-- Changed from int to bigint" (and any similar leftover comments around the OUT parameter worker_pid) so the declaration matches the other functions' plain CREATE FUNCTION form and contains no stale upgrade notes.
🤖 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`:
- Around line 156-174: The build currently gates
build_exclude_extension_string() behind PG_VERSION_NUM >= 180000 but the
--exclude-extension option exists in PG17; change the preprocessor condition to
PG_VERSION_NUM >= 170000 in both the function definition around
build_exclude_extension_string and at its call site in dump_structure() so the
function is compiled and used for PG17+ builds; update the `#if/`#endif macro
checks referencing PG_VERSION_NUM accordingly.
---
Nitpick comments:
In `@sql/spock--6.0.0-devel.sql`:
- Around line 743-751: Replace the non-standard declaration and stale comment
for the function spock.get_apply_worker_status: change the header from "CREATE
OR REPLACE FUNCTION" to "CREATE FUNCTION" and remove the trailing upgrade-script
comment "-- Changed from int to bigint" (and any similar leftover comments
around the OUT parameter worker_pid) so the declaration matches the other
functions' plain CREATE FUNCTION form and contains no stale upgrade notes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7dfbb6d9-866e-4b79-a518-83cd1c9e8aea
📒 Files selected for processing (7)
include/spock_sync.hsql/spock--6.0.0-devel.sqlsrc/spock.csrc/spock_exception_handler.csrc/spock_failover_slots.csrc/spock_sync.cutils/pgindent/typedefs.list
💤 Files with no reviewable changes (3)
- include/spock_sync.h
- src/spock.c
- utils/pgindent/typedefs.list
danolivo
force-pushed
the
arbitrary-improvements-take-2
branch
from
f883fab to
e44418a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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)
1461-1505:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegister slot cleanup before creating the slot.
If
spock_create_slot_and_read_progress()throws afterCREATE_REPLICATION_SLOTsucceeds, this catch block only resumes apply workers. The cleanup callback is installed later, so the new remote slot can be left behind retaining WAL until another retry/manual cleanup. Either wrap this call in the existingPG_ENSURE_ERROR_CLEANUP(...)scope, or do a best-effort slot drop here before rethrowing.Also applies to: 1513-1515
🤖 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` around lines 1461 - 1505, The call to spock_create_slot_and_read_progress can create a replication slot before the cleanup callback is registered, leaving the slot and WAL behind if an exception is thrown; wrap the spock_create_slot_and_read_progress call in a PG_ENSURE_ERROR_CLEANUP scope (or otherwise register the slot-drop cleanup callback before calling it) so the cleanup is guaranteed on error, or alternatively add a best-effort drop inside the PG_CATCH before PG_RE_THROW: use origin_conn and sub->slot_name to drop the newly created slot (via the existing slot-drop helper or a PQexec/pg_drop_replication_slot call) and clear any PQresult, then continue with the current error flow.
🤖 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 `@sql/spock--6.0.0-devel.sql`:
- Line 663: Summary: Demote or remove noisy RAISE NOTICE tracing in the internal
helper so it doesn't surface on every sync run. Locate the RAISE NOTICE call
that emits "SPOCK cswp slot=% v_lsn=%" using the p_slot_name and v_lsn variables
(and the similar RAISE NOTICE calls in the same helper around the block at lines
709-715) and either change it to a lower-visibility level like RAISE DEBUG/LOG
or remove the statement entirely; ensure you apply the same change to the other
NOTICE lines in that helper for consistent behavior.
---
Outside diff comments:
In `@src/spock_sync.c`:
- Around line 1461-1505: The call to spock_create_slot_and_read_progress can
create a replication slot before the cleanup callback is registered, leaving the
slot and WAL behind if an exception is thrown; wrap the
spock_create_slot_and_read_progress call in a PG_ENSURE_ERROR_CLEANUP scope (or
otherwise register the slot-drop cleanup callback before calling it) so the
cleanup is guaranteed on error, or alternatively add a best-effort drop inside
the PG_CATCH before PG_RE_THROW: use origin_conn and sub->slot_name to drop the
newly created slot (via the existing slot-drop helper or a
PQexec/pg_drop_replication_slot call) and clear any PQresult, then continue with
the current error flow.
🪄 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: c149bee6-1f5f-4e85-aaa8-13f55734b9a7
📒 Files selected for processing (7)
include/spock_sync.hsql/spock--6.0.0-devel.sqlsrc/spock.csrc/spock_exception_handler.csrc/spock_failover_slots.csrc/spock_sync.cutils/pgindent/typedefs.list
💤 Files with no reviewable changes (3)
- src/spock.c
- utils/pgindent/typedefs.list
- include/spock_sync.h
✅ Files skipped from review due to trivial changes (1)
- src/spock_exception_handler.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spock_failover_slots.c
|
|
||
| CREATE FUNCTION spock.sub_alter_sync(subscription_name name, truncate boolean DEFAULT false) | ||
| RETURNS boolean STRICT VOLATILE LANGUAGE c AS 'MODULE_PATHNAME', 'spock_alter_subscription_synchronize'; | ||
| RAISE NOTICE 'SPOCK cswp slot=% v_lsn=%', p_slot_name, v_lsn; |
There was a problem hiding this comment.
Drop the NOTICE-level tracing from this internal helper.
These RAISE NOTICE calls will surface on every sync run, so this turns an internal helper into user-visible log/client noise. Please demote them to DEBUG/LOG or remove them before merge.
Also applies to: 709-715
🤖 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 `@sql/spock--6.0.0-devel.sql` at line 663, Summary: Demote or remove noisy
RAISE NOTICE tracing in the internal helper so it doesn't surface on every sync
run. Locate the RAISE NOTICE call that emits "SPOCK cswp slot=% v_lsn=%" using
the p_slot_name and v_lsn variables (and the similar RAISE NOTICE calls in the
same helper around the block at lines 709-715) and either change it to a
lower-visibility level like RAISE DEBUG/LOG or remove the statement entirely;
ensure you apply the same change to the other NOTICE lines in that helper for
consistent behavior.
It is just the fact that --exclude-extension has been introduced in Postgres 18.
Drop Win32-specific code paths including exec_cmd_win32(), GetTempPath() temp directory resolution, Windows argument quoting, and related type definitions. Spock targets POSIX-only environments.
It may be unclear to detect if someone forget to commit after the call of this function wiping out the result. So, add a comment to reduce number of coding errors.
As mentioned in the review it is available since PostgreSQL 17.
danolivo
force-pushed
the
arbitrary-improvements-take-2
branch
from
e44418a to
6ad21e6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/spock.c (1)
71-78: ⚡ Quick winConsider using
#ifdef PG_MODULE_MAGIC_EXTfor better compatibility detection.The syntax is correct—the static analysis error is a false positive from tools unfamiliar with PostgreSQL 18 headers. However, PostgreSQL documentation recommends checking for macro availability via
#ifdef PG_MODULE_MAGIC_EXTrather than numeric version checks, as it more reliably detects support across different builds:Suggested change
`#ifdef` PG_MODULE_MAGIC_EXT PG_MODULE_MAGIC_EXT( .name = "spock", .version = SPOCK_VERSION ); `#else` PG_MODULE_MAGIC; `#endif`🤖 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.c` around lines 71 - 78, Replace the numeric version check with a macro-availability check: detect PG_MODULE_MAGIC_EXT via `#ifdef` instead of comparing PG_VERSION_NUM, and use PG_MODULE_MAGIC_EXT(...) with .name = "spock" and .version = SPOCK_VERSION when available, falling back to PG_MODULE_MAGIC otherwise; update the block that currently references PG_VERSION_NUM, PG_MODULE_MAGIC_EXT, PG_MODULE_MAGIC and SPOCK_VERSION accordingly.
🤖 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.
Nitpick comments:
In `@src/spock.c`:
- Around line 71-78: Replace the numeric version check with a macro-availability
check: detect PG_MODULE_MAGIC_EXT via `#ifdef` instead of comparing
PG_VERSION_NUM, and use PG_MODULE_MAGIC_EXT(...) with .name = "spock" and
.version = SPOCK_VERSION when available, falling back to PG_MODULE_MAGIC
otherwise; update the block that currently references PG_VERSION_NUM,
PG_MODULE_MAGIC_EXT, PG_MODULE_MAGIC and SPOCK_VERSION accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9beea260-402b-44d9-97d0-9c2dfce56ddd
📒 Files selected for processing (6)
include/spock_sync.hsrc/spock.csrc/spock_exception_handler.csrc/spock_failover_slots.csrc/spock_sync.cutils/pgindent/typedefs.list
💤 Files with no reviewable changes (2)
- include/spock_sync.h
- utils/pgindent/typedefs.list
✅ Files skipped from review due to trivial changes (1)
- src/spock_exception_handler.c
2 participants
Summary
A grab-bag of independent cleanups in
contrib/spock. None of them change user-visible behaviour; each commit stands on its own and can be reviewed separately.--exclude-extension. The flag is new in Postgres 18, so its use is now gated behind a version check rather than relying on always-present API.spock_disable_subscriptionwarning comment. Documents that the function's effect can be silently lost if the caller forgets to commit, which has caused coding errors before.spock--6.0.0-devel.sqlUI tidy-up. Regroups the head install script into feature sections and normalises everyCREATE FUNCTIONto a single multi-line shape, so the script reads as a user-facing API rather than insertion-order history. Upgrade scripts are untouched.