Conversation
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.
Group the head install script by feature area and normalise function signatures to a single style, so the script reads as a coherent user-facing interface rather than the accumulated insertion-order history it had become. No functional changes; upgrade scripts untouched.
📝 WalkthroughWalkthroughThis PR removes Windows platform support and reorganizes the Spock extension SQL API. Changes eliminate Windows-specific headers, exec implementations, and temp directory handling across C code; remove Windows typedef entries; and substantially expand the SQL migration with new catalog tables, subscription/replication management functions, apply progress tracking, and sync event handling. ChangesWindows Support Removal
SQL Extension API Reorganization
C Code Adjustments
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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
|
| Metric | Results |
|---|---|
| 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
| * So, don't care about duplicates. | ||
| */ | ||
|
|
||
| #if PG_VERSION_NUM >= 180000 |
There was a problem hiding this comment.
I hadn't noticed --exclude-extension before, but it looks like it got added in Postgres 17.
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.