Re: when the startup process doesn't (logging startup delays)
Robert Haas <robertmhaas@gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
To: Nitin Jadhav <nitinjadhavpostgres@gmail.com>
Cc: 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>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2021-09-28T14:59:27Z
Lists: pgsql-hackers
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
Commits
-
Un-revert "Disable STARTUP_PROGRESS_TIMEOUT in standby mode."
- ecb01e6ebb5a 15.3 landed
-
Revert "Disable STARTUP_PROGRESS_TIMEOUT in standby mode."
- 1eadfbdd7eb0 15.2 landed
-
Disable STARTUP_PROGRESS_TIMEOUT in standby mode.
- 98e7234242a6 15.2 landed
- 8a2f783cc489 16.0 landed
-
Fix race condition in startup progress reporting.
- 5ccceb2946d4 15.0 landed
-
Report progress of startup operations that take a long time.
- 9ce346eabf35 15.0 landed
-
Add enable_timeout_every() to fire the same timeout repeatedly.
- 732e6677a667 15.0 landed