> Since you agreed that SUSET was wrong, and PGC_POSTMASTER doesn't allow > changing the value without restart, doesn't it follow that SIGHUP is what's > wanted ?
Yes. I have done the changes in the attached patch. Apart from this, I have done a few other changes to the patch. The changes include 1. Renamed 'InitCurrentOperation' to 'InitStartupProgress()'. 2. Divided the functionality of 'LogStartupProgress()' into 2 parts. One for logging the progress and the other to log the completion information. The first part's function name remains as is and a new function 'CloseStartupProgress()' added for the second part. 3. In case of any invalid operations found during logging of the startup progress, throwing an error. This is not a concern unless the developer makes a mistake. 4. Modified the 'StartupProcessOp' enums like 'FSYNC_START' to 'FSYNC_IN_PROGRESS' for better readability. 5. Updated the comments and some cosmetic changes. Kindly share your comments. Thanks & Regards, Nitin Jadhav On Thu, Jun 10, 2021 at 3:31 PM Justin Pryzby <pry...@telsasoft.com> wrote: > > On Thu, Jun 10, 2021 at 03:19:20PM +0530, Nitin Jadhav wrote: > > > > > + {"log_min_duration_startup_process", PGC_SUSET, > > > > > LOGGING_WHEN, > > > > > > > > > > I think it should be PGC_SIGHUP, to allow changing it during runtime. > > > > > Obviously it has no effect except during startup, but the change will > > > > > be > > > > > effective if the current process crashes. > > > > > See also: > > > > > https://www.postgresql.org/message-id/20210526001359.ge3...@telsasoft.com > > > > > > > > I did not get exactly how it will change behaviour. In my > > > > understanding, when the server restarts after a crash, it fetches the > > > > value from the config file. So if there is any change that gets > > > > affected. Kindly correct me if I am wrong. > > > > Sorry my understanding was wrong. But I'm not completely convinced > > with the above description saying that the change will be effective if > > the current process crashes. > > AFAIK, whenever we set the GucContext less than PGC_SIGHUP (that is > > either PGC_POSTMASTER or PGC_INTERNAL) then any change in the config > > file will not get affected during restart after crash. If the > > GucContext is greater than or equal to PGC_SIGHUP, then any change in > > the config file will be changed once it receives the SIGHUP signal. So > > it gets affected by a restart after a crash. So since the GucContext > > set here is PGC_SUSET which is greater than PGC_SIGHUP, there is no > > change in the behaviour wrt this point. > > Since you agreed that SUSET was wrong, and PGC_POSTMASTER doesn't allow > changing the value without restart, doesn't it follow that SIGHUP is what's > wanted ? > > > > I've triple checked the behavior using a patch I submitted for Thomas' > > > syncfs > > > feature. ALTER SYSTEM recovery_init_sync_method=syncfs was not picked up > > > when > > > I sent SIGABRT. But with my patch, if I also do SELECT pg_reload_conf(), > > > then > > > a future crash uses syncfs. > > > https://www.postgresql.org/message-id/20210526001359.ge3...@telsasoft.com > > > > The difference is since the behaviour is compared between > > PGC_POSTMASTER and PGC_SIGHUP. > > > > > The GUC definitely isn't SUSET, since it's not useful to write in a > > > (super) > > > user session SET log_min_duration_startup_process=123. > > I agree with this. I may have to change this value as setting in a > > user session is not at all useful. But I am confused between > > PGC_POSTMASTER and PGC_SIGHUP. We should use PGC_SIGHUP if we would > > like to allow the change during restart after a crash. Otherwise > > PGC_POSTMASTER would be sufficient. Kindly share your thoughts. > > > > Thanks & Regards, > > Nitin Jadhav > > > > On Wed, Jun 9, 2021 at 9:49 PM Justin Pryzby <pry...@telsasoft.com> wrote: > > > > > > On Wed, Jun 09, 2021 at 05:09:54PM +0530, Nitin Jadhav wrote: > > > > > + {"log_min_duration_startup_process", PGC_SUSET, > > > > > LOGGING_WHEN, > > > > > > > > > > I think it should be PGC_SIGHUP, to allow changing it during runtime. > > > > > Obviously it has no effect except during startup, but the change will > > > > > be > > > > > effective if the current process crashes. > > > > > See also: > > > > > https://www.postgresql.org/message-id/20210526001359.ge3...@telsasoft.com > > > > > > > > I did not get exactly how it will change behaviour. In my > > > > understanding, when the server restarts after a crash, it fetches the > > > > value from the config file. So if there is any change that gets > > > > affected. Kindly correct me if I am wrong. > > > > > > I don't think so. I checked and SelectConfigFiles is called only once to > > > read > > > config files and cmdline args. And not called on restart_after_crash. > > > > > > The GUC definitely isn't SUSET, since it's not useful to write in a > > > (super) > > > user session SET log_min_duration_startup_process=123. > > > > > > I've triple checked the behavior using a patch I submitted for Thomas' > > > syncfs > > > feature. ALTER SYSTEM recovery_init_sync_method=syncfs was not picked up > > > when > > > I sent SIGABRT. But with my patch, if I also do SELECT pg_reload_conf(), > > > then > > > a future crash uses syncfs. > > > https://www.postgresql.org/message-id/20210526001359.ge3...@telsasoft.com
v3-0001-startup-process-progress.patch
Description: Binary data