On Tue, Nov 02, 2021 at 08:45:57AM -0400, Robert Haas wrote:
> Also, XLogCtl->ThisTimeLineID isn't set until the end of recovery, so
> calling this function during recovery would be a mistake. There seem
> to be a number of interface functions in xlog.c that should only be
> called when not in recovery, and neither their names nor their
> comments make that clear. I wasn't bold enough to go label all the
> ones I think fall into that category, but maybe we should.
I got to wonder whether it would be better to add in GetFlushRecPtr()
the same assertion as GetWALInsertionTimeLine(). All the in-core
callers of this routine already assume that, and it would be buggy if
one passes down insertTLI to get the current TLI.
> I thought about that, but I think it's pointless. I think we can just
> say that if openLogFile == -1, openLogSegNo and openLogTLI are
> meaningless. I don't think we're currently resetting openLogSegNo to
> an invalid value either, so I don't think openLogTLI should be treated
> differently.
Wouldn't it be better to at least document that as a comment rather
than letting the reader guess it, then? I agree that this is a minor
point, though.
> I'm not opposed to introducing InvalidTimeLineID on
> principle, and I looked for a way of doing it in this patch set, but
> it didn't seem all that valuable, and I feel that someone who cares
> about it can do it as a separate effort after I commit these.
No issues from me. I have bumped into a case just at the end of last
week where I wanted a better way to tell if a TLI is invalid rather
than leaving things to 0, FWIW.
>> The handling of XLogCtl->ThisTimeLineID still seems a bit blurry when
>> it comes to timeline switches because we don't update it while in
>> recovery when replaying a end-of-recovery record. This could at least
>> be documented better.
>
> We don't update it during recovery at all. There's exactly one
> assignment statement for that variable and it's here:
Indeed. It looks like my reading was sloppy here.
--
Michael