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.
📝 WalkthroughWalkthroughThis pull request removes Windows platform support from the codebase by eliminating Windows-specific function declarations, process handling logic, temporary directory resolution, and typedef entries, consoliding implementations to POSIX-only paths with version-gating for PostgreSQL compatibility. 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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.
1 participant
Removed all the Windows code as we don't intend to support it.