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:
📝 WalkthroughWalkthroughRemoved Windows-specific code paths and identifiers: deleted a Windows-only header declaration, removed Win32 process-exec helpers and temp-dir branch, gated a helper by PostgreSQL version, trimmed Win32 typedefs in pgindent config, and updated two internal doc comments. (30 words) 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)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/spock_sync.c (2)
106-131:⚠️ Potential issue | 🟡 MinorUnhandled
fork()failure:pid = -1falls through towaitpid(-1, …).With Windows code removed, this is the sole code path. When
fork()returns-1:
- The child block is skipped (correct).
waitpid(-1, &stat, 0)is then called, which waits for any child of the process — an unintended side-effect that could reap an unrelated child.- If
waitpiditself returns-1(e.g.,ECHILD), the checkif (-1 != -1)is false andstatis returned uninitialized (undefined behavior).🐛 Proposed fix
if ((pid = fork()) == 0) { if (execv(cmd, cmdargv) < 0) { ereport(ERROR, (errmsg("could not execute \"%s\": %m", cmd))); /* We're already in the child process here, can't return */ exit(1); } } + else if (pid < 0) + { + /* fork() failed */ + return -1; + } if (waitpid(pid, &stat, 0) != pid) stat = -1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_sync.c` around lines 106 - 131, Handle the case where fork() returns -1 in exec_cmd: after the fork() call check for pid == -1, log/ereport the fork failure (including errno via %m) and return an error indicator (e.g., set stat = -1 and return it) without calling waitpid; only call waitpid when pid > 0 and proceed with the existing child handling for pid == 0 in the same function.
156-174:⚠️ Potential issue | 🟠 MajorWrong PostgreSQL version threshold —
--exclude-extensionwas added in PostgreSQL 17.The
pg_dumpoption--exclude-extensionwas introduced in PostgreSQL 17, but the code guards it withPG_VERSION_NUM >= 180000(PostgreSQL 18). This causesbuild_exclude_extension_string()to be compiled out on PostgreSQL 17, preventing extensions inskip_extension[]from being filtered from dumps.Both locations need fixing:
- Line 156: Function definition guard
- Line 238: Call site guard
Change both from
#if PG_VERSION_NUM >= 180000to#if PG_VERSION_NUM >= 170000.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_sync.c` around lines 156 - 174, The preprocessor guards erroneously require PostgreSQL 18 for the build_exclude_extension_string feature; update both occurrences that wrap the function and its call site to use PG_VERSION_NUM >= 170000 instead of PG_VERSION_NUM >= 180000 so build_exclude_extension_string() and its use (which relies on --exclude-extension and the skip_extension[] list) are compiled for PostgreSQL 17+; locate the two guards surrounding build_exclude_extension_string and its invocation and change the numeric threshold from 180000 to 170000.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/spock_sync.c`:
- Around line 106-131: Handle the case where fork() returns -1 in exec_cmd:
after the fork() call check for pid == -1, log/ereport the fork failure
(including errno via %m) and return an error indicator (e.g., set stat = -1 and
return it) without calling waitpid; only call waitpid when pid > 0 and proceed
with the existing child handling for pid == 0 in the same function.
- Around line 156-174: The preprocessor guards erroneously require PostgreSQL 18
for the build_exclude_extension_string feature; update both occurrences that
wrap the function and its call site to use PG_VERSION_NUM >= 170000 instead of
PG_VERSION_NUM >= 180000 so build_exclude_extension_string() and its use (which
relies on --exclude-extension and the skip_extension[] list) are compiled for
PostgreSQL 17+; locate the two guards surrounding build_exclude_extension_string
and its invocation and change the numeric threshold from 180000 to 170000.
danolivo
force-pushed
the
arbitrary-improvements
branch
from
deb228a to
22cd40b
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)
156-174:⚠️ Potential issue | 🟠 MajorWrong version threshold:
--exclude-extensionwas added in PostgreSQL 17, not 18.
--exclude-extensionwas added topg_dumpin PostgreSQL 17 (commit522ed12f), confirmed by PostgreSQL.org documentation. The current code uses#if PG_VERSION_NUM >= 180000at lines 156 and 238, which meansbuild_exclude_extension_string()is never called for PostgreSQL 17 installations. This silently omits the--exclude-extensionarguments from thepg_dumpinvocation, causing extensions that should be filtered to be included in the sync dump.Proposed fix
Change the version threshold from
180000to170000in two locations:
- Line 156 (function guard):
-#if PG_VERSION_NUM >= 180000 +#if PG_VERSION_NUM >= 170000
- Line 238 (function call):
-#if PG_VERSION_NUM >= 180000 +#if PG_VERSION_NUM >= 170000🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_sync.c` around lines 156 - 174, The preprocessor threshold incorrectly requires PG_VERSION_NUM >= 180000 so build_exclude_extension_string() (which builds "--exclude-extension=" args using skip_extension[] and get_extension_oid()) is not compiled for PostgreSQL 17; change the two conditional checks that use 180000 to 170000 (the function guard around build_exclude_extension_string and the corresponding conditional where the function is invoked) so the function and its invocation are included for PG 17+.
🧹 Nitpick comments (1)
src/spock_sync.c (1)
116-128: Pre-existing: unhandledfork()failure will callwaitpid(-1, …)and may reap an unrelated child.If
fork()returns-1,pidis-1and the code falls through towaitpid(-1, &stat, 0), which waits for any child process rather than returning an error immediately. While this is unlikely to cause data corruption in practice (PostgreSQL backends rarely have unrelated children), the correct handling is an explicit failure guard:🛡️ Suggested fix (pre-existing, not introduced by this PR)
if ((pid = fork()) == 0) { if (execv(cmd, cmdargv) < 0) { ereport(ERROR, (errmsg("could not execute \"%s\": %m", cmd))); - /* We're already in the child process here, can't return */ - exit(1); + _exit(1); /* must not longjmp in child; ereport above may not return */ } } + else if (pid < 0) + ereport(ERROR, + (errmsg("could not fork to execute \"%s\": %m", cmd))); + if (waitpid(pid, &stat, 0) != pid)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_sync.c` around lines 116 - 128, The code does not handle fork() failure: if fork() returns -1 the code falls through and calls waitpid(-1, &stat, 0). Add an explicit error branch for pid == -1 right after the fork() call (before any waitpid) that logs/raises an error via ereport(ERROR, ...) including %m or strerror(errno) and sets stat to -1 (or returns/propagates failure) so waitpid is not called with -1; keep the existing child branch (pid == 0) where execv is attempted and the parent branch where waitpid(pid, &stat, 0) is used unchanged. Ensure you reference the same symbols (pid, fork(), waitpid, execv, stat) when locating and implementing the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/spock_sync.c`:
- Around line 156-174: The preprocessor threshold incorrectly requires
PG_VERSION_NUM >= 180000 so build_exclude_extension_string() (which builds
"--exclude-extension=" args using skip_extension[] and get_extension_oid()) is
not compiled for PostgreSQL 17; change the two conditional checks that use
180000 to 170000 (the function guard around build_exclude_extension_string and
the corresponding conditional where the function is invoked) so the function and
its invocation are included for PG 17+.
---
Nitpick comments:
In `@src/spock_sync.c`:
- Around line 116-128: The code does not handle fork() failure: if fork()
returns -1 the code falls through and calls waitpid(-1, &stat, 0). Add an
explicit error branch for pid == -1 right after the fork() call (before any
waitpid) that logs/raises an error via ereport(ERROR, ...) including %m or
strerror(errno) and sets stat to -1 (or returns/propagates failure) so waitpid
is not called with -1; keep the existing child branch (pid == 0) where execv is
attempted and the parent branch where waitpid(pid, &stat, 0) is used unchanged.
Ensure you reference the same symbols (pid, fork(), waitpid, execv, stat) when
locating and implementing the guard.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
include/spock_conflict.hinclude/spock_sync.hsrc/spock.csrc/spock_sync.cutils/pgindent/typedefs.list
💤 Files with no reviewable changes (3)
- src/spock.c
- utils/pgindent/typedefs.list
- include/spock_sync.h
danolivo
force-pushed
the
arbitrary-improvements
branch
from
22cd40b to
8319df8
Compare
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
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_sync.c (2)
116-128:⚠️ Potential issue | 🟡 MinorMissing error handling for
fork()failure.If
fork()returns-1(failure), the code proceeds to callwaitpid(-1, &stat, 0), which waits for any child process rather than handling the fork error gracefully. This could cause unexpected behavior or hangs if there are other child processes.🛡️ Proposed fix to handle fork() failure
if ((pid = fork()) == 0) { if (execv(cmd, cmdargv) < 0) { ereport(ERROR, (errmsg("could not execute \"%s\": %m", cmd))); /* We're already in the child process here, can't return */ exit(1); } } + else if (pid < 0) + { + /* fork() failed */ + return -1; + } if (waitpid(pid, &stat, 0) != pid) stat = -1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_sync.c` around lines 116 - 128, The code does not check for fork() returning -1, so it may call waitpid(pid, ...) with pid == -1; update the fork/child/parent block around fork(), execv, waitpid to handle fork failure: after calling fork() store the result in pid and if pid == -1 log/ereport an error and set stat = -1 (or return/error out) instead of falling through to waitpid; keep the existing child path (pid == 0) that calls execv and exits on failure, and in the parent path (pid > 0) call waitpid(pid, &stat, 0) as before. Ensure you reference the same variables and functions (fork(), execv(), waitpid(), pid, stat) when implementing the checks so behavior is deterministic on fork failure.
156-174:⚠️ Potential issue | 🔴 CriticalIncorrect version guard:
--exclude-extensionwas added in PostgreSQL 17, not 18.The preprocessor guard uses
#if PG_VERSION_NUM >= 180000, but the--exclude-extensionoption forpg_dumpwas introduced in PostgreSQL 17. Change the guard to#if PG_VERSION_NUM >= 170000to enable this functionality for PostgreSQL 17 users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_sync.c` around lines 156 - 174, The preprocessor guard is off-by-one: the build_exclude_extension_string function (which builds "--exclude-extension=%s" args from skip_extension entries and uses get_extension_oid()) is currently enabled only for PG_VERSION_NUM >= 180000 but the pg_dump "--exclude-extension" flag was added in PostgreSQL 17; change the version check to `#if` PG_VERSION_NUM >= 170000 so build_exclude_extension_string is compiled for PG 17+.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/spock_sync.c`:
- Around line 116-128: The code does not check for fork() returning -1, so it
may call waitpid(pid, ...) with pid == -1; update the fork/child/parent block
around fork(), execv, waitpid to handle fork failure: after calling fork() store
the result in pid and if pid == -1 log/ereport an error and set stat = -1 (or
return/error out) instead of falling through to waitpid; keep the existing child
path (pid == 0) that calls execv and exits on failure, and in the parent path
(pid > 0) call waitpid(pid, &stat, 0) as before. Ensure you reference the same
variables and functions (fork(), execv(), waitpid(), pid, stat) when
implementing the checks so behavior is deterministic on fork failure.
- Around line 156-174: The preprocessor guard is off-by-one: the
build_exclude_extension_string function (which builds "--exclude-extension=%s"
args from skip_extension entries and uses get_extension_oid()) is currently
enabled only for PG_VERSION_NUM >= 180000 but the pg_dump "--exclude-extension"
flag was added in PostgreSQL 17; change the version check to `#if` PG_VERSION_NUM
>= 170000 so build_exclude_extension_string is compiled for PG 17+.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d291182f-f9f7-4f98-a0b6-86f06f02da87
📒 Files selected for processing (6)
include/spock_conflict.hinclude/spock_sync.hsrc/spock.csrc/spock_exception_handler.csrc/spock_sync.cutils/pgindent/typedefs.list
💤 Files with no reviewable changes (3)
- src/spock.c
- include/spock_sync.h
- utils/pgindent/typedefs.list
✅ Files skipped from review due to trivial changes (2)
- include/spock_conflict.h
- src/spock_exception_handler.c
danolivo
force-pushed
the
arbitrary-improvements
branch
3 times, most recently
from
e5d44bd to
7cc98a1
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)
109-128:⚠️ Potential issue | 🔴 CriticalHandle
fork()failure before callingwaitpid().If
fork()returns-1,pidis-1andwaitpid(-1, &stat, 0)will also return-1when this worker has no children. Because the comparison is againstpid, the fallback on line 128 is skipped andexec_cmd()can return an uninitializedstat, causing a spawn failure to be misclassified. Add an explicitpid < 0check immediately afterfork()and initializestat = -1to ensure correct error handling.Suggested fix
static int exec_cmd(const char *cmd, char *cmdargv[]) { pid_t pid; - int stat; + int stat = -1; /* Fire off execv in child */ fflush(stdout); fflush(stderr); - if ((pid = fork()) == 0) + pid = fork(); + if (pid < 0) + return -1; + if (pid == 0) { if (execv(cmd, cmdargv) < 0) { ereport(ERROR, (errmsg("could not execute \"%s\": %m", cmd)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_sync.c` around lines 109 - 128, After calling fork() in exec_cmd(), handle a fork failure: immediately check if pid < 0, set stat = -1 to initialize the exit status, and report the error (e.g., ereport(ERROR, (errmsg("could not fork: %m"))) or equivalent) before returning/aborting; this ensures waitpid(pid, &stat, 0) is not called with pid == -1 and avoids returning an uninitialized stat. Reference the pid variable, the fork() call, the stat variable, and the waitpid() call when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/spock_sync.c`:
- Around line 109-128: After calling fork() in exec_cmd(), handle a fork
failure: immediately check if pid < 0, set stat = -1 to initialize the exit
status, and report the error (e.g., ereport(ERROR, (errmsg("could not fork:
%m"))) or equivalent) before returning/aborting; this ensures waitpid(pid,
&stat, 0) is not called with pid == -1 and avoids returning an uninitialized
stat. Reference the pid variable, the fork() call, the stat variable, and the
waitpid() call when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54232744-b0b9-40ef-bf86-e411e5f14b8e
📒 Files selected for processing (6)
include/spock_conflict.hinclude/spock_sync.hsrc/spock.csrc/spock_exception_handler.csrc/spock_sync.cutils/pgindent/typedefs.list
💤 Files with no reviewable changes (3)
- include/spock_sync.h
- src/spock.c
- utils/pgindent/typedefs.list
✅ Files skipped from review due to trivial changes (2)
- src/spock_exception_handler.c
- include/spock_conflict.h
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.
No functional changes. Pure formatting: every C function CREATE statement in spock--5.0.0--5.0.1.sql, spock--5.0.1--5.0.2.sql, spock--5.0.6--6.0.0-devel.sql, and spock--6.0.0-devel.sql is reformatted to match the established convention: - One parameter per line, indented two spaces - IN parameter names column-aligned within each function - RETURNS clause on its own line - AS 'MODULE_PATHNAME', 'symbol' on its own line - LANGUAGE C [attributes] as the final line before the semicolon Also normalises DEFAULT keyword (replacing = shorthand) and converts tab indentation to two spaces throughout.
danolivo
force-pushed
the
arbitrary-improvements
branch
from
8ba228a to
c353c1d
Compare
mason-sharp
left a comment
mason-sharp
left a comment
There was a problem hiding this comment.
These look good to me:
Re: check_all_uc_indexes, it is used if it can’t find the replica identity to try to find a fallback key to use, different than the description. I am not sure if we should have a comment for it in the enum definition anyway.
Regarding the formatting, I don’t think I would want us to modify the previously released sql files. For spock—6.0.0-devel.sql file, that is a different story.
I think we must: by this, we break vanilla's behaviour (and ignore the same in UPDATE as well). So, we should say words on that. I just spent hours, if not a day, realising why my code and tests don't work because I rely on my knowledge of upstream Postgres. So, to save time and let people understand why we go against the upstream, it makes sense.
Ok. I just note that formatting doesn't break anything, even in the old versions. |
2 participants
Removed all the Windows code as we don't intend to support it.