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

2021-03-29 Thread Masahiro Ikeda
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.

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

2021-03-26 Thread 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 discusse

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

2021-03-26 Thread Andres Freund
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 > > na

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

2021-03-26 Thread Masahiro Ikeda
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?

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

2021-03-25 Thread Fujii Masao
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. S

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

2021-03-25 Thread Masahiro Ikeda
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

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

2021-03-25 Thread Masahiro Ikeda
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 stat

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

2021-03-25 Thread Fujii Masao
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 bac

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

2021-03-25 Thread Fujii Masao
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

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

2021-03-24 Thread Andres Freund
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 processe

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

2021-03-24 Thread Masahiro Ikeda
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 pr

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

2021-03-24 Thread Masahiro Ikeda
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

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

2021-03-24 Thread Fujii Masao
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 walr

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

2021-03-24 Thread Fujii Masao
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 u

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

2021-03-23 Thread Andres Freund
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_

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

2021-03-22 Thread Fujii Masao
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

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

2021-03-22 Thread Masahiro Ikeda
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 o

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

2021-03-22 Thread Fujii Masao
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 Terminat

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

2021-03-22 Thread Masahiro Ikeda
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.

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

2021-03-22 Thread Fujii Masao
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

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

2021-03-19 Thread Masahiro Ikeda
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?

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

2021-03-18 Thread Fujii Masao
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 thin

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

2021-03-18 Thread Masahiro Ikeda
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 chan

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

2021-03-17 Thread Fujii Masao
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 cl

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

2021-03-17 Thread kuroda.hay...@fujitsu.com
Dear Ikeda-san, I confirmed new patch and no problem was found. Thanks. (I'm not a native English speaker, so I cannot check your comments correctly, sorry) Best Regards, Hayato Kuroda FUJITSU LIMITED

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

2021-03-16 Thread Masahiro Ikeda
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

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

2021-03-16 Thread Zhihong Yu
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 'wh

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

2021-03-16 Thread Masahiro Ikeda
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

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

2021-03-15 Thread kuroda.hay...@fujitsu.com
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, Hay

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

2021-03-15 Thread Masahiro Ikeda
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