Thread

Re: Non-blocking archiver process

Artem Gavrilov <artem.gavrilov@percona.com> — 2025-09-25T11:13:24Z

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.gavrilov@percona.com