On Mon, Mar 1, 2021 at 7:23 AM Ajin Cherian <itsa...@gmail.com> wrote: > Few minor comments on 0002 patch ============================= 1. ctx->streaming &= enable_streaming; - ctx->twophase &= enable_twophase; + }
Spurious line addition. 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. 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. 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. -- With Regards, Amit Kapila.