Re: removing global variable ThisTimeLineID

On Mon, Nov 01, 2021 at 03:16:27PM -0400, Robert Haas wrote:
> At the risk of stating the obvious, using the same variable for
> different purposes at different times is not a great programming
> practice. Commits 2f5c4397c39dea49c5608ba583868e26d767fc32 and
> 902a2c280012557b85c7e0fce3f6f0e355cb2d69 show that the possibility of
> programmer error is not zero, even though neither of those issues are
> serious.

This can lead to more serious issues, see 595b9cba as recent example.
I agree that getting rid of it in the long term is appealing.

> Moreover, the global variable itself seems to do nothing but
> invite programming errors. The name itself is a disaster. What is
> "this" timeline ID? Well, uh, the right one, of course! We should be
> more clear about what we mean: the timeline into which we are
> inserting, the timeline from which we are replaying, the timeline from
> which we are performing logical decoding. I suspect that having a
> global variable here isn't even really saving us anything much, as a
> value that does not change can be read from shared memory without a
> lock.

Some callbacks related to the WAL reader to read a patch have
introduced a dependency to ThisTimelineID as a mean to be used for
logical decoding on standbys. This is discussed a bit on this thread,
for example:
https://www.postgresql.org/message-id/ddc31aa9-8083-58b7-0b47-e371cd4c705b@oss.nttdata.com

Tracking properly the TLI in logical decoding was mentioned by Andres
multiple times on -hackers, and this has not been really done. Using
ThisTimelineID for this purpose is something I've found confusing
myself, FWIW, so this is a step in the good direction.

> I would like to clean this up. Attached is a series of patches which
> try to do that. For ease of review, this is separated out into quite a
> few separate patches, but most likely we'd combine some of them for
> commit. Patches 0001 through 0004 eliminate all use of the global
> variable ThisTimeLineID outside of xlog.c, and patch 0005 makes it
> "static" so that it ceases to be visible outside of xlog.c. Patches
> 0006 to 0010 clean up uses of ThisTimeLineID itself, passing it around
> as a function parameter, or otherwise arranging to fetch the relevant
> timeline ID from someplace sensible rather than using the global
> variable as a scratchpad. Finally, patch 0011 deletes the global
> variable.
>
> I have not made a serious attempt to clear up all of the
> terminological confusion created by the term ThisTimeLineID, which
> also appears as part of some structs, so you'll still see that name in
> the source code after applying these patches, just not as the name of
> a global variable. I have, however, used names like insertTLI and
> replayTLI in many places changed by the patch, and I think that makes
> it significantly more clear which timeline is being discussed in each
> case. In some places I have endeavored to improve the comments. There
> is doubtless more that could be done here, but I think this is a
> fairly respectable start.

Thanks for splitting things this way. This helps a lot.

0001 looks fair.

+ /*
+ * If we're writing and flushing WAL, the time line can't be changing,
+ * so no lock is required.
+ */
+ if (insertTLI)
+ *insertTLI = XLogCtl->ThisTimeLineID;
In 0002, there is no downside in putting that in the spinlock section
either, no? It seems to me that we should be able to call this one
even in recovery, as flush LSNs exist while in recovery.

+TimeLineID
+GetWALInsertionTimeLine(void)
+{
+ Assert(XLogCtl->SharedRecoveryState == RECOVERY_STATE_DONE);
Okay, that's cheaper.

The series 0003-0006 seemed rather fine, at quick glance.

In 0007, XLogFileClose() should reset openLogTLI. The same can be
said in BootStrapXLOG() before creating the control file. It may be
cleaner here to introduce an InvalidTimelineId, as discussed a couple
of days ago.

Some comments at the top of recoveryTargetTLIRequested need a refresh
with their references to ThisTimelineID.

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.
--
Michael