Re: recovery_target immediate timestamp - Mailing list pgsql-hackers

On 2020/11/13 8:39, Euler Taveira wrote:
> On Wed, 11 Nov 2020 at 22:40, Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>> wrote:
> 
> 
>     On 2020/11/12 6:00, Euler Taveira wrote:
> 
>      > The first patch adds a new message that prints the latest completed checkpoint
>      > when the consistent state is reached.
> 
>     I'm not sure how useful this information is in practice.
> 
> Fujii, thanks for reviewing it. It provides the same information as the "last
> completed transaction was" message.
> 
>      > It also exposes the checkpoint timestamp
>      > in debug messages.
> 
>                              ereport(DEBUG1,
>                                              (errmsg("checkpoint record is at %X/%X",
>                                                              (uint32) (checkPointLoc >> 32), (uint32)
checkPointLoc)));
>     +                       ereport(DEBUG1,
>     +                                       (errmsg("checkpoint time is %s", str_time(checkPoint.time))));
> 
>     The above first debug message displays the LSN of the checkpoint record.
>     OTOH, the second message displays the time when the checkpoint started
>     (not the time when checkpoint record was written at the end of checkpoint).
>     So isn't it confusing to display those inconsistent information together?
> 
> Indeed the checkpoint timestamp is from before it determines the REDO LSN. Are
> you saying the this checkpoint timestamp is inconsistent because it is not near
> the point it saves the RedoRecPtr? If so, let's move checkPoint.time a few
> lines below.

No. What I'd like to say is; checkPointLoc that the first debug message
outputs is the LSN of checkpoint record, not the checkpoint REDO location.
The checkpoint REDO location is determined at the early stage of
checkpointing. OTOH, the location of checkpoint record is determined
at the end of checkpointing. They are different.

The checkpoint time that the second debug message you added outputs is
the timestamp determined at the beginning of checkpointing. So it seems
not reasonable to display the location of checkpoint record and
the checkpoint time because they are determined at the different timing.
Am I missing something?


> 
>      /*
>       * Here we update the shared RedoRecPtr for future XLogInsert calls; this
>       * must be done while holding all the insertion locks.
>       *
>       * Note: if we fail to complete the checkpoint, RedoRecPtr will be left
>       * pointing past where it really needs to point.  This is okay; the only
>       * consequence is that XLogInsert might back up whole buffers that it
>       * didn't really need to.  We can't postpone advancing RedoRecPtr because
>       * XLogInserts that happen while we are dumping buffers must assume that
>       * their buffer changes are not included in the checkpoint.
>       */
>      RedoRecPtr = XLogCtl->Insert.RedoRecPtr = checkPoint.redo;
>      checkPoint.time = (pg_time_t) time(NULL);
> 
> I realized that I was using the wrong variable in one of the debug messages.
> 
>      > The second patch provides the checkpoint timestamp in the pg_waldump output and
>      > also when you enable wal_debug parameter. The pg_waldump output looks like
> 
>     +1
> 
>     +#ifdef FRONTEND
>     +               strftime(checkpointstr, sizeof(checkpointstr), "%c", localtime(&time_tmp));
>     +#else
>     +               pg_strftime(checkpointstr, sizeof(checkpointstr), "%c", pg_localtime(&time_tmp, log_timezone));
>     +#endif
> 
>     You can simplify the code by using timestamptz_to_str() here instead, like xact_desc_commit() does.
> 
> I have the same idea until I realized that checkPoint.time is pg_time_t and not
> TimestampTz. [digging the code a bit...] I figure out there is a function that
> converts from pg_time_t to TimestampTz: time_t_to_timestamptz(). I removed that
> ugly code but have to duplicate this function into compat.c. I don't have a
> strong preference but I'm attaching a new patch.

Thanks for updating the patch! At least for me this approach looks better.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION