Hi,
Thanks for the reviews!
On 2025-01-09 12:01:05 +0000, Bertrand Drouvot wrote:
> On Wed, Jan 08, 2025 at 02:26:15PM -0500, Andres Freund wrote:
> === 2
>
> + PM_WAIT_XLOG_ARCHIVAL, /* waiting for archiver and walsenders to
>
> > I don't love PM_WAIT_XLOG_ARCHIVAL, but I can't think of anything better.
>
> PM_WAIT_ARCHIVER_WALSENDERS maybe? (that would follow the pattern of naming
> the processes like PM_WAIT_BACKENDS, PM_WAIT_CHECKPOINTER for example).
>
> That said, I'm not 100% convinced it makes it more clear though...
Yea, I just went with PM_WAIT_XLOG_ARCHIVAL, it's ok enough.
> > The last two (0006: trigger checkpoints via SetLatch, 0007: change the
> > shutdown sequence), probably can use a few more eyes.
>
> 0006,
>
> === 3
>
> + * when it does start, with or without getting a signal.
>
> s/getting a signal/getting a latch set/ or "getting notified"?
I went with "with or without the SetLatch()".
> === 4
>
> + if (checkpointerProc == INVALID_PROC_NUMBER)
> {
> if (ntries >= MAX_SIGNAL_TRIES || !(flags & CHECKPOINT_WAIT))
> {
> elog((flags & CHECKPOINT_WAIT) ? ERROR : LOG,
> - "could not signal for checkpoint: checkpointer is not
> running");
> + "could not notify checkpoint: checkpointer is not running");
>
> Worth renaming MAX_SIGNAL_TRIES with MAX_NOTIFY_TRIES then?
IDK, didn't seem worth it. SetLatch() is a form of signalling and I don't
think "notify" really is better. And it requires changing more lines...
> === 5
>
> + pqsignal(SIGINT, ReqShutdownXLOG);
>
> Worth a comment like:
>
> pqsignal(SIGINT, ReqShutdownXLOG); /* trigger shutdown checkpoint */
That seems just a restatement of the function name, don't think we gain
anything by that.
Greetings,
Andres Freund
