On Mon, Dec 13, 2021 at 12:44 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> Hi all,
> (CCing some folks who worked on this area lately)
>
> The following sequence of commands generates an assertion failure, as
> of $subject:
> select pg_replication_origin_create('popo');
> select pg_replication_origin_session_setup('popo');
> begin;
> select txid_current();
> prepare transaction 'popo'; -- assertion fails
>
> The problem originates from 1eb6d65, down to 11, where we finish by
> triggering this assertion before replorigin_session_origin_lsn is not
> valid:
> + if (replorigin)
> + {
> + Assert(replorigin_session_origin_lsn != InvalidXLogRecPtr);
> + hdr->origin_lsn = replorigin_session_origin_lsn;
> + hdr->origin_timestamp = replorigin_session_origin_timestamp;
> + }
>
> As far as I understand this code and based on the docs,
> pg_replication_origin_xact_setup(), that would set of the session
> origin LSN and timestamp, is an optional choise.
> replorigin_session_advance() would be a no-op for remote_lsn, and
> local_lsn requires an update. Now please note that I am really fluent
> with this stuff, so feel free to correct me. The intention of the
> code also seems that XACT_XINFO_HAS_ORIGIN should still be set, but
> with no data.
>
> At the end, it seems to me that the assertion could just be dropped as
> per the attached, as we'd finish by setting up origin_lsn and
> origin_timestamp in the 2PC file header with some invalid data.
Why do we check if replorigin_session_origin_lsn is not invalid data
only when PREPARE TRANSACTION? 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].
Regards,
[1] https://www.postgresql.org/message-id/CAD21AoAMuTrXezGqaV1Q5H-Hf%2BzKqGbU8XuCZk9iQMYueF3%2B8w%40mail.gmail.com
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/