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
signature.asc
Description: PGP signature