On Tue, Aug 10, 2021 at 9:28 AM Nitin Jadhav <nitinjadhavpostg...@gmail.com> wrote: > Please find the updated patch attached.
I think this is getting close. The output looks nice. However, I still see a bunch of issues. You mentioned previously that you would add documentation, but I do not see it here. startup_progress_timer_expired should be declared as sig_atomic_t like we do in other cases (see interrupt.c). The default value of the new GUC is 10s in postgresql.conf.sample, but -1 in guc.c. They should both be 10s, and the one in postgresql.conf.sample should be commented out. I suggest making the GUC GUC_UNIT_MS rather than GUC_UNIT_S, but expressing the default in postgresl.conf.sample as 10s rather than 10000ms. I tried values measured in milliseconds just for testing purposes and didn't initially understand why it wasn't working. I don't think there's any reason it can't work. I would prefer to see log_startup_progress_interval declared and defined in startup.c/startup.h rather than guc.c/guc.h. There's no precedent in the tree for the use of ##__VA_ARGS__. On my system it seems to work fine if I just leave out the ##. Any reason not to do that? Two of the declarations in startup.h forgot the leading "extern", while the other two that are right next to them have it, matching project style. I'm reasonably happy with most of the identifier names now, but I think init_startup_progress() is confusing. The reason I think that is that we call it more than once, which is not really what people think about when they think of an "init" function, I think. It's not initializing the startup progress facility in general; it's preparing for the next phase of startup progress reporting. How about renaming it to begin_startup_progress_phase()? And then startup_process_op_start_time could be startup_process_phase_start_time to match. SyncDataDirectory() potentially walks over the data directory three times: once to call do_syncfs(), once to call pre_sync_fname(), and once to call datadir_fsync_fname(). With this patch, the first and third will emit progress messages if the operation runs long, but the second will not. I think they should all be treated the same. Also, the locations where you've inserted calls to init_startup_progress() don't really make it clear with what code that's associated. I'd put them *immediately* before the call to do_syncfs() or walkdir(). Remember that PostgreSQL comments are typically written "telegraph style," so function comments should say "Does whatever" not "It does whatever". Most of them are correct, but there's one sentence you need to fix. -- Robert Haas EDB: http://www.enterprisedb.com