Re: removing global variable ThisTimeLineID

On Mon, Nov 8, 2021 at 8:27 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Perhaps. That's related to the point I made before, that it might be a
> good idea to be more clear about which of these functions are intended
> to be used in recovery and which ones are intended to be used in
> normal running. I don't rule out the possibility of doing some more
> research here and tightening that up, but I don't want to go start
> tweaking things unless I'm sure I know what I'm talking about. This
> stuff is so confusing that I don't want to take any risk of making
> changes that turn out to be incorrect.

Well I went through and it seems to be OK: all the existing callers of
that function first verify that we're not in recovery. The patch to
make logical decoding work in standby mode might change that, but as
of now it's so. So in the attached patch, I have added the assertion
and comments that you have proposed there. I also removed the
misleading comment we discussed before. In addition to that, I renamed
the local variable ThisTimeLineID to replayTLI, and I also renamed
XLogCtl->ThisTimeLineID to InsertTimeLineID, which I think should make
things clearer: imagine if things that were used for different
purposes had different names!

Even with this patch, the name ThisTimeLineID is still used for
multiple purposes. It remains part of the CheckPoint struct, and also
part of the xl_end_of_recovery struct. But in both of those cases, the
name ThisTimeLineID actually makes sense, because what we're thinking
about is whether there's a timeline change, so we have ThisTimeLineID
and PrevTimeLineID and it seems fairly clear what that's supposed to
mean.

Thoughts?

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