public inbox for [email protected]
help / color / mirror / Atom feedFrom: Drouvot, Bertrand <[email protected]>
To: Kyotaro Horiguchi <[email protected]>
To: [email protected]
To: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns
Date: Mon, 21 Feb 2022 11:29:51 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CAD21AoD00wV4gt-53ze+ZB8n4bqJrdH8J_UnDHddy8S2A+a25g@mail.gmail.com>
<[email protected]>
<OSBPR01MB48886D77AC8AD25B16C28B5CEDBD9@OSBPR01MB4888.jpnprd01.prod.outlook.com>
<[email protected]>
Hi,
On 10/19/21 8:43 AM, Kyotaro Horiguchi wrote:
> At Tue, 19 Oct 2021 02:45:24 +0000, "[email protected]" <[email protected]> wrote in
>> On Thursday, October 14, 2021 11:21 AM Kyotaro Horiguchi <[email protected]> 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);
Attachments:
[text/plain] poc_mark_all_txns_catalog_change.patch (5.2K, ../[email protected]/2-poc_mark_all_txns_catalog_change.patch)
download | inline diff:
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);
view thread (2+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox