Re: Assertion failure with replication origins and PREPARE TRANSACTIOn

On Mon, Dec 13, 2021 at 2:00 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Mon, Dec 13, 2021 at 04:30:36PM +0900, Masahiko Sawada wrote:
> > Why do we check if replorigin_session_origin_lsn is not invalid data
> > only when PREPARE TRANSACTION?
>
> Well, it does not matter for the case of PREPARE TRANSACTION, does it?
> we would include values for the the origin LSN and timestamp in
> any case as these are fixed in the 2PC file header.
>
> > Looking at commit and rollback code, we
> > don't have assertions or checks that check
> > replorigin_session_origin_lsn/timestamp is valid data. So it looks
> > like we accept also invalid data in those cases since
> > replorigin_advance doesn’t move LSN backward while applying a commit
> > or rollback record even if LSN is invalid. The same is true for
> > PREPARE records, i.g., even if replication origin LSN is invalid, it
> > doesn't go backward. If replication origin LSN and timestamp in commit
> > record and rollback record must be valid data too, I think we should
> > similar checks for commit and rollback code and I think the assertions
> > will fail in the case I reported before[1].
>
> It seems to me that the origin LSN and timestamp are optional, so as
> one may choose to not call pg_replication_origin_xact_setup() (as said
> in my first message), and we would not require more sanity checks when
> advancing the replication origin in the commit and rollback code
> paths.
>

This is my understanding as well. I think here the point of Sawada-San
is why to have additional for replorigin_session_origin_lsn in prepare
code path? I think the way you have it in your patch is correct as
well but it is probably better to keep the check only based on
replorigin so as to keep this check consistent in all code paths.

--
With Regards,
Amit Kapila.