Hello Patrick
I did a review of your patch.
Initial Run
===========
The patch applies cleanly to HEAD (196063d6761). All tests successfully
pass on MacOS 15.7.
Comments
===========
1) Instead of `malloc` and `free` it should be `palloc` and `pfree`.
2) In fact `archiveFileno` is a file descriptor, and the first variable one
is not. I think it's worth to rename them to `archiveFile` and `archiveFd`.
Or maybe even just `file` and `fd` as the scope there is not so big.
>
> FILE *archiveFd = NULL;
> int archiveFileno;
3) Variable name `bytesRead` is rare in PG code base. It is used only two
times, while `readBytes` is used four times. Other variants, like `nbytes`
are more common. Let's pick some popular name.
4) Variable name `dwRc` is unique for the PG codebase and not really clear
as for me. How about name it just `exitcode`?
5) `return` is redundant here as it will never be reached.
ereport(FATAL,
> (errmsg_internal("Failed to malloc win32cmd %m")));
> return false;
6) Same here, `free` and `return` are unreachable due ereport with fatal
level.
> ereport(FATAL,
> (errmsg("CreateProcess() call failed: %m (error code %lu)",
> GetLastError())));
> free(win32cmd);
> return false;
7) This loop can be stuck forever as `WaitForSingleObject` may
return `WAIT_FAILED*` *and it's not always retriable.
> while (true)
> {
> CHECK_FOR_INTERRUPTS();
> if (WaitForSingleObject(pi.hProcess, POLL_TIMEOUT_MSEC) ==
> WAIT_OBJECT_0)
> break;
> }
--
<https://www.percona.com/>
Artem Gavrilov
Senior Software Engineer, Percona
[email protected]
