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. regards, Ajin Cherian Fujitsu Australia
v8-0001-Add-option-to-enable-two-phase-commits-in-pg_crea.patch
Description: Binary data