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

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@gmail.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@telsasoft.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