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

Hi,

This thread came from another thread about collecting the WAL
stats([1]).

Is it better to make the stats collector shutdown without writing the
stats files
if the immediate shutdown is requested?

There was a related discussion([2]) although it's stopped on 12/1/2016.
IIUC, the thread's consensus are

(1) It's useful to make the stats collector shutdown quickly without
writing the stats files
     if the immediate shutdown is requested in some cases because there
is a possibility
     that it takes a long time until the failover happens. The possible
reasons are that
     disk write speed is slow, the stats files are big, and so on. And
there is no negative impact
     on the users because all stats files are removed in a crash recovery
phase now.

(2) As another aspect, it needs to change the behavior removing all
stats files because autovacuum
     uses the stats. There are some ideas, for example writing the stats
files every X minutes
     (using wal or another mechanism) and even if a crash occurs, the
startup process can restore
     the stats with slightly low freshness. But, it needs to find a way
how to handle the stats files
     when deleting on PITR rewind or stats collector crash happens.

I agree that the above consensus and I think we can make separate two
patches.
The first one is for (1) and the second one is for (2).

Why don't you apply the patch for (1) first?
I attached the patch based on tsunakawa-san's patch([2]).
(v1-0001-pgstat_avoid_writing_on_sigquit.patch)

At this time, there are no cons points for the users because
the stats files are removed in a crash recovery phase as pointed in the
discussion.

[1]
https://www.postgresql.org/message-id/c616cf24bf4ecd818f7cab0900a40842%40oss.nttdata.com
[2]
https://www.postgresql.org/message-id/flat/0A3221C70F24FB45833433255569204D1F5EF25A%40G01JPEXMBYT05

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

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, 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

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, 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: 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 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.


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:
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: 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: 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:
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:
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:
Fujii-san, Andres-san,
Thanks for your comments!

I didn't think about the start order. From the point of view, I noticed that
the current source code has two other concerns.


1. This problem is not only for the wal receiver.

The problem which the wal receiver starts before the stats collector
is launched during archive recovery is not only for the the wal receiver but
also the checkpointer and the bgwriter. Before starting redo, the startup
process sends the postmaster "PMSIGNAL_RECOVERY_STARTED" signal to launch the
checkpointer and the bgwriter to be able to perform creating restartpoint.

Although the socket for communication between the stats collector and the
other processes is made in earlier stage via pgstat_init(), I agree to make
the stats collector starts earlier stage is defensive. BTW, in my
environments(linux, net.core.rmem_default = 212992), the socket can buffer
almost 300 WAL stats messages. This mean that, as you said, if the redo phase
is too long, it can lost the messages easily.


2. To make the stats clear in redo phase.

The statistics can be reset after the wal receiver, the checkpointer and
the wal writer are started in redo phase. So, it's not enough the stats
collector is invoked at more earlier stage. We need to fix it.



(I hope I am not missing something.)
Thanks to Andres-san's work([1]), the above problems will be handle in the
shared memory stats patch. First problem will be resolved since the stats are
collected in shared memory, so the stats collector process is unnecessary
itself. Second problem will be resolved to remove the reset code because the
temporary stats file won't generated, and if the permanent stats file
corrupted, just recreate it.

[1]
https://github.com/anarazel/postgres/compare/master...shmstat-before-split-2021-03-22

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

On 2021/03/24 18:36, Fujii Masao wrote:
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