Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
diff --git a/src/backend/postmaster/interrupt.c
b/src/backend/postmaster/interrupt.c
index dd9136a942..d1b1f95400 100644
--- a/src/backend/postmaster/interrupt.c
+++ b/src/backend/postmaster/interrupt.c
@@ -64,9 +64,28 @@ SignalHandlerForConfigReload(SIGNAL_ARGS)
}
/*
- * 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);
+}
+
+/*
+ * Simple signal handler for processes which have touched shared memory to
+ * exit quickly.
+ *
+ * Normally, this would be used for handling SIGQUIT as if due to a crash.
*/
void
SignalHandlerForCrashExit(SIGNAL_ARGS)
@@ -93,9 +112,8 @@ SignalHandlerForCrashExit(SIGNAL_ARGS)
* shut down and exit.
*
* Typically, this handler would be used for SIGTERM, but some processes use
- * other signals. In particular, the checkpointer exits on SIGUSR2, the
- * stats collector on SIGQUIT, and the WAL writer exits on either SIGINT
- * or SIGTERM.
+ * other signals. In particular, the checkpointer and the stats collector exit
+ * on SIGUSR2, and the WAL writer exits on either SIGINT or SIGTERM.
*
* ShutdownRequestPending should be checked at a convenient place within the
* main loop, or else the main loop should call HandleMainLoopInterrupts.
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 60f45ccc4e..fd0af0f289 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -1,7 +1,23 @@
/* --
* pgstat.c
*
- * All the statistics collector stuff hacked up in one big, ugly file.
+ * All the statistics collector stuff hacked up in one big, ugly file.
+ *
+ * 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).
+ * 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.
+ *
+ * Because the statistics collector doesn't touch shared memory, even if
+ * the statistics collector exits unexpectedly, the postmaster doesn't
+ * treat it as a crash. The postmaster will just try to restart a new one.
*
* TODO: - Separate collector, postmaster and backend stuff
* into different files.
@@ -724,6 +740,7 @@ pgstat_reset_remove_files(const char *directory)
snprintf(fname, sizeof(fname), "%s/%s", directory,
entry->d_name);
+ elog(DEBUG2, "removing stats file \"%s\"", fname);
unlink(fname);
}
FreeDir(dir);
@@ -4821,17 +4838,31 @@ PgstatCollectorMain(int argc, char *argv[])
/*
* Ignore all signals usually bound to some action in the postmaster,
-* except SIGHUP and SIGQUIT. Note we don't need a SIGUSR1 handler to
-* support latch operations, because we only use a local latch.
+* except SIGHUP, SIGQUIT and SIGUSR2. Note we don't need a SIGUSR1
+* handler to support latch operations, because we only use a local
latch.
+*
+* We deliberately ignore SIGTERM and exit in correct order because we
+* want to collect the stats sent during the shutdown from all
processes.
+* SIGTERM will be received during a standard Unix system shutdown cycle
+* because init will SIGTERM all processes at once, and the postmaster
+* will SIGTERM all
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
diff --git a/src/backend/postmaster/interrupt.c b/src/backend/postmaster/interrupt.c
index dd9136a942..e09a7b3cad 100644
--- a/src/backend/postmaster/interrupt.c
+++ b/src/backend/postmaster/interrupt.c
@@ -64,9 +64,27 @@ SignalHandlerForConfigReload(SIGNAL_ARGS)
}
/*
- * 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.
+ * If processes already touched shared memory, call
+ * SignalHandlerForCrashExit() 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);
+}
+
+/*
+ * Simple signal handler for processes which have touched shared memory to
+ * exit quickly.
+ *
+ * Normally, this would be used for handling SIGQUIT as if due to a crash.
*/
void
SignalHandlerForCrashExit(SIGNAL_ARGS)
@@ -93,9 +111,8 @@ SignalHandlerForCrashExit(SIGNAL_ARGS)
* shut down and exit.
*
* Typically, this handler would be used for SIGTERM, but some processes use
- * other signals. In particular, the checkpointer exits on SIGUSR2, the
- * stats collector on SIGQUIT, and the WAL writer exits on either SIGINT
- * or SIGTERM.
+ * other signals. In particular, the checkpointer and the stats collector exit
+ * on SIGUSR2, and the WAL writer exits on either SIGINT or SIGTERM.
*
* ShutdownRequestPending should be checked at a convenient place within the
* main loop, or else the main loop should call HandleMainLoopInterrupts.
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index b7af7c2707..53b84eda56 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -1,7 +1,21 @@
/* --
* pgstat.c
*
- * All the statistics collector stuff hacked up in one big, ugly file.
+ * All the statistics collector stuff hacked up in one big, ugly file.
+ *
+ * The statistics collector is started by the postmaster as soon as the
+ * startup subprocess finishes. 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).
+ * 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.
+ *
+ * Because the statistics collector doesn't touch shared memory, even if
+ * the statistics collector exits unexpectedly, the
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
diff --git a/src/backend/postmaster/interrupt.c b/src/backend/postmaster/interrupt.c
index dd9136a942..8621212eda 100644
--- a/src/backend/postmaster/interrupt.c
+++ b/src/backend/postmaster/interrupt.c
@@ -64,9 +64,27 @@ SignalHandlerForConfigReload(SIGNAL_ARGS)
}
/*
- * 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.
+ * If processes already touched shared memory, call
+ * SignalHandlerForCrashExit() 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);
+}
+
+/*
+ * Simple signal handler for processes which have touched shared memory to
+ * exit quickly.
+ *
+ * Normally, this would be used for handling SIGQUIT as if due to a crash.
*/
void
SignalHandlerForCrashExit(SIGNAL_ARGS)
@@ -93,9 +111,8 @@ SignalHandlerForCrashExit(SIGNAL_ARGS)
* shut down and exit.
*
* Typically, this handler would be used for SIGTERM, but some processes use
- * other signals. In particular, the checkpointer exits on SIGUSR2, the
- * stats collector on SIGQUIT, and the WAL writer exits on either SIGINT
- * or SIGTERM.
+ * other signals. In particular, the checkpointer exits on SIGUSR2 and the
+ * WAL writer exits on either SIGINT or SIGTERM.
*
* ShutdownRequestPending should be checked at a convenient place within the
* main loop, or else the main loop should call HandleMainLoopInterrupts.
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 208a33692f..44c471937b 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -724,6 +724,7 @@ pgstat_reset_remove_files(const char *directory)
snprintf(fname, sizeof(fname), "%s/%s", directory,
entry->d_name);
+ elog(DEBUG2, "removing stats file \"%s\"", fname);
unlink(fname);
}
FreeDir(dir);
@@ -4821,13 +4822,19 @@ PgstatCollectorMain(int argc, char *argv[])
/*
* Ignore all signals usually bound to some action in the postmaster,
- * except SIGHUP and SIGQUIT. Note we don't need a SIGUSR1 handler to
- * support latch operations, because we only use a local latch.
+ * except SIGHUP, SIGTERM and SIGQUIT. Note we don't need a SIGUSR1
+ * handler to support latch operations, because we only use a local latch.
*/
pqsignal(SIGHUP, SignalHandlerForConfigReload);
pqsignal(SIGINT, SIG_IGN);
- pqsignal(SIGTERM, SIG_IGN);
- pqsignal(SIGQUIT, SignalHandlerForShutdownRequest);
+ pqsignal(SIGTERM, SignalHandlerForShutdownRequest);
+
+ /*
+ * If SIGQUIT is received, exit quickly without doing any additional work,
+ * for example writing stats files. We arrange to do _exit(1) because the
+ * stats collector doesn't touch shared memory.
+ */
+ pqsignal(SIGQUIT, SignalHandlerForNonCrashExit);
pqsignal(SIGALRM, SIG_IGN);
pqsignal(SIGPIPE, SIG_IGN);
pqsignal(SIGUSR1, SIG_IGN);
@@ -4852,8 +4859,8 @@ PgstatCollectorMain(int argc, char *argv[])
AddWaitEventToSet(wes,
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
On 2021-03-18 13:37, Fujii Masao wrote: On 2021/03/18 11:59, kuroda.hay...@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, stats_temp_directory 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 pg_stat 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 CORPORATIONdiff --git a/src/backend/postmaster/interrupt.c b/src/backend/postmaster/interrupt.c index dd9136a942..2cc5a39ec5 100644 --- a/src/backend/postmaster/interrupt.c +++ b/src/backend/postmaster/interrupt.c @@ -93,9 +93,8 @@ SignalHandlerForCrashExit(SIGNAL_ARGS) * shut down and exit. * * Typically, this handler would be used for SIGTERM, but some processes use - * other signals. In particular, the checkpointer exits on SIGUSR2, the - * stats collector on SIGQUIT, and the WAL writer exits on either SIGINT - * or SIGTERM. + * other signals. In particular, the checkpointer exits on SIGUSR2 and the + * WAL writer exits on either SIGINT or SIGTERM. * * ShutdownRequestPending should be checked at a convenient place within the * main loop, or else the main loop should call HandleMainLoopInterrupts. diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index 208a33692f..7c09a8d5fa 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -190,6 +190,8 @@ static time_t
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
On 2021/03/18 11:59, kuroda.hay...@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
RE: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
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
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
On 2021-03-17 08:25, Zhihong Yu wrote:
Hi,
Thanks for your comments!
+ * Simple signal handler for processes HAVE NOT yet touched or DO NOT
I think there should be a 'which' between processes and HAVE. It seems
the words in Capital letters should be in lower case.
+ * Simple signal handler for processes have touched shared memory to
+ * exit quickly.
Add 'which' between processes and have.
OK, I fixed them.
unlink(fname);
+
+ elog(DEBUG2, "removing stats file \"%s\"", fname);
It seems the return value from unlink should be checked and reflected
in the debug message.
There are related codes that show log and call unlink() in slru.c and
pgstat.c.
```
pgstat_write_db_statsfile(PgStat_StatDBEntry *dbentry, bool permanent)
{
// some code
elog(DEBUG2, "removing temporary stats file \"%s\"", statfile);
unlink(statfile)
}
```
I fixed it in the same way instead of checking the return value because
IIUC, it's unimportant if an error has occurred. The log shows that just
to try
removing it. Though?
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATIONdiff --git a/src/backend/postmaster/interrupt.c b/src/backend/postmaster/interrupt.c
index dd9136a942..df8e0eb081 100644
--- a/src/backend/postmaster/interrupt.c
+++ b/src/backend/postmaster/interrupt.c
@@ -64,9 +64,27 @@ SignalHandlerForConfigReload(SIGNAL_ARGS)
}
/*
- * 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.
+ * If processes already touched shared memory, call
+ * SignalHandlerForCrashExit() because shared memory may be corrupted.
+ */
+void
+SignalHandlerForUnsafeExit(SIGNAL_ARGS)
+{
+ /*
+ * Since we don't touch shared memory, we can just pull the plug and exit
+ * without running any atexit handlers.
+ */
+ _exit(1);
+}
+
+/*
+ * Simple signal handler for processes which have touched shared memory to
+ * exit quickly.
+ *
+ * Normally, this would be used for handling SIGQUIT as if due to a crash.
*/
void
SignalHandlerForCrashExit(SIGNAL_ARGS)
@@ -93,9 +111,8 @@ SignalHandlerForCrashExit(SIGNAL_ARGS)
* shut down and exit.
*
* Typically, this handler would be used for SIGTERM, but some processes use
- * other signals. In particular, the checkpointer exits on SIGUSR2, the
- * stats collector on SIGQUIT, and the WAL writer exits on either SIGINT
- * or SIGTERM.
+ * other signals. In particular, the checkpointer exits on SIGUSR2 and the
+ * WAL writer exits on either SIGINT or SIGTERM.
*
* ShutdownRequestPending should be checked at a convenient place within the
* main loop, or else the main loop should call HandleMainLoopInterrupts.
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index b1e2d94951..31b461eb18 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -724,6 +724,7 @@ pgstat_reset_remove_files(const char *directory)
snprintf(fname, sizeof(fname), "%s/%s", directory,
entry->d_name);
+ elog(DEBUG2, "removing stats file \"%s\"", fname);
unlink(fname);
}
FreeDir(dir);
@@ -4821,13 +4822,19 @@ PgstatCollectorMain(int argc, char *argv[])
/*
* Ignore all signals usually bound to some action in the postmaster,
- * except SIGHUP and SIGQUIT. Note we don't need a SIGUSR1 handler to
- * support latch operations, because we only use a local latch.
+ * except SIGHUP, SIGTERM and SIGQUIT. Note we don't need a SIGUSR1
+ * handler to support latch operations, because we only use a local latch.
*/
pqsignal(SIGHUP, SignalHandlerForConfigReload);
pqsignal(SIGINT, SIG_IGN);
- pqsignal(SIGTERM, SIG_IGN);
- pqsignal(SIGQUIT, SignalHandlerForShutdownRequest);
+ pqsignal(SIGTERM, SignalHandlerForShutdownRequest);
+
+ /*
+ * If SIGQUIT is received, exit quickly without doing any additional work,
+ * for example writing stats files. We arrange to do _exit(1) because the
+ * stats collector doesn't touch shared memory.
+ */
+ pqsignal(SIGQUIT, SignalHandlerForUnsafeExit);
pqsignal(SIGALRM, SIG_IGN);
pqsignal(SIGPIPE, SIG_IGN);
pqsignal(SIGUSR1, SIG_IGN);
@@ -4852,8 +4859,8 @@ PgstatCollectorMain(int argc, char *argv[])
AddWaitEventToSet(wes, WL_SOCKET_READABLE, pgStatSock, NULL, NULL);
/*
- * 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.
*
* For performance reasons, we don't want to do ResetLatch/WaitLatch after
* every message; instead, do that only after a recv() fails to obtain a
@@ -4871,7 +4878,7 @@ PgstatCollectorMain(int argc, char *argv[])
ResetLatch(MyLatch);
/*
- * Quit if we get SIGQUIT from the postmaster.
+ * Quit if we get SIGTERM from the postmaster.
*/
if (ShutdownRequestPending)
Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
Hi, + * Simple signal handler for processes HAVE NOT yet touched or DO NOT I think there should be a 'which' between processes and HAVE. It seems the words in Capital letters should be in lower case. + * Simple signal handler for processes have touched shared memory to + * exit quickly. Add 'which' between processes and have. unlink(fname); + + elog(DEBUG2, "removing stats file \"%s\"", fname); It seems the return value from unlink should be checked and reflected in the debug message. Thanks On Tue, Mar 16, 2021 at 4:09 PM Masahiro Ikeda wrote: > On 2021-03-16 13:44, kuroda.hay...@fujitsu.com wrote: > > Dear Ikeda-san > > > > I think the idea is good. > > > > I read the patch and other sources, and I found > > process_startup_packet_die also execute _exit(1). > > I think they can be combined into one function and moved to > > interrupt.c, but > > some important comments might be removed. How do you think? > > Hi, Kuroda-san. > Thanks for your comments. > > I agreed that your idea. > I combined them into one function and moved the comments to > the calling function side. > (v2-0001-pgstat_avoid_writing_on_sigquit.patch) > > Regards, > -- > Masahiro Ikeda > NTT DATA CORPORATION
RE: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
On 2021-03-16 13:44, kuroda.hay...@fujitsu.com wrote:
Dear Ikeda-san
I think the idea is good.
I read the patch and other sources, and I found
process_startup_packet_die also execute _exit(1).
I think they can be combined into one function and moved to
interrupt.c, but
some important comments might be removed. How do you think?
Hi, Kuroda-san.
Thanks for your comments.
I agreed that your idea.
I combined them into one function and moved the comments to
the calling function side.
(v2-0001-pgstat_avoid_writing_on_sigquit.patch)
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATIONdiff --git a/src/backend/postmaster/interrupt.c b/src/backend/postmaster/interrupt.c
index dd9136a942..9b25294a14 100644
--- a/src/backend/postmaster/interrupt.c
+++ b/src/backend/postmaster/interrupt.c
@@ -64,9 +64,27 @@ SignalHandlerForConfigReload(SIGNAL_ARGS)
}
/*
- * Simple signal handler for exiting quickly as if due to a crash.
+ * Simple signal handler for processes HAVE NOT yet touched or DO NOT
+ * touch shared memory to exit quickly.
*
- * Normally, this would be used for handling SIGQUIT.
+ * If processes already touched shared memory, call
+ * SignalHandlerForCrashExit() because shared memory may be corrupted.
+ */
+void
+SignalHandlerForUnsafeExit(SIGNAL_ARGS)
+{
+ /*
+ * Since we don't touch shared memory, we can just pull the plug and exit
+ * without running any atexit handlers.
+ */
+ _exit(1);
+}
+
+/*
+ * Simple signal handler for processes have touched shared memory to
+ * exit quickly.
+ *
+ * Normally, this would be used for handling SIGQUIT as if due to a crash.
*/
void
SignalHandlerForCrashExit(SIGNAL_ARGS)
@@ -93,9 +111,8 @@ SignalHandlerForCrashExit(SIGNAL_ARGS)
* shut down and exit.
*
* Typically, this handler would be used for SIGTERM, but some processes use
- * other signals. In particular, the checkpointer exits on SIGUSR2, the
- * stats collector on SIGQUIT, and the WAL writer exits on either SIGINT
- * or SIGTERM.
+ * other signals. In particular, the checkpointer exits on SIGUSR2 and the
+ * WAL writer exits on either SIGINT or SIGTERM.
*
* ShutdownRequestPending should be checked at a convenient place within the
* main loop, or else the main loop should call HandleMainLoopInterrupts.
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index b1e2d94951..b2156cec9d 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -725,6 +725,8 @@ pgstat_reset_remove_files(const char *directory)
snprintf(fname, sizeof(fname), "%s/%s", directory,
entry->d_name);
unlink(fname);
+
+ elog(DEBUG2, "removing stats file \"%s\"", fname);
}
FreeDir(dir);
}
@@ -4821,13 +4823,19 @@ PgstatCollectorMain(int argc, char *argv[])
/*
* Ignore all signals usually bound to some action in the postmaster,
- * except SIGHUP and SIGQUIT. Note we don't need a SIGUSR1 handler to
- * support latch operations, because we only use a local latch.
+ * except SIGHUP, SIGTERM and SIGQUIT. Note we don't need a SIGUSR1
+ * handler to support latch operations, because we only use a local latch.
*/
pqsignal(SIGHUP, SignalHandlerForConfigReload);
pqsignal(SIGINT, SIG_IGN);
- pqsignal(SIGTERM, SIG_IGN);
- pqsignal(SIGQUIT, SignalHandlerForShutdownRequest);
+ pqsignal(SIGTERM, SignalHandlerForShutdownRequest);
+
+ /*
+ * If SIGQUIT is received, exit quickly without doing any additional work,
+ * for example writing stats files. We arrange to do _exit(1) because the
+ * stats collector doesn't touch shared memory.
+ */
+ pqsignal(SIGQUIT, SignalHandlerForUnsafeExit);
pqsignal(SIGALRM, SIG_IGN);
pqsignal(SIGPIPE, SIG_IGN);
pqsignal(SIGUSR1, SIG_IGN);
@@ -4852,8 +4860,8 @@ PgstatCollectorMain(int argc, char *argv[])
AddWaitEventToSet(wes, WL_SOCKET_READABLE, pgStatSock, NULL, NULL);
/*
- * 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.
*
* For performance reasons, we don't want to do ResetLatch/WaitLatch after
* every message; instead, do that only after a recv() fails to obtain a
@@ -4871,7 +4879,7 @@ PgstatCollectorMain(int argc, char *argv[])
ResetLatch(MyLatch);
/*
- * Quit if we get SIGQUIT from the postmaster.
+ * Quit if we get SIGTERM from the postmaster.
*/
if (ShutdownRequestPending)
break;
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 6436ae0f48..4e1d47e0a1 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -400,7 +400,6 @@ static void SIGHUP_handler(SIGNAL_ARGS);
static void pmdie(SIGNAL_ARGS);
static void reaper(SIGNAL_ARGS);
static void sigusr1_handler(SIGNAL_ARGS);
-static void process_startup_packet_die(SIGNAL_ARGS);
static void dummy_handler(SIGNAL_ARGS);
static void StartupPacketTimeoutHandler(void);
static void
RE: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.
Dear Ikeda-san I think the idea is good. I read the patch and other sources, and I found process_startup_packet_die also execute _exit(1). I think they can be combined into one function and moved to interrupt.c, but some important comments might be removed. How do you think? Best Regards, Hayato Kuroda FUJITSU LIMITED
