Re: [HACKERS] logical decoding of two-phase transactions - Mailing list pgsql-hackers

From Peter Smith
Subject Re: [HACKERS] logical decoding of two-phase transactions
Date
Msg-id CAHut+PuJKTNRjFre0VBufWMz9BEScC__nT+PUhbSaUNW2biPow@mail.gmail.com
Whole thread Raw
In response to Re: [HACKERS] logical decoding of two-phase transactions  (Greg Nancarrow <gregn4422@gmail.com>)
Responses Re: [HACKERS] logical decoding of two-phase transactions
List pgsql-hackers
On Thu, Jun 17, 2021 at 6:22 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Wed, Jun 16, 2021 at 9:08 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> >
> > Please find attached the latest patch set v86*
> >
>
> A couple of comments:
>
> (1)  I think one of my suggested changes was missed (or was that intentional?):
>
> BEFORE:
> +                The LSN of the commit prepared.
> AFTER:
> +                The LSN of the commit prepared transaction.
>

No, not missed. I already dismissed that one and wrote about it when I
posted v85 [1].

>
> (2)  In light of Tom Lane's recent changes in:
>
> fe6a20ce54cbbb6fcfe9f6675d563af836ae799a (Don't use Asserts to check
> for violations of replication protocol)
>
> there appear to be some instances of such code in these patches.

Yes, I already noted [2] there are likely to be such cases which need
to be fixed.

>
> For example, in the v86-0001 patch:
>
> +/*
> + * Handle PREPARE message.
> + */
> +static void
> +apply_handle_prepare(StringInfo s)
> +{
> + LogicalRepPreparedTxnData prepare_data;
> + char gid[GIDSIZE];
> +
> + logicalrep_read_prepare(s, &prepare_data);
> +
> + Assert(prepare_data.prepare_lsn == remote_final_lsn);
>
> The above Assert() should be changed to something like:
>
> +    if (prepare_data.prepare_lsn != remote_final_lsn)
> +        ereport(ERROR,
> +                (errcode(ERRCODE_PROTOCOL_VIOLATION),
> +                 errmsg_internal("incorrect prepare LSN %X/%X in
> prepare message (expected %X/%X)",
> +                                 LSN_FORMAT_ARGS(prepare_data.prepare_lsn),
> +                                 LSN_FORMAT_ARGS(remote_final_lsn))));
>
> Without being more familiar with this code, it's difficult for me to
> judge exactly how many of such cases are in these patches.

Thanks for the above example. I will fix this one later, after
receiving some more reviews and reports of other Assert cases just
like this one.

------
[1] https://www.postgresql.org/message-id/CAHut%2BPvOVkiVBf4P5chdVSoVs5%3Da%3DF_GtTSHHoXDb4LiOM_8Qw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAHut%2BPvdio4%3DOE6cz5pr8VcJNcAgt5uGBPdKf-tnGEMa1mANGg%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia



pgsql-hackers by date:

Previous
From: Ranier Vilela
Date:
Subject: Re: Avoid call MaintainOldSnapshotTimeMapping, if old_snapshot_threshold is disabled.
Next
From: Tom Lane
Date:
Subject: Re: Centralizing protective copying of utility statements