Re: removing global variable ThisTimeLineID

On Wed, Nov 3, 2021 at 9:12 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> 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.

It's possible that more consistency would be better here, but I don't
think making this name more generic is going in the right direction.
If somebody gets the TLI using this function, it's the timeline into
which a system not in recovery is inserting WAL, which is the same
timeline to which WAL is being flushed. I think it's important for
this name to try to make that clear. It doesn't really matter if
someone treats the insertTLI as the writeTLI or the flushTLI since on
a system in production they're all the same - but they must not
confuse it with, say, the replayTLI.

> How about logsegtli instead, to be inlined with logsegno ?
> Similarly, openLogSegTLI ?

Hmm, well I guess it depends on how you think the words group. If you
logsegno means "the number of the log segment" then it would make
sense to have logsegli to mean "the tli of the log segment." But I
think of logsegno as meaning "the segment number of the log file," so
it makes more sense to have logtli to mean "the TLI of the log file".
I think it's definitely not a "log segment file" - just a "log file".

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

That, plus to avoid failing the assertion in that function. As we've
discussed on the ALTER SYSTEM READ ONLY thread, xlog.c itself inserts
some WAL before recovery is officially over.

--
Robert Haas
EDB: http://www.enterprisedb.com