Re: shared-memory based stats collector

At Tue, 9 Mar 2021 23:24:10 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote in
>
>
> On 2021/03/09 16:51, Kyotaro Horiguchi wrote:
> > At Sat, 6 Mar 2021 00:32:07 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>
> > wrote in
> >> I don't think that we should treat non-zero exit condition as a crash,
> >> as before. Otherwise when archive_command fails on a signal,
> >> archiver emits FATAL error and which leads the server restart.
> > Sounds reasonable. Now archiver is treated the same way to wal
> > receiver. Specifically exit(1) doesn't cause server restart.
>
> Thanks!
>
> - if (PgArchStartupAllowed())
> - PgArchPID = pgarch_start();
>
> In the latest patch, why did you remove the code to restart new archiver
> in reaper()? When archiver dies, I think new archiver should be restarted like
> the current reaper() does. Otherwise, the restart of archiver can be
> delayed until the next cycle of ServerLoop, which may take time.

Agreed. The code moved to the original place and added the crash
handling code. And I added a phrase to the comment.

+ * Was it the archiver? If exit status is zero (normal) or one (FATAL
+ * exit), we assume everything is all right just like normal backends
+ * and just try to restart a new one so that we immediately retry
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * archiving of remaining files. (If fail, we'll try again in future
~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> I read v50_003 patch.
>
> When archiver dies, ProcGlobal->archiverLatch should be reset to NULL,
> like walreceiver does the similar thing in WalRcvDie()?

Differently from walwriter and checkpointer, archiver as well as
walreceiver may die while server is running. Leaving the latch pointer
alone may lead to nudging a wrong process that took over the same
procarray slot. Added pgarch_die() to do that.

(I moved the archiverLatch to just after checkpointerLatch in this version.)

> In pgarch.c, #include "postmaster/fork_process.h" seems no longer necessary.

Right. That's not due to this patch, postmater.h, dsm.h and pg_shmem.h
are not used. (fd.h is not necessary but pgarch.c uses AllocateDir().)

> + if (strcmp(argv[1], "--forkarch") == 0)
> + {
>
> Why is this necessary? I was thinking that "--forkboot" handles archiver
> in SubPostmasterMain().

Yeah, the correspondent code is removed in the same patch at the same
time.

The attached is v51 patchset.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center