Re: repeated decoding of prepared transactions

On Mon, Mar 1, 2021 at 7:23 AM Ajin Cherian <itsajin(at)gmail(dot)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.