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

On Tue, Jun 8, 2021 at 4:19 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, Jun 3, 2021 at 7:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Jun 2, 2021 at 4:34 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > Please find attached the latest patch set v82*
> > >

Attaching patchset-v84 that addresses some of Amit's and Vignesh's comments:
This patch-set also modifies the test case added for copy_data = false
to check that two-phase
transactions are decoded correctly.

> > 2.
> > @@ -85,11 +85,16 @@ typedef struct LogicalDecodingContext
> >   bool streaming;
> >
> >   /*
> > - * Does the output plugin support two-phase decoding, and is it enabled?
> > + * Does the output plugin support two-phase decoding.
> >   */
> >   bool twophase;
> >
> >   /*
> > + * Is two-phase option given by output plugin?
> > + */
> > + bool twophase_opt_given;
> > +
> > + /*
> >   * State for writing output.
> >
> > I think we can write few comments as to why we need a separate
> > twophase parameter here? The description of twophase_opt_given can be
> > changed to: "Is two-phase option given by output plugin? This is to
> > allow output plugins to enable two_phase at the start of streaming. We
> > can't rely on twophase parameter that tells whether the plugin
> > provides all the necessary two_phase APIs for this purpose." Feel free
> > to add more to it.
> >
>
> TODO

Added comments here.

> > 3.
> > @@ -432,10 +432,19 @@ CreateInitDecodingContext(const char *plugin,
> >   MemoryContextSwitchTo(old_context);
> >
> >   /*
> > - * We allow decoding of prepared transactions iff the two_phase option is
> > - * enabled at the time of slot creation.
> > + * We allow decoding of prepared transactions when the two_phase is
> > + * enabled at the time of slot creation, or when the two_phase option is
> > + * given at the streaming start.
> >   */
> > - ctx->twophase &= MyReplicationSlot->data.two_phase;
> > + ctx->twophase &= (ctx->twophase_opt_given || slot->data.two_phase);
> > +
> > + /* Mark slot to allow two_phase decoding if not already marked */
> > + if (ctx->twophase && !slot->data.two_phase)
> > + {
> > + slot->data.two_phase = true;
> > + ReplicationSlotMarkDirty();
> > + ReplicationSlotSave();
> > + }
> >
> > Why do we need to change this during CreateInitDecodingContext which
> > is called at create_slot time? At that time, we don't need to consider
> > any options and there is no need to toggle slot's two_phase value.
> >
> >
>
> TODO

As part of the recent changes, we do turn on two_phase at create_slot time when
the subscription is created with (copy_data = false, two_phase = on).
So, this code is required.

Amit:

"1.
-   <term><literal>CREATE_REPLICATION_SLOT</literal> <replaceable
class="parameter">slot_name</replaceable> [
<literal>TEMPORARY</literal> ] { <literal>PHYSICAL</literal> [
<literal>RESERVE_WAL</literal> ] | <literal>LOGICAL</literal>
<replaceable class="parameter">output_plugin</replaceable> [
<literal>EXPORT_SNAPSHOT</literal> |
<literal>NOEXPORT_SNAPSHOT</literal> | <literal>USE_SNAPSHOT</literal>
] }
+   <term><literal>CREATE_REPLICATION_SLOT</literal> <replaceable
class="parameter">slot_name</replaceable> [
<literal>TEMPORARY</literal> ] [ <literal>TWO_PHASE</literal> ] {
<literal>PHYSICAL</literal> [ <literal>RESERVE_WAL</literal> ] |
<literal>LOGICAL</literal> <replaceable
class="parameter">output_plugin</replaceable> [
<literal>EXPORT_SNAPSHOT</literal> |
<literal>NOEXPORT_SNAPSHOT</literal> | <literal>USE_SNAPSHOT</literal>
] }

Can we do some testing of the code related to this in some way? One
random idea could be to change the current subscriber-side code just
for testing purposes to see if this works. Can we enhance and use
pg_recvlogical to test this? It is possible that if you address
comment number 13 below, this can be tested with Create Subscription
command."

Actually this is tested in the test case added when Create
Subscription with (copy_data = false) because in that case
the slot is created with the two-phase option.

Vignesh's comment:

"We could add some debug level log messages for the transaction that
will be skipped."

Updated debug messages.


regards,
Ajin Cherian
Fujitsu Australia