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

Attachment: v8-0001-Add-option-to-enable-two-phase-commits-in-pg_crea.patch
Description: Binary data

Reply via email to