From: | Ajin Cherian <itsajin(at)gmail(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: repeated decoding of prepared transactions |
Date: | 2021-03-02 01:07:06 |
Message-ID: | CAFPTHDaZBoj-yxPp5t8P2zG0rDCwa_5X9R8Qs4k41K-jwOxi6g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Mar 1, 2021 at 8:08 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 | Content-Type | Size |
---|---|---|
v8-0001-Add-option-to-enable-two-phase-commits-in-pg_crea.patch | application/octet-stream | 42.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Thomas Munro | 2021-03-02 01:10:20 | Re: PostmasterIsAlive() in recovery (non-USE_POST_MASTER_DEATH_SIGNAL builds) |
Previous Message | Thomas Munro | 2021-03-02 00:40:11 | Re: Why does the BF sometimes not dump regression.diffs? |