Re: New WAL record to detect the checkpoint redo location

On Mon, Aug 28, 2023 at 5:14 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Aug 25, 2023 at 11:08:25AM +0530, Dilip Kumar wrote:
> > Here is the updated version of the patch.
>
> The concept of the patch looks sound to me.  I have a few comments.

Thanks for the review

> +        * This special record, however, is not required when we doing a shutdown
> +        * checkpoint, as there will be no concurrent wal insertions during that
> +        * time.  So, the shutdown checkpoint LSN will be the same as
> +        * checkpoint-redo LSN.
>
> This is missing "are", as in "when we are doing a shutdown
> checkpoint".

Fixed

> -   freespace = INSERT_FREESPACE(curInsert);
> -   if (freespace == 0)
>
> The variable "freespace" can be moved within the if block introduced
> by this patch when calculating the REDO location for the shutdown
> case.  And you can do the same with curInsert?

Done, I have also moved code related to computing curInsert in the
same if (shutdown) block.

> -        * Compute new REDO record ptr = location of next XLOG record.
> -        *
> -        * NB: this is NOT necessarily where the checkpoint record itself will be,
> -        * since other backends may insert more XLOG records while we're off doing
> -        * the buffer flush work.  Those XLOG records are logically after the
> -        * checkpoint, even though physically before it.  Got that?
> +        * In case of shutdown checkpoint, compute new REDO record
> +        * ptr = location of next XLOG record.
>
> It seems to me that keeping around this comment is important,
> particularly for the case where we have a shutdown checkpoint and we
> expect nothing to run, no?

I removed this mainly because now in other comments[1] where we are
introducing this new CHECKPOINT_REDO record we are explaining the
problem
that the redo location and the actual checkpoint records are not at
the same place and that is because of the concurrent xlog insertion.
I think we are explaining in more
detail by also stating that in case of a shutdown checkpoint, there
would not be any concurrent insertion so the shutdown checkpoint redo
will be at the same place.  So I feel keeping old comments is not
required.  And we are explaining it when we are setting this for
non-shutdown checkpoint because for shutdown checkpoint this statement
is anyway not correct because for the shutdown checkpoint the
checkpoint record will be at the same location and there will be no
concurrent wal insertion, what do you think?

[1]
+ /*
+ * Insert a dummy CHECKPOINT_REDO record and set start LSN of this record
+ * as checkpoint.redo.  Although we have the checkpoint record that also
+ * contains the exact redo lsn, there might have been some other records
+ * those got inserted between the redo lsn and the actual checkpoint
+ * record.  So when processing the wal, we cannot rely on the checkpoint
+ * record if we want to stop at the checkpoint-redo LSN.
+ *
+ * This special record, however, is not required when we are doing a
+ * shutdown checkpoint, as there will be no concurrent wal insertions
+ * during that time.  So, the shutdown checkpoint LSN will be the same as
+ * checkpoint-redo LSN.
+ */

>
> How about adding a test in pg_walinspect?  There is an online
> checkpoint running there, so you could just add something like that
> to check that the REDO record is at the expected LSN stored in the
> control file:
> +-- Check presence of REDO record.
> +SELECT redo_lsn FROM pg_control_checkpoint() \gset
> +SELECT start_lsn = :'redo_lsn'::pg_lsn AS same_lsn, record_type
> +  FROM pg_get_wal_record_info(:'redo_lsn');
> --

Done, thanks.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com