Subject: Re: Add CHECKPOINT_REQUESTED flag to the log message in LogCheckpointStart()

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 2 Mar 2022 18:18:10 +0530

On Wed, Mar 2, 2022 at 5:41 PM Nitin Jadhav
<[email-masked]> wrote:
>
> Hi,
>
> I have noticed that the CHECKPOINT_REQUESTED flag information is not
> present in the log message of LogCheckpointStart() function. I would
> like to understand if it was missed or left intentionally. The log
> message describes all the possible checkpoint flags except
> CHECKPOINT_REQUESTED flag. I feel we should support this. Thoughts?

I don't think that's useful. Being in LogCheckpointStart
(CreateCheckPoint or CreateRestartPoint) itself means that somebody
has requested a checkpoint. Having CHECKPOINT_REQUESTED doesn't add
any value.

I would suggest removing the CHECKPOINT_REQUESTED flag as it's not
being used anywhere instead CheckpointerShmem->ckpt_flags is used as
an indication of the checkpoint requested in CheckpointerMain [1]. If
others don't agree to remove as it doesn't cause any harm, then, I
would  add something like this for more readability:

         if ((((volatile CheckpointerShmemStruct *)
CheckpointerShmem)->ckpt_flags) & CHECKPOINT_REQUESTED))
        {
            do_checkpoint = true;
            PendingCheckpointerStats.m_requested_checkpoints++;
        }

[1]
        /*
         * Detect a pending checkpoint request by checking whether the flags
         * word in shared memory is nonzero.  We shouldn't need to acquire the
         * ckpt_lck for this.
         */
        if (((volatile CheckpointerShmemStruct *)
CheckpointerShmem)->ckpt_flags)
        {
            do_checkpoint = true;
            PendingCheckpointerStats.m_requested_checkpoints++;
        }

Regards,
Bharath Rupireddy.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 03 Mar 2022 09:39:37 +0900 (JST)

At Wed, 2 Mar 2022 18:18:10 +0530, Bharath Rupireddy <[email-masked]> wrote in 
> has requested a checkpoint. Having CHECKPOINT_REQUESTED doesn't add

Agreed.

> an indication of the checkpoint requested in CheckpointerMain [1]. If

Actually no one does but RequestCheckpoint() accepts 0 as flags.
Checkpointer would be a bit more complex without CHECKPOINT_REQUESTED.
I don't think it does us any good to get rid of the flag value.

     if (((volatile CheckpointerShmemStruct *)
-          CheckpointerShmem)->ckpt_flags)
+          CheckpointerShmem)->ckpt_flags) & CHECKPOINT_REQUESTED))

I don't particularly object to this, but I don't think that change
makes the code significantly easier to read either.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Michael Paquier <[email-masked]>
Date: Thu, 3 Mar 2022 10:27:10 +0900

>> has requested a checkpoint. Having CHECKPOINT_REQUESTED doesn't add

Exactly my impression.  This would apply now to the WAL shutdown code
paths, and I'd suspect that the callers of CreateCheckPoint() are not
going to increase soon.  The point is: the logs already provide some
contexts for any of those callers so I see no need for this additional
information.

> I don't think it does us any good to get rid of the flag value.

I'd rather keep this code as-is.
--
Michael

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 03 Mar 2022 12:04:32 +0900 (JST)

At Thu, 3 Mar 2022 10:27:10 +0900, Michael Paquier <[email-masked]> wrote in 
> I'd rather keep this code as-is.

I fail to identify the nuance of the phrase, so just for a
clarification.  In short, I think we should keep the exiting code
as-is.

regards.