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
