Hi,
The global variable ThisTimeLineID is rather confusing. During
recovery, in the startup process, when we're reading a WAL record, it
is the timeline of the WAL record we are trying to read or have just
read, except when we're trying to read the initial checkpoint record,
when it's zero. In other processes, it's typically 0 throughout
recovery, but sometimes it's not, because for example
CreateRestartPoint temporarily sets it to the timeline from which
we're replaying WAL, since there's no other way to get
RemoveOldXlogFiles() to recycle files onto the right timeline, or
PreallocXlogFiles() to preallocate onto the right timeline. Similarly,
walreceiver and walsender find it convenient to make ThisTimeLineID
the timeline from which WAL is being replayed or at least the one from
which it was being replayed at last check. logicalfuncs.c and
slotfuncs.c set the global variable sometimes too, apparently just for
fun, as the value doesn't seem to get used for anything. In normal
running, once the startup process exits, the next call to
RecoveryInProgress() sets the value to the timeline into which new WAL
can be inserted. Note also that all of this is different from
XLogCtl->ThisTimeLineID, which is set at the end of recovery and thus,
in the startup process, generally different from ThisTImeLineID, but
in other processes, generally the same as ThisTimeLineID.
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. 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.
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.
Opinions welcome.
Thanks,
--
Robert Haas
EDB: http://www.enterprisedb.com