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.
+ * 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".
- 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?
- * 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?
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');
--
Michael
