Re: PITR promote bug: Checkpointer writes to older timeline

On Mon, Mar 15, 2021 at 04:38:08PM +0900, Michael Paquier wrote:
> On Mon, Mar 15, 2021 at 03:01:09PM +0900, Kyotaro Horiguchi wrote:
>> Logical decoding stuff is (I think) designed to turn any backend into
>> a walsender, which may need to maintain ThisTimeLineID.  It seems to
>> me that logical decoding stuff indents to maintain ThisTimeLineID of
>> such backends at reading a WAL record. logical_read_xlog_page also
>> updates ThisTimeLineID and pg_logical_slot_get_changes_guts(),
>> pg_replication_slot_advance() (and maybe other functions) updates
>> ThisTimeLineID. So it is natural that local_read_xlog_page() updates
>> it since it is intended to be used used in logical decoding plugins.
>
> Logical decoding contexts cannot be created while in recovery as per
> CheckLogicalDecodingRequirements(), and as mentioned not everything
> is
> in place to allow that.  FWIW, I think that it is just confusing for
> pg_replication_slot_advance() and pg_logical_slot_get_changes_guts()
> to update it, and we just look for the latest value each time it is
> necessary when reading a new WAL page.

Studying some history today, having read_local_xlog_page() directly
update ThisTimeLineID has been extensively discussed here back in 2017
to attempt to introduce logical decoding on standbys (1148e22a):
https://www.postgresql.org/message-id/CAMsr%2BYEVmBJ%3DdyLw%3D%2BkTihmUnGy5_EW4Mig5T0maieg_Zu%3DXCg%40mail.gmail.com

Currently with HEAD and back branches, nothing would be broken as
logical contexts cannot exist in recovery.  Still it would be easy
to miss the new behavior for anybody attempting to work more on this
feature in the future if we change read_local_xlog_page to not update
ThisTimeLineID anymore.  Resetting the value of ThisTimeLineID in
read_local_xlog_page() does not seem completely right either with this
argument, as they could be some custom code relying on the existing
behavior of read_local_xlog_page() to maintain ThisTimeLineID.

Hmmm.  I am wondering whether the best answer for the moment would not
be to save and reset ThisTimeLineID just in XlogReadTwoPhaseData(), as
that's the local change that uses read_local_xlog_page().

The state of the code is really confusing on HEAD, and I'd like to
think that the best thing we could do in the long-term is to make the
logical decoding path not rely on ThisTimeLineID at all and decouple
all that, putting the code in a state sane enough so as we don't
finish with similar errors as what is discussed on this thread.  That
would be a work for a different patch though, not for stable
branches.  And seeing some slot and advancing functions update
directly ThisTimeLineID is no good either..

Any thoughts?
--
Michael