Laurenz Albe <laurenz.albe@cybertec.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
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 5e65785306..c14d1f3ef6 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -2548,8 +2548,13 @@ recoveryStopsBefore(XLogReaderState *record)
stopsHere = (recordXid == recoveryTargetXid);
}
- if (recoveryTarget == RECOVERY_TARGET_TIME &&
- getRecordTimestamp(record, &recordXtime))
+ /*
+ * Note: we must fetch recordXtime regardless of recoveryTarget setting.
+ * We don't expect getRecordTimestamp ever to fail, since we already know
+ * this is a commit or abort record; but test its result anyway.
+ */
+ if (getRecordTimestamp(record, &recordXtime) &&
+ recoveryTarget == RECOVERY_TARGET_TIME)
{
/*
* There can be many transactions that share the same commit time, so