Обсуждение: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
Hi,
This thread came from another thread about collecting the WAL
stats([1]).
Is it better to make the stats collector shutdown without writing the
stats files
if the immediate shutdown is requested?
There was a related discussion([2]) although it's stopped on 12/1/2016.
IIUC, the thread's consensus are
(1) It's useful to make the stats collector shutdown quickly without
writing the stats files
if the immediate shutdown is requested in some cases because there
is a possibility
that it takes a long time until the failover happens. The possible
reasons are that
disk write speed is slow, the stats files are big, and so on. And
there is no negative impact
on the users because all stats files are removed in a crash recovery
phase now.
(2) As another aspect, it needs to change the behavior removing all
stats files because autovacuum
uses the stats. There are some ideas, for example writing the stats
files every X minutes
(using wal or another mechanism) and even if a crash occurs, the
startup process can restore
the stats with slightly low freshness. But, it needs to find a way
how to handle the stats files
when deleting on PITR rewind or stats collector crash happens.
I agree that the above consensus and I think we can make separate two
patches.
The first one is for (1) and the second one is for (2).
Why don't you apply the patch for (1) first?
I attached the patch based on tsunakawa-san's patch([2]).
(v1-0001-pgstat_avoid_writing_on_sigquit.patch)
At this time, there are no cons points for the users because
the stats files are removed in a crash recovery phase as pointed in the
discussion.
[1]
https://www.postgresql.org/message-id/c616cf24bf4ecd818f7cab0900a40842%40oss.nttdata.com
[2]
https://www.postgresql.org/message-id/flat/0A3221C70F24FB45833433255569204D1F5EF25A%40G01JPEXMBYT05
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
Вложения
RE: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
От
"kuroda.hayato@fujitsu.com"
Дата:
Dear Ikeda-san, I confirmed new patch and no problem was found. Thanks. (I'm not a native English speaker, so I cannot check your comments correctly, sorry) Best Regards, Hayato Kuroda FUJITSU LIMITED
On 2021/03/18 11:59, kuroda.hayato@fujitsu.com wrote: > Dear Ikeda-san, > > I confirmed new patch and no problem was found. Thanks. > (I'm not a native English speaker, so I cannot check your comments correctly, sorry) One user-visible side-effect by this change is; with the patch, the stats is cleared when only the stats collector is killed (with SIGQUIT) accidentally and restarted by postmaster later. On the other than, currently the stats is written in that case and subsequently-restarted stats collector can use that stats file. I'm not sure if we need to keep supporting this behavior, though. When only the stats collector exits by SIGQUIT, with the patch FreeWaitEventSet() is also skipped. Is this ok? - * Loop to process messages until we get SIGQUIT or detect ungraceful - * death of our parent postmaster. + * Loop to process messages until we get SIGTERM or SIGQUIT of our parent + * postmaster. "SIGTERM or SIGQUIT of our parent postmaster" should be "SIGTERM, SIGQUIT, or detect ungraceful death of our parent postmaster"? +SignalHandlerForUnsafeExit(SIGNAL_ARGS) I don't think SignalHandlerForUnsafeExit is good name. Because that's not "unsafe" exit. No? Even after this signal handler is triggered, the server is still running normally and a process that exits will be restarted later. What about SignalHandlerForNonCrashExit or SignalHandlerForNonFatalExit? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 2021-03-18 13:37, Fujii Masao wrote:
> On 2021/03/18 11:59, kuroda.hayato@fujitsu.com wrote:
>> Dear Ikeda-san,
>>
>> I confirmed new patch and no problem was found. Thanks.
>> (I'm not a native English speaker, so I cannot check your comments
>> correctly, sorry)
>
> One user-visible side-effect by this change is; with the patch, the
> stats is
> cleared when only the stats collector is killed (with SIGQUIT)
> accidentally
> and restarted by postmaster later.
Thanks for your comments.
As you said, the temporary stats files don't removed if the stats
collector is killed with SIGQUIT.
So, if the user change the GUC parameter "stats_temp_directory" after
immediate shutdown,
temporary stats file can't be removed forever.
But, I think this case is rarely happened and unimportant. Actually,
pgstat_write_statsfiles()
didn't check error of unlink() and the same problem is occurred if the
server is crashed now.
The documentation said following. I think it's enough.
```
For better performance, <varname>stats_temp_directory</varname> can
be
pointed at a RAM-based file system, decreasing physical I/O
requirements.
When the server shuts down cleanly, a permanent copy of the
statistics
data is stored in the <filename>pg_stat</filename> subdirectory, so
that
statistics can be retained across server restarts. When recovery is
performed at server start (e.g., after immediate shutdown, server
crash,
and point-in-time recovery), all statistics counters are reset.
```
> On the other than, currently the stats is
> written in that case and subsequently-restarted stats collector can use
> that stats file. I'm not sure if we need to keep supporting this
> behavior, though.
I don't have any strong opinion this behaivor is useless too.
Since the reinitialized phase is not executed when only the stats
collector is crashed
(since it didn't touch the shared memory), if the permanent stats file
is exists, the
stats collector can use it. But, IIUC, the case is rare.
The case is happened by operation mistake which a operator sends the
SIGQUIT signal to
the stats collector. Please let me know if there are more important
case.
But, if SIGKILL is sent by operator, the stats can't be rescure now
because the permanent stats
files can't be written before exiting. Since the case which can rescure
the stats is rare,
I think it's ok to initialize the stats even if SIGQUIT is sent.
If to support this feature, we need to implement the following first.
> (2) As another aspect, it needs to change the behavior removing all
> stats files because autovacuum
> uses the stats. There are some ideas, for example writing the stats
> files every X minutes
> (using wal or another mechanism) and even if a crash occurs, the
> startup process can restore
> the stats with slightly low freshness. But, it needs to find a way
> how to handle the stats files
> when deleting on PITR rewind or stats collector crash happens.
> When only the stats collector exits by SIGQUIT, with the patch
> FreeWaitEventSet() is also skipped. Is this ok?
Thanks, I fixed it.
> - * Loop to process messages until we get SIGQUIT or detect ungraceful
> - * death of our parent postmaster.
> + * Loop to process messages until we get SIGTERM or SIGQUIT of our
> parent
> + * postmaster.
>
> "SIGTERM or SIGQUIT of our parent postmaster" should be
> "SIGTERM, SIGQUIT, or detect ungraceful death of our parent
> postmaster"?
Yes, I fixed it.
> +SignalHandlerForUnsafeExit(SIGNAL_ARGS)
>
> I don't think SignalHandlerForUnsafeExit is good name. Because that's
> not
> "unsafe" exit. No? Even after this signal handler is triggered, the
> server is
> still running normally and a process that exits will be restarted
> later. What
> about SignalHandlerForNonCrashExit or SignalHandlerForNonFatalExit?
OK, I fixed.
I changed to the SignalPgstatHandlerForNonCrashExit() to add
FreeWaitEventSet()
in the handler for the stats collector.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
Вложения
On 2021/03/18 19:16, Masahiro Ikeda wrote: > As you said, the temporary stats files don't removed if the stats collector is killed with SIGQUIT. > So, if the user change the GUC parameter "stats_temp_directory" after immediate shutdown, > temporary stats file can't be removed forever. > > But, I think this case is rarely happened and unimportant. Actually, pgstat_write_statsfiles() > didn't check error of unlink() and the same problem is occurred if the server is crashed now. > The documentation said following. I think it's enough. Thanks for considering this! Understood. > I don't have any strong opinion this behaivor is useless too. > > Since the reinitialized phase is not executed when only the stats collector is crashed > (since it didn't touch the shared memory), if the permanent stats file is exists, the > stats collector can use it. But, IIUC, the case is rare. > > The case is happened by operation mistake which a operator sends the SIGQUIT signal to > the stats collector. Please let me know if there are more important case. > > But, if SIGKILL is sent by operator, the stats can't be rescure now because the permanent stats > files can't be written before exiting. Since the case which can rescure the stats is rare, > I think it's ok to initialize the stats even if SIGQUIT is sent. Sounds reasonable. >> When only the stats collector exits by SIGQUIT, with the patch >> FreeWaitEventSet() is also skipped. Is this ok? > > Thanks, I fixed it. I'm still not sure if FreeWaitEventSet() is actually necessary in that exit case. Could you tell me why you thought FreeWaitEventSet() is necessary in the case? Since it's not good to do many things in a signal handler, even when FreeWaitEventSet() is really necessary, it should be called at subsequent startup of the stats collector instead of calling it in the handler at the end? BTW, I found bgworker calls FreeWaitEventSet() via ShutdownLatchSupport() at its startup. But I'm also not sure if this is really necessary at the startup... Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
>>> When only the stats collector exits by SIGQUIT, with the patch >>> FreeWaitEventSet() is also skipped. Is this ok? >> >> Thanks, I fixed it. > > I'm still not sure if FreeWaitEventSet() is actually necessary in that > exit case. Could you tell me why you thought FreeWaitEventSet() is > necessary in the case? Sorry, there is no evidence we should call it. I thought that to call FreeWaitEventSet() is a manner after using CreateWaitEventSet() and the performance impact to call it seems to be too small. (Please let me know if my understanding is wrong.) The reason why I said this is a manner because I thought it's no problem even without calling FreeWaitEventSet() before the process exits regardless of fast or immediate shutdown. Since the "WaitEventSet *wes" is a process local resource, this will be released by OS even if FreeWaitEventSet() is not called. I'm now changing my mind to skip it is better because the code can be simpler and, it's unimportant for the above reason especially when the immediate shutdown is requested which is a bad manner itself. BTW, the SysLoggerMain() create the WaitEventSet, but it didn't call FreeWaitEventSet(). Is it better to call FreeWaitEventSet() before exiting too? > Since it's not good to do many things in a signal handler, even when > FreeWaitEventSet() is really necessary, it should be called at > subsequent > startup of the stats collector instead of calling it in the handler > at the end? BTW, I found bgworker calls FreeWaitEventSet() via > ShutdownLatchSupport() at its startup. But I'm also not sure if this > is really necessary at the startup... OK, I understood that I need to change the signal handler's implementation if FreeWaitEventSet() is necessary. In my understanding from the following commit message, the ordinary bgworker which not access the shared memory doesn't use the latch which postmaster installed. So, ShutdownLatchSupport() is called at the startup. Though? 2021/3/1 commit: 83709a0d5a46559db016c50ded1a95fd3b0d3be6 ``` The signal handler is now installed in all postmaster children by InitializeLatchSupport(). Those wishing to disconnect from it should call ShutdownLatchSupport(). ``` Regards, -- Masahiro Ikeda NTT DATA CORPORATION
On 2021/03/20 13:40, Masahiro Ikeda wrote: > Sorry, there is no evidence we should call it. > I thought that to call FreeWaitEventSet() is a manner after using > CreateWaitEventSet() and the performance impact to call it seems to be too small. > > (Please let me know if my understanding is wrong.) > The reason why I said this is a manner because I thought it's no problem > even without calling FreeWaitEventSet() before the process exits regardless > of fast or immediate shutdown. Since the "WaitEventSet *wes" is a process local resource, > this will be released by OS even if FreeWaitEventSet() is not called. > > I'm now changing my mind to skip it is better because the code can be simpler and, > it's unimportant for the above reason especially when the immediate shutdown is > requested which is a bad manner itself. Thanks for explaining this! So you're thinking that v3 patch should be chosen? Probably some review comments I posted upthread need to be applied to v3 patch, e.g., rename of SignalHandlerForUnsafeExit() function. > BTW, the SysLoggerMain() create the WaitEventSet, but it didn't call FreeWaitEventSet(). > Is it better to call FreeWaitEventSet() before exiting too? Yes if which could cause actual issue. Otherwise I don't have strong reason to do that for now.. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 2021/03/22 23:59, Fujii Masao wrote: > > > On 2021/03/20 13:40, Masahiro Ikeda wrote: >> Sorry, there is no evidence we should call it. >> I thought that to call FreeWaitEventSet() is a manner after using >> CreateWaitEventSet() and the performance impact to call it seems to be too >> small. >> >> (Please let me know if my understanding is wrong.) >> The reason why I said this is a manner because I thought it's no problem >> even without calling FreeWaitEventSet() before the process exits regardless >> of fast or immediate shutdown. Since the "WaitEventSet *wes" is a process >> local resource, >> this will be released by OS even if FreeWaitEventSet() is not called. >> >> I'm now changing my mind to skip it is better because the code can be >> simpler and, >> it's unimportant for the above reason especially when the immediate shutdown is >> requested which is a bad manner itself. > > Thanks for explaining this! So you're thinking that v3 patch should be chosen? > Probably some review comments I posted upthread need to be applied to > v3 patch, e.g., rename of SignalHandlerForUnsafeExit() function. Yes. I attached the v5 patch based on v3 patch. I renamed SignalHandlerForUnsafeExit() and fixed the following comment. > "SIGTERM or SIGQUIT of our parent postmaster" should be > "SIGTERM, SIGQUIT, or detect ungraceful death of our parent > postmaster"? >> BTW, the SysLoggerMain() create the WaitEventSet, but it didn't call >> FreeWaitEventSet(). >> Is it better to call FreeWaitEventSet() before exiting too? > > Yes if which could cause actual issue. Otherwise I don't have strong > reason to do that for now.. OK. AFAIK, this doesn't lead critical problems like memory leak. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Вложения
On 2021/03/23 9:05, Masahiro Ikeda wrote: > Yes. I attached the v5 patch based on v3 patch. > I renamed SignalHandlerForUnsafeExit() and fixed the following comment. Thanks for updating the patch! When the startup process exits because of recovery_target_action=shutdown, reaper() calls TerminateChildren(SIGTERM). This function sends SIGTERM to the stats collector. Currently the stats collector ignores SIGTERM, but with the patch it exits normally. This change of behavior might be problematic. That is, TerminateChildren(SIGTERM) sends SIGTERM to various processes. But currently the stats collector and checkpointer don't exit even when SIGTERM arrives because they ignore SIGTERM. After several processes other than the stats collector and checkpointer exit by SIGTERM, PostmasterStateMachine() and reaper() make checkpointer exit and then the stats collector exit. The shutdown terminates the processes in this order. On the other hand, with the patch, the stats collector exits by SIGTERM before checkpointer exits. This is not normal order of processes to exit in shutdown. To address this issue, one idea is to use SIGUSR2 for normal exit of the stats collector, instead of SIGTERM. If we do this, TerminateChildren(SIGTERM) cannot terminate the stats collector. Thought? If we adopt this idea, the detail comment about why SIGUSR2 is used for that needs to be added. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 2021/03/23 11:40, Fujii Masao wrote:
>
>
> On 2021/03/23 9:05, Masahiro Ikeda wrote:
>> Yes. I attached the v5 patch based on v3 patch.
>> I renamed SignalHandlerForUnsafeExit() and fixed the following comment.
>
> Thanks for updating the patch!
>
> When the startup process exits because of recovery_target_action=shutdown,
> reaper() calls TerminateChildren(SIGTERM). This function sends SIGTERM to
> the stats collector. Currently the stats collector ignores SIGTERM, but with
> the patch it exits normally. This change of behavior might be problematic.
>
> That is, TerminateChildren(SIGTERM) sends SIGTERM to various processes.
> But currently the stats collector and checkpointer don't exit even when
> SIGTERM arrives because they ignore SIGTERM. After several processes
> other than the stats collector and checkpointer exit by SIGTERM,
> PostmasterStateMachine() and reaper() make checkpointer exit and then
> the stats collector exit. The shutdown terminates the processes in this order.
>
> On the other hand, with the patch, the stats collector exits by SIGTERM
> before checkpointer exits. This is not normal order of processes to exit in
> shutdown.
>
> To address this issue, one idea is to use SIGUSR2 for normal exit of the stats
> collector, instead of SIGTERM. If we do this, TerminateChildren(SIGTERM)
> cannot terminate the stats collector. Thought?
>
> If we adopt this idea, the detail comment about why SIGUSR2 is used for that
> needs to be added.
Thanks for your comments. I agreed your concern and suggestion.
Additionally, we need to consider system shutdown cycle too as
CheckpointerMain()'s comment said.
```
* Note: we deliberately ignore SIGTERM, because during a standard Unix
* system shutdown cycle, init will SIGTERM all processes at once. We
* want to wait for the backends to exit, whereupon the postmaster will
* tell us it's okay to shut down (via SIGUSR2)
```
I changed the signal from SIGTERM to SIGUSR2 and add the comments why SIGUSR2
is used.
(v6-0001-pgstat_avoid_writing_on_sigquit.patch)
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
Вложения
On 2021/03/23 14:54, Masahiro Ikeda wrote: > Thanks for your comments. I agreed your concern and suggestion. > Additionally, we need to consider system shutdown cycle too as > CheckpointerMain()'s comment said. > > ``` > * Note: we deliberately ignore SIGTERM, because during a standard Unix > * system shutdown cycle, init will SIGTERM all processes at once. We > * want to wait for the backends to exit, whereupon the postmaster will > * tell us it's okay to shut down (via SIGUSR2) > ``` Good catch! > I changed the signal from SIGTERM to SIGUSR2 and add the comments why SIGUSR2 > is used. > (v6-0001-pgstat_avoid_writing_on_sigquit.patch) Thanks for updating the patch! + * The statistics collector is started by the postmaster as soon as the + * startup subprocess finishes. This comment needs to be updated? Because the stats collector can be invoked when the startup process sends PMSIGNAL_BEGIN_HOT_STANDBY signal. This fact makes me wonder that if we collect the statistics about WAL writing from walreceiver as we discussed in other thread, the stats collector should be invoked at more earlier stage. IIUC walreceiver can be invoked before PMSIGNAL_BEGIN_HOT_STANDBY is sent. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Hi, On 2021-03-23 15:50:46 +0900, Fujii Masao wrote: > This fact makes me wonder that if we collect the statistics about WAL writing > from walreceiver as we discussed in other thread, the stats collector should > be invoked at more earlier stage. IIUC walreceiver can be invoked before > PMSIGNAL_BEGIN_HOT_STANDBY is sent. FWIW, in the shared memory stats patch the stats subsystem is initialized early on by the startup process. Greetings, Andres Freund
On 2021/03/23 15:50, Fujii Masao wrote: > + * The statistics collector is started by the postmaster as soon as the > + * startup subprocess finishes. > > This comment needs to be updated? Because the stats collector can > be invoked when the startup process sends PMSIGNAL_BEGIN_HOT_STANDBY > signal. I updated this comment in the patch. Attached is the updated version of the patch. Barring any objection, I'm thinking to commit this patch. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Вложения
On 2021/03/24 3:51, Andres Freund wrote: > Hi, > > On 2021-03-23 15:50:46 +0900, Fujii Masao wrote: >> This fact makes me wonder that if we collect the statistics about WAL writing >> from walreceiver as we discussed in other thread, the stats collector should >> be invoked at more earlier stage. IIUC walreceiver can be invoked before >> PMSIGNAL_BEGIN_HOT_STANDBY is sent. > > FWIW, in the shared memory stats patch the stats subsystem is > initialized early on by the startup process. This is good news! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 2021/03/24 18:36, Fujii Masao wrote: > > > On 2021/03/24 3:51, Andres Freund wrote: >> Hi, >> >> On 2021-03-23 15:50:46 +0900, Fujii Masao wrote: >>> This fact makes me wonder that if we collect the statistics about WAL writing >>> from walreceiver as we discussed in other thread, the stats collector should >>> be invoked at more earlier stage. IIUC walreceiver can be invoked before >>> PMSIGNAL_BEGIN_HOT_STANDBY is sent. >> >> FWIW, in the shared memory stats patch the stats subsystem is >> initialized early on by the startup process. > > This is good news! Fujii-san, Andres-san, Thanks for your comments! I didn't think about the start order. From the point of view, I noticed that the current source code has two other concerns. 1. This problem is not only for the wal receiver. The problem which the wal receiver starts before the stats collector is launched during archive recovery is not only for the the wal receiver but also the checkpointer and the bgwriter. Before starting redo, the startup process sends the postmaster "PMSIGNAL_RECOVERY_STARTED" signal to launch the checkpointer and the bgwriter to be able to perform creating restartpoint. Although the socket for communication between the stats collector and the other processes is made in earlier stage via pgstat_init(), I agree to make the stats collector starts earlier stage is defensive. BTW, in my environments(linux, net.core.rmem_default = 212992), the socket can buffer almost 300 WAL stats messages. This mean that, as you said, if the redo phase is too long, it can lost the messages easily. 2. To make the stats clear in redo phase. The statistics can be reset after the wal receiver, the checkpointer and the wal writer are started in redo phase. So, it's not enough the stats collector is invoked at more earlier stage. We need to fix it. (I hope I am not missing something.) Thanks to Andres-san's work([1]), the above problems will be handle in the shared memory stats patch. First problem will be resolved since the stats are collected in shared memory, so the stats collector process is unnecessary itself. Second problem will be resolved to remove the reset code because the temporary stats file won't generated, and if the permanent stats file corrupted, just recreate it. [1] https://github.com/anarazel/postgres/compare/master...shmstat-before-split-2021-03-22 Regards, -- Masahiro Ikeda NTT DATA CORPORATION
On 2021/03/24 18:36, Fujii Masao wrote: > On 2021/03/23 15:50, Fujii Masao wrote: >> + * The statistics collector is started by the postmaster as soon as the >> + * startup subprocess finishes. >> >> This comment needs to be updated? Because the stats collector can >> be invoked when the startup process sends PMSIGNAL_BEGIN_HOT_STANDBY >> signal. > > I updated this comment in the patch. > Attached is the updated version of the patch. > > Barring any objection, I'm thinking to commit this patch. Thanks for your comments and updating the patch! I checked your patch and I don't have any comments. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Hi,
On 2021-03-24 18:36:14 +0900, Fujii Masao wrote:
> Barring any objection, I'm thinking to commit this patch.
To which branches? I am *strongly* opposed to backpatching this.
> /*
> - * Simple signal handler for exiting quickly as if due to a crash.
> + * Simple signal handler for processes which have not yet touched or do not
> + * touch shared memory to exit quickly.
> *
> - * Normally, this would be used for handling SIGQUIT.
> + * Note that if processes already touched shared memory, use
> + * SignalHandlerForCrashExit() instead and force the postmaster into
> + * a system reset cycle because shared memory may be corrupted.
> + */
> +void
> +SignalHandlerForNonCrashExit(SIGNAL_ARGS)
> +{
> + /*
> + * Since we don't touch shared memory, we can just pull the plug and exit
> + * without running any atexit handlers.
> + */
> + _exit(1);
> +}
This strikes me as a quite a misleading function name. 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.
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;
}
> + * The statistics collector is started by the postmaster as soon as the
> + * startup subprocess finishes, or as soon as the postmaster is ready
> + * to accept read only connections during archive recovery. It remains
> + * alive until the postmaster commands it to terminate. Normal
> + * termination is by SIGUSR2 after the checkpointer must exit(0),
> + * which instructs the statistics collector to save the final statistics
> + * to reuse at next startup and then exit(0).
I can't parse "...after the checkpointer must exit(0), which instructs..."
> + * Emergency termination is by SIGQUIT; like any backend, the statistics
> + * collector will exit quickly without saving the final statistics. It's
> + * ok because the startup process will remove all statistics at next
> + * startup after emergency termination.
Normally there won't be any stats to remove, no? The permanent stats
file has been removed when the stats collector started up.
Greetings,
Andres Freund
On 2021/03/25 9:31, Masahiro Ikeda wrote: > > > On 2021/03/24 18:36, Fujii Masao wrote: >> >> >> On 2021/03/24 3:51, Andres Freund wrote: >>> Hi, >>> >>> On 2021-03-23 15:50:46 +0900, Fujii Masao wrote: >>>> This fact makes me wonder that if we collect the statistics about WAL writing >>>> from walreceiver as we discussed in other thread, the stats collector should >>>> be invoked at more earlier stage. IIUC walreceiver can be invoked before >>>> PMSIGNAL_BEGIN_HOT_STANDBY is sent. >>> >>> FWIW, in the shared memory stats patch the stats subsystem is >>> initialized early on by the startup process. >> >> This is good news! > > Fujii-san, Andres-san, > Thanks for your comments! > > I didn't think about the start order. From the point of view, I noticed that > the current source code has two other concerns. > > > 1. This problem is not only for the wal receiver. > > The problem which the wal receiver starts before the stats collector > is launched during archive recovery is not only for the the wal receiver but > also the checkpointer and the bgwriter. Before starting redo, the startup > process sends the postmaster "PMSIGNAL_RECOVERY_STARTED" signal to launch the > checkpointer and the bgwriter to be able to perform creating restartpoint. > > Although the socket for communication between the stats collector and the > other processes is made in earlier stage via pgstat_init(), I agree to make > the stats collector starts earlier stage is defensive. BTW, in my > environments(linux, net.core.rmem_default = 212992), the socket can buffer > almost 300 WAL stats messages. This mean that, as you said, if the redo phase > is too long, it can lost the messages easily. > > > 2. To make the stats clear in redo phase. > > The statistics can be reset after the wal receiver, the checkpointer and > the wal writer are started in redo phase. So, it's not enough the stats > collector is invoked at more earlier stage. We need to fix it. > > > > (I hope I am not missing something.) > Thanks to Andres-san's work([1]), the above problems will be handle in the > shared memory stats patch. First problem will be resolved since the stats are > collected in shared memory, so the stats collector process is unnecessary > itself. Second problem will be resolved to remove the reset code because the > temporary stats file won't generated, and if the permanent stats file > corrupted, just recreate it. Yes. So we should wait for the shared memory stats patch to be committed before working on walreceiver stats patch more? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 2021/03/25 9:58, Andres Freund wrote:
> Hi,
>
> On 2021-03-24 18:36:14 +0900, Fujii Masao wrote:
>> Barring any objection, I'm thinking to commit this patch.
>
> To which branches?
I was thinking to push the patch to the master branch
because this is not a bug fix.
> I am *strongly* opposed to backpatching this.
+1
> This strikes me as a quite a misleading function name.
Yeah, better name is always welcome :)
> 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?
> 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;
>> + * The statistics collector is started by the postmaster as soon as the
>> + * startup subprocess finishes, or as soon as the postmaster is ready
>> + * to accept read only connections during archive recovery. It remains
>> + * alive until the postmaster commands it to terminate. Normal
>> + * termination is by SIGUSR2 after the checkpointer must exit(0),
>> + * which instructs the statistics collector to save the final statistics
>> + * to reuse at next startup and then exit(0).
>
> I can't parse "...after the checkpointer must exit(0), which instructs..."
What about changing that to the following?
------------------------
Normal termination is requested after checkpointer exits. It's by SIGUSR2,
which instructs the statistics collector to save the final statistics and
then exit(0).
------------------------
>> + * Emergency termination is by SIGQUIT; like any backend, the statistics
>> + * collector will exit quickly without saving the final statistics. It's
>> + * ok because the startup process will remove all statistics at next
>> + * startup after emergency termination.
>
> Normally there won't be any stats to remove, no? The permanent stats
> file has been removed when the stats collector started up.
Yes. In normal case, when the stats collector starts up, it loads the stats
from the file and remove the file. OTOH, when the recovery is necessary,
instead the startup process calls pgstat_reset_all() and removes the stats files.
You're thinking that the above comments are confusing?
If so, what about the following?
----------------------
Emergency termination is by SIGQUIT; the statistics collector
will exit quickly without saving the final statistics. In this case
the statistics files don't need to be written because the next startup
will trigger a crash recovery and remove all statistics files forcibly
even if they exist.
----------------------
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
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
On 2021/03/25 19:48, Fujii Masao wrote: > > > On 2021/03/25 9:31, Masahiro Ikeda wrote: >> >> >> On 2021/03/24 18:36, Fujii Masao wrote: >>> >>> >>> On 2021/03/24 3:51, Andres Freund wrote: >>>> Hi, >>>> >>>> On 2021-03-23 15:50:46 +0900, Fujii Masao wrote: >>>>> This fact makes me wonder that if we collect the statistics about WAL >>>>> writing >>>>> from walreceiver as we discussed in other thread, the stats collector should >>>>> be invoked at more earlier stage. IIUC walreceiver can be invoked before >>>>> PMSIGNAL_BEGIN_HOT_STANDBY is sent. >>>> >>>> FWIW, in the shared memory stats patch the stats subsystem is >>>> initialized early on by the startup process. >>> >>> This is good news! >> >> Fujii-san, Andres-san, >> Thanks for your comments! >> >> I didn't think about the start order. From the point of view, I noticed that >> the current source code has two other concerns. >> >> >> 1. This problem is not only for the wal receiver. >> >> The problem which the wal receiver starts before the stats collector >> is launched during archive recovery is not only for the the wal receiver but >> also the checkpointer and the bgwriter. Before starting redo, the startup >> process sends the postmaster "PMSIGNAL_RECOVERY_STARTED" signal to launch the >> checkpointer and the bgwriter to be able to perform creating restartpoint. >> >> Although the socket for communication between the stats collector and the >> other processes is made in earlier stage via pgstat_init(), I agree to make >> the stats collector starts earlier stage is defensive. BTW, in my >> environments(linux, net.core.rmem_default = 212992), the socket can buffer >> almost 300 WAL stats messages. This mean that, as you said, if the redo phase >> is too long, it can lost the messages easily. >> >> >> 2. To make the stats clear in redo phase. >> >> The statistics can be reset after the wal receiver, the checkpointer and >> the wal writer are started in redo phase. So, it's not enough the stats >> collector is invoked at more earlier stage. We need to fix it. >> >> >> >> (I hope I am not missing something.) >> Thanks to Andres-san's work([1]), the above problems will be handle in the >> shared memory stats patch. First problem will be resolved since the stats are >> collected in shared memory, so the stats collector process is unnecessary >> itself. Second problem will be resolved to remove the reset code because the >> temporary stats file won't generated, and if the permanent stats file >> corrupted, just recreate it. > > Yes. So we should wait for the shared memory stats patch to be committed > before working on walreceiver stats patch more? Yes, I agree. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
On 2021/03/26 9:25, Masahiro Ikeda wrote:
>
> 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.
True.
>
>
>>> 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.
Yes. I was mistakenly thinking that HandleChildCrash() was called
even when the stats collector exits with non-zero code, like other processes.
But that's not true.
How should we do this? HandleChildCrash() calls LogChildExit()
when FatalError = false and Shutdown != ImmediateShutdown.
We should use the same conditions for the stats collector as follows?
if (pid == PgStatPID)
{
PgStatPID = 0;
- if (!EXIT_STATUS_0(exitstatus))
+ if (!EXIT_STATUS_0(exitstatus) && !FatalError &&
+ Shutdown != ImmediateShutdown)
LogChildExit(LOG, _("statistics collector process"),
pid, exitstatus);
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2021/03/26 9:54, Fujii Masao wrote:
> On 2021/03/26 9:25, Masahiro Ikeda wrote:
>> On 2021/03/25 21:23, Fujii Masao wrote:
>>> On 2021/03/25 9:58, Andres Freund wrote:
>>>> 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.
>
> Yes. I was mistakenly thinking that HandleChildCrash() was called
> even when the stats collector exits with non-zero code, like other processes.
> But that's not true.
>
> How should we do this? HandleChildCrash() calls LogChildExit()
> when FatalError = false and Shutdown != ImmediateShutdown.
> We should use the same conditions for the stats collector as follows?
>
> if (pid == PgStatPID)
> {
> PgStatPID = 0;
> - if (!EXIT_STATUS_0(exitstatus))
> + if (!EXIT_STATUS_0(exitstatus) && !FatalError &&
> + Shutdown != ImmediateShutdown)
> LogChildExit(LOG, _("statistics collector
> process"),
> pid, exitstatus);
Thanks, I agree the above code if it's ok that the exit status of the stats
collector is not 0 in immediate shutdown case.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
Hi, On 2021-03-25 21:23:17 +0900, Fujii Masao wrote: > > This strikes me as a quite a misleading function name. > > Yeah, better name is always welcome :) It might just be best to not introduce a generic function and just open code one just for the stats collector... > > 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? My main complaint isn't so much that you made the stats collector _exit(1). It's that that you added a function that sounded generic, but should basically not be used anywhere (we have very few non-shmem connected processes left - I don't think that number will increase). Greetings, Andres Freund
Hi, On 2021-03-26 09:27:19 +0900, Masahiro Ikeda wrote: > On 2021/03/25 19:48, Fujii Masao wrote: > > Yes. So we should wait for the shared memory stats patch to be committed > > before working on walreceiver stats patch more? > > Yes, I agree. Agreed. One thing that I didn't quite see discussed enough in the thread so far: Have you considered what regressions the stats file removal in immediate shutdowns could cause? Right now we will - kind of accidentally - keep the stats file for immediate shutdowns, which means that autovacuum etc will still have stats to work with after the next start. After this, not so much? Greetings, Andres Freund
On 2021/03/27 2:14, Andres Freund wrote: > Hi, > > On 2021-03-26 09:27:19 +0900, Masahiro Ikeda wrote: >> On 2021/03/25 19:48, Fujii Masao wrote: >>> Yes. So we should wait for the shared memory stats patch to be >>> committed before working on walreceiver stats patch more? >> >> Yes, I agree. > > Agreed. OK. And I withdraw this thread since the shared memory stats patch will solve this problem too. > One thing that I didn't quite see discussed enough in the thread so far: > Have you considered what regressions the stats file removal in immediate > shutdowns could cause? Right now we will - kind of accidentally - keep the > stats file for immediate shutdowns, which means that autovacuum etc will > still have stats to work with after the next start. After this, not so > much? Yes([1]). Although we keep the stats file for immediate shutdowns, IIUC, we'll remove them in redo phase via pgstat_reset_all() anyway. So, this means that autovaccum etc can't use the stats after the next start. FWIW, the issue is discussed([2]). The consensus is that we need to change the behavior removing all stats files even if server crash is occurred because autovacuum uses the stats. There are some ideas, for example writing the stats files every X minutes (using wal or another mechanism). Then, the startup process can restore the stats with slightly low freshness even if a crash occurs. But, it needs to find a way how to handle the stats files when deleting on PITR rewind. This is not solved yet. If my understanding is correct, the shared memory stats patch didn't consider to handle the issue yet, but to solve it makes the patch more complicated... I think it's better to work on the issue after the base patch of the shared memory stats is committed. [1] https://www.postgresql.org/message-id/c96d8989100e4bce4fa586064aa7e0e9%40oss.nttdata.com [2] https://www.postgresql.org/message-id/flat/0A3221C70F24FB45833433255569204D1F5EF25A%40G01JPEXMBYT05 Regards, -- Masahiro Ikeda NTT DATA CORPORATION