Conversation
📝 WalkthroughWalkthroughThis pull request removes Windows-specific APIs and code paths (header and source), consolidates process execution and temp-directory logic to POSIX, gates a build helper by PostgreSQL version, trims Windows typedefs from pgindent config, and updates two documentation comments. (34 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 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 |
![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
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.
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
from
ec11ab0 to
8319df8
Compare
1 participant
Removed all the Windows code as we don't intend to support it.