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

Few comments for v4 patch:

@@ -7351,6 +7363,8 @@ StartupXLOG(void)
(errmsg("redo starts at %X/%X",
LSN_FORMAT_ARGS(ReadRecPtr))));

+ InitStartupProgress();
+
/*
* main redo apply loop
*/
@@ -7358,6 +7372,8 @@ StartupXLOG(void)
{
bool switchedTLI = false;

+ LogStartupProgress(RECOVERY_IN_PROGRESS, NULL);
+
#ifdef WAL_DEBUG
if (XLOG_DEBUG ||
(rmid == RM_XACT_ID && trace_recovery_messages <= DEBUG2) ||
@@ -7569,6 +7585,8 @@ StartupXLOG(void)
* end of main redo apply loop
*/

+ CloseStartupProgress(RECOVERY_END);

I am not sure I am getting the code flow correctly. From CloseStartupProgress()
naming it seems, it corresponds to InitStartupProgress() but what it is doing
is similar to LogStartupProgress(). I think it should be renamed to be inlined
with LogStartupProgress(), IMO.
---

+
+ /* Return if any invalid operation */
+ if (operation >= SYNCFS_END)
+ return;
....
+ /* Return if any invalid operation */
+ if (operation < SYNCFS_END)
+ return;
+

This part should be an assertion, it's the developer's responsibility to call
correctly.
---

+/*
+ * Codes of the operations performed during startup process
+ */
+typedef enum StartupProcessOp
+{
+ /* Codes for in-progress operations */
+ SYNCFS_IN_PROGRESS,
+ FSYNC_IN_PROGRESS,
+ RECOVERY_IN_PROGRESS,
+ RESET_UNLOGGED_REL_IN_PROGRESS,
+ /* Codes for end of operations */
+ SYNCFS_END,
+ FSYNC_END,
+ RECOVERY_END,
+ RESET_UNLOGGED_REL_END
+} StartupProcessOp;
+

Since we do have a separate call for the in-progress operation and the
end-operation, only a single enum would have been enough. If we do this, then I
think we should remove get_startup_process_operation_string() move messages to
the respective function.
---

Also, with your patch "make check-world" has few failures, kindly check that.

Regards,
Amul

On Mon, Jun 21, 2021 at 12:06 PM Nitin Jadhav
<nitinjadhavpostgres(at)gmail(dot)com> wrote:
>
> > What is DUMMY about ? If you just want to separate the "start" from "end",
> > you could write:
> >
> > /* codes for start of operations */
> > FSYNC_IN_PROGRESS
> > SYNCFS_IN_PROGRESS
> > ...
> > /* codes for end of operations */
> > FSYNC_END
> > SYNCFS_END
> > ...
>
> That was by mistake and I have corrected it in the attached patch.
>
> Thanks & Regards,
> Nitin Jadhav
>
> On Thu, Jun 17, 2021 at 6:22 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> >
> > + * Codes of the operations performed during startup process
> > + */
> > +typedef enum StartupProcessOp
> > +{
> > + SYNCFS_IN_PROGRESS,
> > + FSYNC_IN_PROGRESS,
> > + RECOVERY_IN_PROGRESS,
> > + RESET_UNLOGGED_REL_IN_PROGRESS,
> > + DUMMY,
> > + SYNCFS_END,
> > + FSYNC_END,
> > + RECOVERY_END,
> > + RESET_UNLOGGED_REL_END
> > +} StartupProcessOp;
> >
> > What is DUMMY about ? If you just want to separate the "start" from "end",
> > you could write:
> >
> > /* codes for start of operations */
> > FSYNC_IN_PROGRESS
> > SYNCFS_IN_PROGRESS
> > ...
> > /* codes for end of operations */
> > FSYNC_END
> > SYNCFS_END
> > ...
> >
> > Or group them together like:
> >
> > FSYNC_IN_PROGRESS,
> > FSYNC_END,
> > SYNCFS_IN_PROGRESS,
> > SYNCFS_END,
> > RECOVERY_IN_PROGRESS,
> > RECOVERY_END,
> > RESET_UNLOGGED_REL_IN_PROGRESS,
> > RESET_UNLOGGED_REL_END,
> >
> > --
> > Justin