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

On 2021-03-16 13:44, kuroda.hayato@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

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 <ikedamsh@oss.nttdata.com>
wrote:

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 CORPORATION

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]https://github.com/anarazel/postgres/compare/master...shmstat-before-split-2021-03-22), 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
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]/messages/by-id/c96d8989100e4bce4fa586064aa7e0e9@oss.nttdata.com). 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]/messages/by-id/0A3221C70F24FB45833433255569204D1F5EF25A@G01JPEXMBYT05). 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]: /messages/by-id/c96d8989100e4bce4fa586064aa7e0e9@oss.nttdata.com
/messages/by-id/c96d8989100e4bce4fa586064aa7e0e9@oss.nttdata.com
[2]: /messages/by-id/0A3221C70F24FB45833433255569204D1F5EF25A@G01JPEXMBYT05
/messages/by-id/0A3221C70F24FB45833433255569204D1F5EF25A@G01JPEXMBYT05

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION