> +/* > + * Decides whether to log the startup progress or not based on whether the > + * timer is expired or not. Returns FALSE if the timer is not expired, > otherwise > + * calculates the elapsed time and sets the respective out parameters secs > and > + * usecs. Enables the timer for the next log message and returns TRUE. > + */ > +bool > +startup_progress_timeout_has_expired(long *secs, int *usecs)
I think this comment can be worded better. It says it "decides", but it doesn't actually decide on any action to take -- it just reports whether the timer expired or not, to allow its caller to make the decision. In such situations we just say something like "Report whether startup progress has caused a timeout, return true and rearm the timer if it did, or just return false otherwise"; and we don't indicate what the value is going to be used *for*. Then the caller can use the boolean return value to make a decision, such as whether something is going to be logged. This function can be oblivious to details such as this: > + /* If the timeout has not occurred, then no need to log the details. */ > + if (!startup_progress_timer_expired) > + return false; here we can just say "No timeout has occurred" and make no inference about what's going to happen or not happen. Also, for functions that do things like this we typically use English sentence structure with the function name starting with the verb -- perhaps has_startup_progress_timeout_expired(). Sometimes we are lax about this if we have some sort of poor-man's modularisation by using a common prefix for several functions doing related things, which perhaps could be "startup_progress_*" in your case, but your other functions are already not doing that (such as begin_startup_progress_phase) so it's not clear why you would not use the most natural name for this one. > + ereport_startup_progress("syncing data directory (syncfs), elapsed > time: %ld.%02d s, current path: %s", > + path); Please make sure to add ereport_startup_progress() as a translation trigger in src/backend/nls.mk. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/