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

On Fri, May 28, 2021 at 9:14 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Wed, May 26, 2021 at 6:53 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Tue, May 25, 2021 at 8:54 AM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > On Fri, May 21, 2021 at 6:43 PM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > > Fixed in v77-0001, v77-0002
> > >
> > > Attaching a new patch-set that rebases the patch, addresses review
> > > comments from Peter as well as a test failure reported by Tang. I've
> > > also added some new test case into patch-2 authored by Tang.
> >
> > Thanks for the updated patch, few comments:
> > 1)  Should "The end LSN of the prepare." be changed to "end LSN of the
> > prepare transaction."?
>
> No, this is the end LSN of the prepare. The prepare consists of multiple LSNs.
>
> > 2) Should the ";" be "," here?
> > +++ b/doc/src/sgml/catalogs.sgml
> > @@ -7639,6 +7639,18 @@ SCRAM-SHA-256$<replaceable><iteration
> > count></replaceable>:<replaceable>&l
> >
> >       <row>
> >        <entry role="catalog_table_entry"><para role="column_definition">
> > +       <structfield>subtwophasestate</structfield> <type>char</type>
> > +      </para>
> > +      <para>
> > +       State code:
> > +       <literal>d</literal> = two_phase mode was not requested, so is disabled;
> > +       <literal>p</literal> = two_phase mode was requested, but is
> > pending enablement;
> > +       <literal>e</literal> = two_phase mode was requested, and is enabled.
> > +      </para></entry>
>
> no, I think the ";" is correct here, connecting multiple parts of the sentence.
>
> >
> > 3) Should end_lsn be commit_end_lsn?
> > +       prepare_data->commit_end_lsn = pq_getmsgint64(in);
> > +       if (prepare_data->commit_end_lsn == InvalidXLogRecPtr)
> >                 elog(ERROR, "end_lsn is not set in commit prepared message");
> > +       prepare_data->prepare_time = pq_getmsgint64(in);
>
> Changed this.
>
> >
> > 4) This change is not required
> >
> > diff --git a/src/include/replication/pgoutput.h
> > b/src/include/replication/pgoutput.h
> > index 0dc460f..93c6731 100644
> > --- a/src/include/replication/pgoutput.h
> > +++ b/src/include/replication/pgoutput.h
> > @@ -29,5 +29,4 @@ typedef struct PGOutputData
> >         bool            messages;
> >         bool            two_phase;
> >  } PGOutputData;
> > -
>
> removed.
>
>
> >  #endif                                                 /* PGOUTPUT_H */
> >
> > 5) Will the worker receive commit prepared/rollback prepared as we
> > have skip logic to skip commit prepared / commit rollback in
> > pgoutput_rollback_prepared_txn and pgoutput_commit_prepared_txn:
> >
> > +        * It is possible that we haven't received the prepare because
> > +        * the transaction did not have any changes relevant to this
> > +        * subscription and was essentially an empty prepare. In which case,
> > +        * the walsender is optimized to drop the empty transaction and the
> > +        * accompanying prepare. Silently ignore if we don't find the prepared
> > +        * transaction.
> >          */
> > -       replorigin_session_origin_lsn = prepare_data.end_lsn;
> > -       replorigin_session_origin_timestamp = prepare_data.commit_time;
> > +       if (LookupGXact(gid, prepare_data.prepare_end_lsn,
> > +                                       prepare_data.prepare_time))
> > +       {
> >
> Commit prepared will be skipped if it happens in the same walsender's
> lifetime. But if the walsender restarts it no longer
> knows about the skipped prepare. In this case walsender will not skip
> the commit prepared. Hence, the logic for handling
> stray commit prepared in the apply worker.
>
>
> > 6) I'm not sure if we could add some tests for skip empty prepare
> > transactions, if possible add few tests.
>
> I've added a test case using pg_logical_slot_peek_binary_changes() for
> empty prepares
>  have a look.
>
> >
> > 7) We could add some debug level log messages for the transaction that
> > will be skipped.
>
> If this is for the test, I was able to add a test without debug messages.

The idea here is to include any debug logs which will help in
analyzing any bugs that we might get from an environment where debug
access might not be available.
Thanks for fixing the comments and posting an updated patch.

Regards,
Vignesh