Conversation
📝 WalkthroughWalkthroughThis pull request removes Windows-specific declarations and code paths, consolidating process and temp-directory handling to POSIX-only implementations, updates PostgreSQL version gating for a build helper, deletes Windows typedefs from pgindent config, and adds a comment to spock conflict documentation. (48 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.
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.
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
1 participant
Removed all the Windows code as we don't intend to support it.