Re: repeated decoding of prepared transactions

On Tue, Mar 2, 2021 at 6:37 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Mon, Mar 1, 2021 at 8:08 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> > Few minor comments on 0002 patch
> > =============================
> > 1.
> > ctx->streaming &= enable_streaming;
> > - ctx->twophase &= enable_twophase;
> > +
> > }
> >
> > Spurious line addition.
>
> Deleted.
>
> >
> > 2.
> > - proallargtypes =>
> > '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8}',
> > - proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o}',
> > - proargnames =>
> > '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size}',
> > + proallargtypes =>
> > '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool}',
> > + proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
> > + proargnames =>
> > '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,twophase}',
> > prosrc => 'pg_get_replication_slots' },
> > { oid => '3786', descr => 'set up a logical replication slot',
> > proname => 'pg_create_logical_replication_slot', provolatile => 'v',
> > - proparallel => 'u', prorettype => 'record', proargtypes => 'name name bool',
> > - proallargtypes => '{name,name,bool,name,pg_lsn}',
> > - proargmodes => '{i,i,i,o,o}',
> > - proargnames => '{slot_name,plugin,temporary,slot_name,lsn}',
> > + proparallel => 'u', prorettype => 'record', proargtypes => 'name
> > name bool bool',
> > + proallargtypes => '{name,name,bool,bool,name,pg_lsn}',
> > + proargmodes => '{i,i,i,i,o,o}',
> > + proargnames => '{slot_name,plugin,temporary,twophase,slot_name,lsn}',
> >
> > I think it is better to use two_phase here and at other places as well
> > to be consistent with similar parameters.
>
> Updated as requested.
> >
> > 3.
> > --- a/src/backend/catalog/system_views.sql
> > +++ b/src/backend/catalog/system_views.sql
> > @@ -894,7 +894,8 @@ CREATE VIEW pg_replication_slots AS
> > L.restart_lsn,
> > L.confirmed_flush_lsn,
> > L.wal_status,
> > - L.safe_wal_size
> > + L.safe_wal_size,
> > + L.twophase
> > FROM pg_get_replication_slots() AS L
> >
> > Indentation issue. Here, you need you spaces instead of tabs.
>
> Updated.
> >
> > 4.
> > @@ -533,6 +533,12 @@ CreateDecodingContext(XLogRecPtr start_lsn,
> >
> > ctx->reorder->output_rewrites = ctx->options.receive_rewrites;
> >
> > + /*
> > + * If twophase is set on the slot at create time, then
> > + * make sure the field in the context is also updated.
> > + */
> > + ctx->twophase &= MyReplicationSlot->data.twophase;
> > +
> >
> > Why didn't you made similar change in CreateInitDecodingContext when I
> > already suggested the same in my previous email? If we don't make that
> > change then during slot initialization two_phase will always be true
> > even though user passed in as false. It looks inconsistent and even
> > though there is no direct problem due to that but it could be cause of
> > possible problem in future.
>
> Updated.
>

I have a minor comment regarding the below:
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>two_phase</structfield> <type>bool</type>
+ </para>
+ <para>
+ True if two-phase commits are enabled on this slot.
+ </para></entry>
+ </row>

Can we change something like:
True if the slot is enabled for decoding prepared transaction
information. Refer link for more information.(link should point where
more detailed information is available for two-phase in
pg_create_logical_replication_slot).

Also there is one small indentation in that line, I think there should
be one space before "True if....".

Regards,
Vignesh