Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.

On 2021/03/25 21:23, Fujii Masao wrote:
> On 2021/03/25 9:58, Andres Freund wrote:
>> Outside of very
>> narrow circumstances a normal exit shouldn't use _exit(1). Neither 1 no
>> _exit() (as opposed to exit()) seem appropriate. This seems like a bad
>> idea.
>
> So you're thinking that the stats collector should do proc_exit(0)
> or something even when it receives immediate shutdown request?
>
> One idea to avoid using _exit(1) is to change the SIGQUIT handler
> so that it just sets the flag. Then if the stats collector detects that
> the flag is set in the main loop, it gets out of the loop,
> skips writing the permanent stats file and then exits with exit(0).
> That is, normal and immediate shutdown requests are treated
> almost the same way in the stats collector. Only the difference of
> them is whether it saves the stats to the file or not. Thought?

Although I don't oppose the idea to change from _exit(1) to proc_exit(0), the
reason why I used _exit(1) is that I thought the behavior to skip writing the
stat counters is not normal exit. Actually, other background processes use
_exit(2) instead of _exit(0) or proc_exit(0) in immediate shutdown although
the status code is different because they touch shared memory.

>> Also, won't this lead to postmaster now starting to complain about
>> pgstat having crashed in an immediate shutdown? Right now only exit
>> status 0 is expected:
>>
>>         if (pid == PgStatPID)
>>         {
>>             PgStatPID = 0;
>>             if (!EXIT_STATUS_0(exitstatus))
>>                 LogChildExit(LOG, _("statistics collector process"),
>>                              pid, exitstatus);
>>             if (pmState == PM_RUN || pmState == PM_HOT_STANDBY)
>>                 PgStatPID = pgstat_start();
>>             continue;
>>         }
>
> No. In immediate shutdown case, pmdie() sets "Shutdown" variable to
> ImmdiateShutdown and HandleChildCrash() doesn't complain that in that case
> because of the following.
>
>     /*
>      * We only log messages and send signals if this is the first process
>      * crash and we're not doing an immediate shutdown; otherwise, we're only
>      * here to update postmaster's idea of live processes.  If we have already
>      * signaled children, nonzero exit status is to be expected, so don't
>      * clutter log.
>      */
>     take_action = !FatalError && Shutdown != ImmediateShutdown;

IIUC, in immediate shutdown case (and other cases too?), HandleChildCrash() is
never invoked due to the exit of pgstat. My understanding of Andres-san's
comment is that is it ok to show like the following log message?

```
LOG: statistics collector process (PID 64991) exited with exit code 1
```

Surely, other processes don't output the log like it. So, I agree to suppress
the log message.

FWIW, in immediate shutdown case, since pmdie() sets "pmState" variable to
"PM_WAIT_BACKENDS", pgstat_start() won't be invoked.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION