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

On Mon, Jun 21, 2021 at 12:06 PM Nitin Jadhav
<nitinjadhavpostgres@gmail.com> wrote:
> That was by mistake and I have corrected it in the attached patch.

Thanks for the patch. Here are some comments. Please ignore if I
repeat any of the comments given previously, as I didn't look at the
entire thread.

1) A new line between function return value and the function name:
+void CloseStartupProgress(StartupProcessOp operation)
+{
Like below:
+void
+CloseStartupProgress(StartupProcessOp operation)
+{

2) Add an entry in the commit fest, if it's not done yet. That way,
the patch gets tested on many platforms.

3) Replace "zero" with the number "0".
+ # -1 disables the feature, zero logs all

4) I think we can just rename InitStartupProgress to
EnableStartupProgress or EnableStartupOpProgress to be more in sync
with what it does.
+/*
+ * Sets the start timestamp of the current operation and also enables the
+ * timeout for logging the progress of startup process.
+ */
+void
+InitStartupProgress(void)
+{

5) Do we need the GetCurrentOperationStartTimestamp function at all?
It does nothing great actually, you might have referred to
GetCurrentTimestamp which does a good amount of work that qualifies to
be inside a function. Can't we just use the operationStartTimestamp
variable? Can we rename operationStartTimestamp (I don't think we need
to specify Timestamp in a variable name) to startup_op_start_time or
some other better name?
+/*
+ * Fetches the start timestamp of the current operation.
+ */
+TimestampTz
+GetCurrentOperationStartTimestamp(void)
+{

6) I think you can transform below
+ /* If the feature is disabled, then no need to proceed further. */
+ if (log_startup_progress_interval < 0)
+ return;
to
+ /* If the feature is disabled, then no need to proceed further. */
+ if (log_startup_progress_interval == -1)
+ return;
as -1 means feature disabled and values < -1 are not allowed to be set at all.

7) Isn't RECOVERY_IN_PROGRESS supposed to be REDO_IN_PRGRESS? Because,
"recovery in progress" generally applies to the entire startup process
right? Put it another way, the startup process as a whole does the
entire recovery, and you have the RECOVERY_IN_PROGRESS in the redo
phase of the entire startup process.

8) Why do we need to call get_startup_process_operation_string here?
Can't you directly use the error message text?
if (operation == RECOVERY_IN_PROGRESS)
ereport(LOG,
(errmsg("%s, elapsed time: %ld.%03d ms, current LSN: %X/%X",
get_startup_process_operation_string(operation),

9) Do you need error handling in the default case of
get_startup_process_operation_string? Instead, how about an assertion,
Assert(operation >= SYNCFS_IN_PROGRESS && operation <=
RESET_UNLOGGED_REL_END);?
        default:
            ereport(ERROR,
                    (errmsg("unrecognized operation (%d) in startup
progress update",
                            operation)));
10) I personally didn't like the name
get_startup_process_operation_string. How about get_startup_op_string?

11) As pointed out by Robert, the error log message should start with
small letters.
"syncing data directory (syncfs)");
"syncing data directory (fsync)");
"performing crash recovery");
"resetting unlogged relations");
 In general, the error log message should start with small letters and
not end with ".". The detail/hit messages should start with capital
letters and end with "."
        ereport(ERROR,
                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                 errmsg("only B-Tree indexes are supported as targets
for verification"),
                 errdetail("Relation \"%s\" is not a B-Tree index.",
                           RelationGetRelationName(rel))));
         ereport(ERROR,
                        (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                         errmsg("sslcert and sslkey are superuser-only"),
                         errhint("User mappings with the sslcert or
sslkey options set may only be created or modified by the
superuser")));

12) How about starting SYNCFS_IN_PROGRESS = 1, and leaving 0 for some
unknown value?
typedef enum StartupProcessOp
{
    /* Codes for in-progress operations */
    SYNCFS_IN_PROGRESS = 1,

13) Can we combine LogStartupProgress and CloseStartupProgress?  Let's
have a single function LogStartupProgress(StartupProcessOp operation,
const char *path, bool start);, and differentiate the functionality
with the start flag.

14) Can we move log_startup_progress_interval declaration from guc.h
and guc.c to xlog.h and xlog.c? Because it makes more sense to be
there, similar to the other GUCs under /* these variables are GUC
parameters related to XLOG */ in xlog.h.

Regards,
Bharath Rupireddy.