Re: minor bug

If we never expect getRecordTimestamp to fail, then why put it in the
if-condition?

getRecordTimestamp can fail if the record is not a restore point nor a
commit or abort record. A few lines before in the same function there is
this:

/* Otherwise we only consider stopping before COMMIT or ABORT records. */
if (XLogRecGetRmid(record) != RM_XACT_ID)
return false;

I guess that make sure getRecordTimestamp can never fail.

The way it is written in your patch invites it to be optimized out again.
The only thing that prevents it is the comment.

Why not

(void)getRecordTimestamp(record, &recordXtime);
if (recoveryTarget == RECOVERY_TARGET_TIME)
...

On Wed, Jan 18, 2023 at 9:03 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at> writes:
> > On Tue, 2023-01-17 at 10:32 -0500, Tom Lane wrote:
> >> I seem to recall that the original idea was to report the timestamp
> >> of the commit/abort record we are stopping at. Maybe my memory is
> >> faulty, but I think that'd be significantly more useful than the
> >> current behavior, *especially* when the replay stopping point is
> >> defined by something other than time.
> >> (Also, the wording of the log message suggests that that's what
> >> the reported time is supposed to be. I wonder if somebody messed
> >> this up somewhere along the way.)
>
> > recoveryStopTime is set to recordXtime (the time of the xlog record)
> > a few lines above that patch, so this is useful information if it is
> > present.
>
> Ah, but that only happens if recoveryTarget == RECOVERY_TARGET_TIME.
> Digging in the git history, I see that this did use to work as
> I remember: we always extracted the record time before printing it.
> That was accidentally broken in refactoring in c945af80c. I think
> the correct fix is more like the attached.
>
> regards, tom lane
>
>