Re: Non-blocking archiver process

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

artem(dot)gavrilov(at)percona(dot)com