Re: when the startup process doesn't (logging startup delays) - Mailing list pgsql-hackers

> It is common to mention what the default is as part of the
> documentation of a GUC. I don't see why this one should be an
> exception, especially since not mentioning it reduces the length of
> the documentation by exactly one word.
>
> I don't mind the sentence you added at the end to clarify the default
> units, but the way you've rewritten the first sentence makes it, in my
> opinion, much less clear.

Ok. I have kept your documentation as it is and added the sentence at
the end to clarify the default units.

> v9 was posted on August 3rd. I told you that it wasn't working on
> September 23rd. You posted a new version that still does not work on
> September 27th. I think you should have tested each version of your
> patch before posting it, and especially after any major refactorings.
> And if for whatever reason you didn't, then certainly after I told you
> on September 23rd that it didn't work, I think you should have fixed
> it before posting a new version.

Sorry. There was a misunderstanding about this and for the patch
shared on September 27th, I had tested for the value '0' and observed
that no progress messages were getting logged, probably the time at
which 'enable_timeout_at' is getting called is past the 'next_timeout'
value. This behaviour is completely dependent on the system. Now added
an extra condition to disable the feature in case of '0' setting.

> 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:
>
> here we can just say "No timeout has occurred" and make no inference
> about what's going to happen or not happen.

Modified the comment.

> 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.

I agree that has_startup_progress_timeout_expired() is better than the
existing function name. So I changed the function name accordingly.

> Please make sure to add ereport_startup_progress() as a translation
> trigger in src/backend/nls.mk.

I have added ereport_startup_progress() under the section
GETTEXT_TRIGGERS and GETTEXT_FLAGS in src/backend/nls.mk. Also
verified the messages in src/backend/po/postgres.pot file.

Kindly let me know if I have missed anything.

Thanks & Regards,
Nitin Jadhav

On Tue, Sep 28, 2021 at 8:29 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Sep 28, 2021 at 8:06 AM Nitin Jadhav
> <nitinjadhavpostgres@gmail.com> wrote:
> > I thought mentioning the unit in milliseconds and the example in
> > seconds would confuse the user, so I changed the example to
> > milliseconds.The message behind the above description looks good to me
> > however I feel some sentences can be explained in less words. The
> > information related to the units is missing and I feel it should be
> > mentioned in the document. The example says, if the setting has the
> > default value of 10 seconds, then it explains the behaviour. I feel it
> > may not be the default value, it can be any value set by the user. So
> > mentioning 'default' in the example does not look good to me. I feel
> > we just have to mention "if this setting is set to the value of 10
> > seconds". Below is the modified version of the above information.
>
> It is common to mention what the default is as part of the
> documentation of a GUC. I don't see why this one should be an
> exception, especially since not mentioning it reduces the length of
> the documentation by exactly one word.
>
> I don't mind the sentence you added at the end to clarify the default
> units, but the way you've rewritten the first sentence makes it, in my
> opinion, much less clear.
>
> > I had added additional code to check the value of the
> > 'log_startup_progress_interval' variable and  disable the feature in
> > case of -1 in the earlier versions of the patch (Specifically
> > v9.patch). There was a review comment for v9 patch and it resulted in
> > major refactoring of the patch.
> ...
> > The answer for the question of "I don't understand why you posted the
> > previous version of the patch without testing that it works" is, for
> > the value of -1, the above description was my understanding and for
> > the value of 0, the older versions of the patch was behaving as
> > documented. But with the later versions the behaviour got changed and
> > I missed to modify the documentation. So I modified it in the last
> > version.
>
> v9 was posted on August 3rd. I told you that it wasn't working on
> September 23rd. You posted a new version that still does not work on
> September 27th. I think you should have tested each version of your
> patch before posting it, and especially after any major refactorings.
> And if for whatever reason you didn't, then certainly after I told you
> on September 23rd that it didn't work, I think you should have fixed
> it before posting a new version.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com