Re: removing global variable ThisTimeLineID

On Tue, Nov 2, 2021 at 12:46 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> [...]
> 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.
>

I had a plan to look at these patches closely, but unfortunately,
didn't get enough time; and might not be able to spend time in the
rest of this week. Here are a few thoughts that I had in the initial
go-through, but that may or may not sound very interesting:

0002:
-GetFlushRecPtr(void)
+GetFlushRecPtr(TimeLineID *insertTLI)

Should we rename this argument to more generic as "tli", like
GetStandbyFlushRecPtr, since the caller in 003 patch trying to fetch a
TLI that means different for them, e.g. currTLI, FlushTLI, etc.


0004:
 static int
-XLogFileInitInternal(XLogSegNo logsegno, bool *added, char *path)
+XLogFileInitInternal(XLogSegNo logsegno, TimeLineID logtli,
+                    bool *added, char *path)

 int
-XLogFileInit(XLogSegNo logsegno)
+XLogFileInit(XLogSegNo logsegno, TimeLineID logtli)
 {

How about logsegtli instead, to be inlined with logsegno ?


0007:
 static XLogSegNo openLogSegNo = 0;
+static TimeLineID openLogTLI = 0;

Similarly, openLogSegTLI ?


0008:
+   /*
+    * Given that we're not in recovery, ThisTimeLineID is set and can't
+    * change, so we can read it without a lock.
+    */
+   insertTLI = XLogCtl->ThisTimeLineID;

Can't GetWALInsertionTimeLine() called instead? I guess the reason is
to avoid the unnecessary overhead involved in the function call. (Same
at a few other places.)

Regards,
Amul