Re: when the startup process doesn't (logging startup delays)

On Thu, Aug 12, 2021 at 7:40 AM Nitin Jadhav
<nitinjadhavpostgres@gmail.com> wrote:
> > 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.
>
> As suggested, I have changed it to GUC_UNIT_MS and kept the default
> value to 10s. I would like to know the reason why it can't be
> GUC_UNIT_S as we are expressing the values in terms of seconds.

I mean, it *could* be. There's just no advantage. Values in seconds
will work correctly either way. But values in milliseconds will only
work if it's GUC_UNIT_MS. It seems to me that it's better to make more
things work rather than fewer.

> > 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?
>
> I had added this to support if no variable argument are passed to the
> macro. For example, in the previous patches we used to log the
> progress at the end of the operation like
> "ereport_startup_progress(true, "data directory sync (syncfs) complete
> after %ld.%02d s");". Since these calls are removed now, ## is not
> required. Hence removed in the attached patch.

Hmm, OK. That's actually a pretty good reason for using ## there. It
just made me nervous because we have no similar uses of ## in the
backend code. We rely on it elsewhere for concatenation, but not for
comma removal. Let's try leaving it out for now unless somebody else
shows up with a different opinion.

> I had not added extern since those function were not used in the other
> files. Now added to match the project style.

Anything that's not used in other files should be declared static in
the file itself, and not present in the header. Once you fix this for
reset_startup_progress_timeout, the header won't need to include
datatype/timestamp.h any more, which is good, because we don't want
header files to depend on more other header files than necessary.

Looking over this version, I realized something I (or you) should have
noticed sooner: you've added the RegisterTimeout call to
InitPostgres(), but that's for things that are used by all backends,
and this is only used by the startup process. So it seems to me that
the right place is StartupProcessMain. That would have the further
advantage of allowing startup_progress_timeout_handler to be made
static. begin_startup_progress_phase() and
startup_progress_timeout_has_expired() are the actual API functions
though so they will need to remain extern.

@@ -679,7 +680,6 @@ static char *recovery_target_lsn_string;
 /* should be static, but commands/variable.c needs to get at this */
 char      *role_string;

-
 /*
  * Displayable names for context types (enum GucContext)
  *

This hunk should be removed.

-- 
Robert Haas
EDB: http://www.enterprisedb.com