Subject: Re: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns

From: "Drouvot, Bertrand" <[email-masked]>
Date: Mon, 21 Feb 2022 11:29:51 +0100

Hi,

On 10/19/21 8:43 AM, Kyotaro Horiguchi wrote:
> At Tue, 19 Oct 2021 02:45:24 +0000, "[email-masked]" <[email-masked]> wrote in
>> On Thursday, October 14, 2021 11:21 AM Kyotaro Horiguchi <[email-masked]> wrote:
>>> It adds a call to ReorderBufferAssignChild but usually subtransactions are
>>> assigned to top level elsewherae.  Addition to that
>>> ReorderBufferCommitChild() called just later does the same thing.  We are
>>> adding the third call to the same function, which looks a bit odd.
>> It can be odd. However, we
>> have a check at the top of ReorderBufferAssignChild
>> to judge if the sub transaction is already associated or not
>> and skip the processings if it is.
> My question was why do we need to make the extra call to
> ReorerBufferCommitChild when XACT_XINFO_HAS_INVALS in spite of the
> existing call to the same fuction that unconditionally made.  It
> doesn't cost so much but also it's not free.
>
>>> And I'm not sure it is wise to mark all subtransactions as "catalog changed"
>>> always when the top transaction is XACT_XINFO_HAS_INVALS.
>> In order to avoid this,
>> can't we have a new flag (for example, in reorderbuffer struct) to check
>> if we start decoding from RUNNING_XACTS, which is similar to the first patch of [1]
>> and use it at DecodeCommit ? This still leads to some extra specific codes added
>> to DecodeCommit and this solution becomes a bit similar to other previous patches though.
> If it is somehow wrong in any sense that we add subtransactions in
> SnapBuildProcessRunningXacts (for example, we should avoid relaxing
> the assertion condition.), I think we would go another way.  Otherwise
> we don't even need that additional flag.  (But Sawadasan's recent PoC
> also needs that relaxation.)
>
> ASAICS, and unless I'm missing something (that odds are rtlatively
> high:p), we need the specially added subransactions only for the
> transactions that were running at passing the first RUNNING_XACTS,
> becuase otherwise (substantial) subtransactions are assigned to
> toplevel by the first record of the subtransaction.
>
> Before reaching consistency, DecodeCommit feeds the subtransactions to
> ReorderBufferForget individually so the subtransactions are not needed
> to be assigned to the top transaction at all. Since the
> subtransactions added by the first RUNNING_XACT are processed that
> way, we don't need in the first place to call ReorderBufferCommitChild
> for such subtransactions.
>
>> [1] - https://www.postgresql.org/message-id/81D0D8B0-E7C4-4999-B616-1E5004DBDCD2%40amazon.com
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
Just rebased (minor change in the contrib/test_decoding/Makefile) the 
last POC version linked to the CF entry as it was failing the CF bot.

Thanks

Bertrand
diff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile
index 36929dd97d..05c0c5a2f8 100644
--- a/contrib/test_decoding/Makefile
+++ b/contrib/test_decoding/Makefile
@@ -9,7 +9,7 @@ REGRESS = ddl xact rewrite toast permissions decoding_in_xact \
 	sequence
 ISOLATION = mxact delayed_startup ondisk_startup concurrent_ddl_dml \
 	oldest_xmin snapshot_transfer subxact_without_top concurrent_stream \
-	twophase_snapshot slot_creation_error
+	twophase_snapshot slot_creation_error catalog_change_snapshot
 
 REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
 ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
