Date: Mon, 27 Sep 2021 13:47:38 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Nitin Jadhav <nitinjadhavpostgres@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>,
	Justin Pryzby <pryzby@telsasoft.com>,
	Michael Paquier <michael@paquier.xyz>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>,
	Stephen Frost <sfrost@snowman.net>,
	Magnus Hagander <magnus@hagander.net>,
	Thomas Munro <thomas.munro@gmail.com>,
	Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: when the startup process doesn't (logging startup delays)
Message-ID: <202109271647.frp35okoz4ou@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAMm1aWbZvHuodydSeV5=K8iuF_Qf+OaY9zFBzNAb3-B5umjWng@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202109271647.frp35okoz4ou%40alvherre.pgsql>
Precedence: bulk

> +/*
> + * 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/