Hello.
At Mon, 27 Jan 2020 18:06:27 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in
> Actually looking again, getRecordTimestamp is looking pretty strange.
> It looks much more natural by using nested switch/case blocks, as with
> this diff. I think the compiler does a better job this way too.
Agreed. Anyway I looked the latest version.
The aim of the patch seem reasonable. XLOG_END_OF_RECOVERY and
XLOG_XACT_PREPARE also have a timestamp but it doesn't help much. (But
could be included for consistency.)
The timestamp of a checkpoint record is the start time of a checkpoint
(and doesn't have subseconds part, but it's not a problem.). That
means that the timestamp is usually far behind the time at the record
has been inserted. As the result the stored timestamp can go back by a
significant internval. I don't think that causes an actual problem but
the movement looks wierd as the result of
pg_last_xact_replay_timestamp().
Asides from the backward movement, a timestamp from other than
commit/abort records in recvoeryLastXTime affects the following code.
xlog.c:7329: (similar code exists at line 9332)
> ereport(LOG,
> (errmsg("redo done at %X/%X",
> (uint32) (ReadRecPtr >> 32), (uint32) ReadRecPtr)));
> xtime = GetLatestXTime();
> if (xtime)
> ereport(LOG,
> (errmsg("last completed transaction was at log time %s",
> timestamptz_to_str(xtime))));
This code assumes (and the name GetLatestXTime() suggests, I first
noticed that here..) that the timestamp comes from commit/abort logs,
so otherwise it shows a wrong timestamp. We shouldn't update the
variable by other than that kind of records.
If (I don't think that comes true..) we set the timestamp from other
than that kind of record, the names and the comments of the functions
should be changed.
> /*
> * Save timestamp of latest processed commit/abort record.
> *
> * We keep this in XLogCtl, not a simple static variable, so that it can be
> * seen by processes other than the startup process. Note in particular
> * that CreateRestartPoint is executed in the checkpointer.
> */
> static void
> SetLatestXTime(TimestampTz xtime)
...
> /*
> * Fetch timestamp of latest processed commit/abort record.
> */
> TimestampTz
> GetLatestXTime(void)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center