On Tue, Mar 2, 2021 at 6:37 AM Ajin Cherian <itsa...@gmail.com> wrote: > > On Mon, Mar 1, 2021 at 8:08 PM Amit Kapila <amit.kapil...@gmail.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