Re: [HACKERS] logical decoding of two-phase transactions

On Fri, Jun 18, 2021 at 3:37 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Jun 18, 2021 at 7:43 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > On Thu, Jun 17, 2021 at 6:22 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> > >
> > > 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.
> >
>
> I think on similar lines below asserts also need to be changed.
>
> 1.
> +static void
> +apply_handle_begin_prepare(StringInfo s)
> +{
> + LogicalRepPreparedTxnData begin_data;
> + char gid[GIDSIZE];
> +
> + /* Tablesync should never receive prepare. */
> + Assert(!am_tablesync_worker());
>
> 2.
> +static void
> +TwoPhaseTransactionGid(Oid subid, TransactionId xid, char *gid, int szgid)
> +{
> ..
> + Assert(TransactionIdIsValid(xid));
>
> 3.
> +static void
> +apply_handle_stream_prepare(StringInfo s)
> +{
> + int nchanges = 0;
> + LogicalRepPreparedTxnData prepare_data;
> + TransactionId xid;
> + char gid[GIDSIZE];
> +
> ..
> ..
> +
> + /* Tablesync should never receive prepare. */
> + Assert(!am_tablesync_worker());
>

Please find attached the latest patch set v88*

Differences from v87* are:

* Rebased to HEAD @ today.

* Replaces several protocol Asserts with ereports
(ERRCODE_PROTOCOL_VIOLATION) in patch 0003 and 0004, as reported by
Greg [1] and Amit [2]. This is in keeping with the commit [3].

----
[1] https://www.postgresql.org/message-id/CAHut%2BPuJKTNRjFre0VBufWMz9BEScC__nT%2BPUhbSaUNW2biPow%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAA4eK1JO3HsOurS988%3DJarej%3DAK6ChE1tLuMNP%3DAZCt6--hVrw%40mail.gmail.com
[3] https://github.com/postgres/postgres/commit/fe6a20ce54cbbb6fcfe9f6675d563af836ae799a

Kind Regards,
Peter Smith.
Fujitsu Australia