From: feichanghong <feichanghong@qq.com>
To: feichanghong <feichanghong@qq.com>
To: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Subject: Re:BUG #18280: logical decoding build wrong snapshot for subtransactions
Date: Wed, 10 Jan 2024 21:56:00 +0800
Message-ID: <tencent_0726471CA7087C4AC6EF1F19409DF11FCF08@qq.com> (raw)
In-Reply-To: <18280-4c8060178cb41750@postgresql.org>
References: <18280-4c8060178cb41750@postgresql.org>
> SnapBuildXidSetCatalogChanges (introduced in 272248a) skipping the check for
> the sub-transactions when the parent transaction has been marked as
> containing catalog changes should be the root cause of the problem.
My fix is to remove the ReorderBufferXidHasCatalogChanges condition from the SnapBuildXidSetCatalogChanges function. This approach has a false positive; the subtransactions that didn't change catalog to the snapshot are also added to the commit list. But that won't be a problem since we use snapshot built during decoding only to read system catalogs, as 272248a's commit message says.
The attached patch has been verified to resolve the mentioned issue.
Best regards.
Alibaba Cloud Computing Ltd.
Attachments:
[application/octet-stream] fix_wrong_snapshot_for_logical_decoding.patch (5.6K, ../tencent_0726471CA7087C4AC6EF1F19409DF11FCF08@qq.com/3-fix_wrong_snapshot_for_logical_decoding.patch)
download | inline diff:diff --git a/contrib/test_decoding/expected/catalog_change_snapshot.out b/contrib/test_decoding/expected/catalog_change_snapshot.outindex b33e49c0b1..a2eadc5418 100644--- a/contrib/test_decoding/expected/catalog_change_snapshot.out+++ b/contrib/test_decoding/expected/catalog_change_snapshot.out@@ -132,3 +132,41 @@ COMMIT
stop
(1 row)
++starting permutation: s0_init s0_begin s0_savepoint s0_create_part1 s0_savepoint_release s1_checkpoint s0_create_part2 s0_commit s0_begin s0_truncate s1_checkpoint s1_get_changes s0_insert_part s1_get_changes s0_commit+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_create_part1: CREATE TABLE tbl1_part_p1 PARTITION OF tbl1_part FOR VALUES FROM (0) TO (10);+step s0_savepoint_release: RELEASE SAVEPOINT sp1;+step s1_checkpoint: CHECKPOINT;+step s0_create_part2: CREATE TABLE tbl1_part_p2 PARTITION OF tbl1_part FOR VALUES FROM (10) TO (20);+step s0_commit: COMMIT;+step s0_begin: BEGIN;+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, 'skip-empty-xacts', '1', 'include-xids', '0');+data+----+(0 rows)++step s0_insert_part: INSERT INTO tbl1_part VALUES (1);+step s1_get_changes: SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts', '1', 'include-xids', '0');+data +--------------------------------------------------+BEGIN +table public.tbl1_part_p1: INSERT: val1[integer]:1+COMMIT +(3 rows)++step s0_commit: COMMIT;+?column?+--------+stop +(1 row)+diff --git a/contrib/test_decoding/specs/catalog_change_snapshot.spec b/contrib/test_decoding/specs/catalog_change_snapshot.specindex 770dbd642d..0a17a0dd17 100644--- a/contrib/test_decoding/specs/catalog_change_snapshot.spec+++ b/contrib/test_decoding/specs/catalog_change_snapshot.spec@@ -3,13 +3,16 @@
setup
{
DROP TABLE IF EXISTS tbl1;
+ DROP TABLE IF EXISTS tbl1_part;
CREATE TABLE tbl1 (val1 integer, val2 integer);
+ CREATE TABLE tbl1_part (val1 integer) PARTITION BY RANGE (val1);
CREATE TABLE user_cat (val1 integer) WITH (user_catalog_table = true);
}
teardown
{
DROP TABLE tbl1;
+ DROP TABLE tbl1_part;
DROP TABLE user_cat;
SELECT 'stop' FROM pg_drop_replication_slot('isolation_slot');
}
@@ -19,15 +22,19 @@ 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_savepoint_release" { RELEASE SAVEPOINT sp1; }
step "s0_truncate" { TRUNCATE tbl1; }
step "s0_insert" { INSERT INTO tbl1 VALUES (1); }
step "s0_insert2" { INSERT INTO user_cat VALUES (1); }
+step "s0_create_part1" { CREATE TABLE tbl1_part_p1 PARTITION OF tbl1_part FOR VALUES FROM (0) TO (10); }+step "s0_create_part2" { CREATE TABLE tbl1_part_p2 PARTITION OF tbl1_part FOR VALUES FROM (10) TO (20); }
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, 'skip-empty-xacts', '1', 'include-xids', '0'); }
+step "s0_insert_part" { INSERT INTO tbl1_part VALUES (1); }
# For the transaction that TRUNCATEd the table tbl1, the last decoding decodes
# only its COMMIT record, because it starts from the RUNNING_XACTS record emitted
@@ -60,3 +67,9 @@ permutation "s0_init" "s0_begin" "s0_savepoint" "s0_insert" "s1_checkpoint" "s1_
# to skip this xact but ensure that corresponding invalidation messages
# get processed.
permutation "s0_init" "s0_begin" "s0_savepoint" "s0_insert" "s1_checkpoint" "s1_get_changes" "s0_truncate" "s0_commit" "s0_begin" "s0_insert" "s1_checkpoint" "s1_get_changes" "s0_commit" "s1_get_changes"
++# For the subtransaction that created the table tbl1_part_p1, the last decoding does+# not decode any WAL records for it, because it ends before the RUNNING_XACTS record.+# When decoding the COMMIT record of its parent transaction, the subtransaction also+# needs to be added to the committed list because it modified the catalog.+permutation "s0_init" "s0_begin" "s0_savepoint" "s0_create_part1" "s0_savepoint_release" "s1_checkpoint" "s0_create_part2" "s0_commit" "s0_begin" "s0_truncate" "s1_checkpoint" "s1_get_changes" "s0_insert_part" "s1_get_changes" "s0_commit"
\ No newline at end of file
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.cindex 3eaaaac6b3..91cc70e14b 100644--- a/src/backend/replication/logical/snapbuild.c+++ b/src/backend/replication/logical/snapbuild.c@@ -2126,11 +2126,12 @@ SnapBuildXidSetCatalogChanges(SnapBuild *builder, TransactionId xid, int subxcnt
TransactionId *subxacts, XLogRecPtr lsn)
{
/*
- * Skip if there is no initial running xacts information or the- * transaction is already marked as containing catalog changes.+ * Skip if there is no initial running xacts information.+ * Even if the transaction has been marked as containing catalog changes,+ * it cannot be skipped because its subtransactions that modified the+ * catalog may not be marked.
*/
- if (NInitialRunningXacts == 0 ||- ReorderBufferXidHasCatalogChanges(builder->reorder, xid))+ if (NInitialRunningXacts == 0)
return;
if (bsearch(&xid, InitialRunningXacts, NInitialRunningXacts,
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: pgsql-bugs@postgresql.org
Cc: feichanghong@qq.com, pgsql-bugs@lists.postgresql.org
Subject: Re:BUG #18280: logical decoding build wrong snapshot for subtransactions
In-Reply-To: <tencent_0726471CA7087C4AC6EF1F19409DF11FCF08@qq.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox