Add CHECKPOINT_REQUESTED flag to the log message in LogCheckpointStart()

On Wed, Mar 2, 2022 at 5:41 PM Nitin Jadhav
<nitinjadhavpostgres@gmail.com> 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]/* * 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++; }. 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++; }
/*
* 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.

At Wed, 2 Mar 2022 18:18:10 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in

On Wed, Mar 2, 2022 at 5:41 PM Nitin Jadhav
<nitinjadhavpostgres@gmail.com> 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.

Agreed.

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

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.

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)
+          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

On Thu, Mar 03, 2022 at 09:39:37AM +0900, Kyotaro Horiguchi wrote:

At Wed, 2 Mar 2022 18:18:10 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in

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.

Agreed.

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.

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.

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

At Thu, 3 Mar 2022 10:27:10 +0900, Michael Paquier <michael@paquier.xyz> wrote in

On Thu, Mar 03, 2022 at 09:39:37AM +0900, Kyotaro Horiguchi wrote:

At Wed, 2 Mar 2022 18:18:10 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in

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.

Agreed.

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.

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.

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.

--
Kyotaro Horiguchi
NTT Open Source Software Center