diff --git a/contrib/test_decoding/expected/catalog_change_snapshot.out b/contrib/test_decoding/expected/catalog_change_snapshot.out
new file mode 100644
index 0000000000..bc142fc384
--- /dev/null
+++ b/contrib/test_decoding/expected/catalog_change_snapshot.out
@@ -0,0 +1,44 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s0_init s0_begin s0_savepoint s0_truncate s1_checkpoint s1_get_changes s0_commit s0_begin s0_insert s1_checkpoint s1_get_changes s0_commit s1_get_changes
+step s0_init: SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'test_decoding');
+?column?
+--------
+init    
+(1 row)
+
+step s0_begin: BEGIN;
+step s0_savepoint: SAVEPOINT sp1;
+step s0_truncate: TRUNCATE tbl1;
+step s1_checkpoint: CHECKPOINT;
+step s1_get_changes: SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'include-xids', '0');
+data
+----
+(0 rows)
+
+step s0_commit: COMMIT;
+step s0_begin: BEGIN;
+step s0_insert: INSERT INTO tbl1 VALUES (1);
+step s1_checkpoint: CHECKPOINT;
+step s1_get_changes: SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'include-xids', '0');
+data                                   
+---------------------------------------
+BEGIN                                  
+table public.tbl1: TRUNCATE: (no-flags)
+COMMIT                                 
+(3 rows)
+
+step s0_commit: COMMIT;
+step s1_get_changes: SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'include-xids', '0');
+data                                                         
+-------------------------------------------------------------
+BEGIN                                                        
+table public.tbl1: INSERT: val1[integer]:1 val2[integer]:null
+COMMIT                                                       
+(3 rows)
+
+?column?
+--------
+stop    
+(1 row)
+
diff --git a/contrib/test_decoding/specs/catalog_change_snapshot.spec b/contrib/test_decoding/specs/catalog_change_snapshot.spec
new file mode 100644
index 0000000000..43c0b64289
--- /dev/null
+++ b/contrib/test_decoding/specs/catalog_change_snapshot.spec
@@ -0,0 +1,32 @@
+setup
+{
+    DROP TABLE IF EXISTS tbl1;
+    CREATE TABLE tbl1 (val1 integer, val2 integer);
+}
+
+teardown
+{
+    DROP TABLE tbl1;
+    SELECT 'stop' FROM pg_drop_replication_slot('isolation_slot');
+}
+
+session "s0"
+setup { SET synchronous_commit=on; }
+step "s0_init" { SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'test_decoding'); }
+step "s0_begin" { BEGIN; }
+step "s0_savepoint" { SAVEPOINT sp1; }
+step "s0_truncate" { TRUNCATE tbl1; }
+step "s0_insert" { INSERT INTO tbl1 VALUES (1); }
+step "s0_commit" { COMMIT; }
+
+session "s1"
+setup { SET synchronous_commit=on; }
+step "s1_checkpoint" { CHECKPOINT; }
+step "s1_get_changes" { SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'include-xids', '0'); }
+
+# For the transaction that TRUNCATEd the table tbl1, the last decoding decodes
+# only its COMMIT record, because it starts from the RUNNING_XACT record emitted
+# during the second checkpoint execution.  This transaction must be marked as
+# containing catalog changes during decoding the COMMIT record and the decoding
+# of the INSERT record must read the pg_class with the correct historic snapshot.
+permutation "s0_init" "s0_begin" "s0_savepoint" "s0_truncate" "s1_checkpoint" "s1_get_changes" "s0_commit" "s0_begin" "s0_insert" "s1_checkpoint" "s1_get_changes" "s0_commit" "s1_get_changes"
diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c
index 18cf931822..ade48bd71e 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -629,6 +629,25 @@ DecodeCommit(LogicalDecodingContext *ctx, XLogRecordBuffer *buf,
 		commit_time = parsed->origin_timestamp;
 	}
 
+	/*
+	 * Mark the top transaction and its subtransactions as containing catalog
+	 * changes, if the commit record has invalidation message.  This is necessary
+	 * for the case where we decode only the commit record of the transaction
+	 * that actually has done catalog changes.
+	 */
+	if (parsed->xinfo & XACT_XINFO_HAS_INVALS)
+	{
+		ReorderBufferXidSetCatalogChanges(ctx->reorder, xid, buf->origptr);
+
+		for (int i = 0; i < parsed->nsubxacts; i++)
+		{
+			ReorderBufferAssignChild(ctx->reorder, xid, parsed->subxacts[i],
+									 buf->origptr);
+			ReorderBufferXidSetCatalogChanges(ctx->reorder, parsed->subxacts[i],
+											  buf->origptr);
+		}
+	}
+
 	SnapBuildCommitTxn(ctx->snapshot_builder, buf->origptr, xid,
 					   parsed->nsubxacts, parsed->subxacts);