Skip to content

Commit d2169c9

Browse files
author
Amit Kapila
committed
Fix the incorrect assertion introduced in commit 7f13ac8.
It has been incorrectly assumed in commit 7f13ac8 that we can either purge all or none in the catalog modifying xids list retrieved from a serialized snapshot. It is quite possible that some of the xids in that array are old enough to be pruned but not others. As per buildfarm Author: Amit Kapila and Masahiko Sawada Reviwed-by: Masahiko Sawada Discussion: https://postgr.es/m/CAA4eK1LBtv6ayE+TvCcPmC-xse=DVg=SmbyQD1nv_AaqcpUJEg@mail.gmail.com
1 parent 8c7fc86 commit d2169c9

File tree

3 files changed

+106
-20
lines changed

3 files changed

+106
-20
lines changed

‎contrib/test_decoding/expected/catalog_change_snapshot.out‎

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
Parsed test spec with 2 sessions
1+
Parsed test spec with 3 sessions
22

33
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
44
step s0_init: SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'test_decoding');
@@ -42,3 +42,57 @@ COMMIT
4242
stop
4343
(1 row)
4444

45+
46+
starting permutation: s0_init s0_begin s0_truncate s2_begin s2_truncate s1_checkpoint s1_get_changes s0_commit s0_begin s0_insert s1_checkpoint s1_get_changes s2_commit s1_checkpoint s1_get_changes s0_commit s1_get_changes
47+
step s0_init: SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'test_decoding');
48+
?column?
49+
--------
50+
init
51+
(1 row)
52+
53+
step s0_begin: BEGIN;
54+
step s0_truncate: TRUNCATE tbl1;
55+
step s2_begin: BEGIN;
56+
step s2_truncate: TRUNCATE tbl2;
57+
step s1_checkpoint: CHECKPOINT;
58+
step s1_get_changes: SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts', '1', 'include-xids', '0');
59+
data
60+
----
61+
(0 rows)
62+
63+
step s0_commit: COMMIT;
64+
step s0_begin: BEGIN;
65+
step s0_insert: INSERT INTO tbl1 VALUES (1);
66+
step s1_checkpoint: CHECKPOINT;
67+
step s1_get_changes: SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts', '1', 'include-xids', '0');
68+
data
69+
---------------------------------------
70+
BEGIN
71+
table public.tbl1: TRUNCATE: (no-flags)
72+
COMMIT
73+
(3 rows)
74+
75+
step s2_commit: COMMIT;
76+
step s1_checkpoint: CHECKPOINT;
77+
step s1_get_changes: SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts', '1', 'include-xids', '0');
78+
data
79+
---------------------------------------
80+
BEGIN
81+
table public.tbl2: TRUNCATE: (no-flags)
82+
COMMIT
83+
(3 rows)
84+
85+
step s0_commit: COMMIT;
86+
step s1_get_changes: SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts', '1', 'include-xids', '0');
87+
data
88+
-------------------------------------------------------------
89+
BEGIN
90+
table public.tbl1: INSERT: val1[integer]:1 val2[integer]:null
91+
COMMIT
92+
(3 rows)
93+
94+
?column?
95+
--------
96+
stop
97+
(1 row)
98+

‎contrib/test_decoding/specs/catalog_change_snapshot.spec‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,15 @@
33
setup
44
{
55
DROP TABLE IF EXISTS tbl1;
6+
DROP TABLE IF EXISTS tbl2;
67
CREATE TABLE tbl1 (val1 integer, val2 integer);
8+
CREATE TABLE tbl2 (val1 integer, val2 integer);
79
}
810

911
teardown
1012
{
1113
DROP TABLE tbl1;
14+
DROP TABLE tbl2;
1215
SELECT 'stop' FROM pg_drop_replication_slot('isolation_slot');
1316
}
1417

@@ -26,6 +29,12 @@ setup { SET synchronous_commit=on; }
2629
step "s1_checkpoint" { CHECKPOINT; }
2730
step "s1_get_changes" { SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts', '1', 'include-xids', '0'); }
2831

32+
session "s2"
33+
setup { SET synchronous_commit=on; }
34+
step "s2_begin" { BEGIN; }
35+
step "s2_truncate" { TRUNCATE tbl2; }
36+
step "s2_commit" { COMMIT; }
37+
2938
# For the transaction that TRUNCATEd the table tbl1, the last decoding decodes
3039
# only its COMMIT record, because it starts from the RUNNING_XACTS record emitted
3140
# during the first checkpoint execution. This transaction must be marked as
@@ -37,3 +46,14 @@ step "s1_get_changes" { SELECT data FROM pg_logical_slot_get_changes('isolation_
3746
# record written by bgwriter. One might think we can either stop the bgwriter or
3847
# increase LOG_SNAPSHOT_INTERVAL_MS but it's not practical via tests.
3948
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"
49+
50+
# Test that we can purge the old catalog modifying transactions after restoring
51+
# them from the serialized snapshot. The first checkpoint will serialize the list
52+
# of two catalog modifying xacts. The purpose of the second checkpoint is to allow
53+
# partial pruning of the list of catalog modifying xact. The third checkpoint
54+
# followed by get_changes establishes a restart_point at the first checkpoint LSN.
55+
# The last get_changes will start decoding from the first checkpoint which
56+
# restores the list of catalog modifying xacts and then while decoding the second
57+
# checkpoint record it prunes one of the xacts in that list and when decoding the
58+
# next checkpoint, it will completely prune that list.
59+
permutation "s0_init" "s0_begin" "s0_truncate" "s2_begin" "s2_truncate" "s1_checkpoint" "s1_get_changes" "s0_commit" "s0_begin" "s0_insert" "s1_checkpoint" "s1_get_changes" "s2_commit" "s1_checkpoint" "s1_get_changes" "s0_commit" "s1_get_changes"

‎src/backend/replication/logical/snapbuild.c‎

Lines changed: 31 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -969,28 +969,40 @@ SnapBuildPurgeOlderTxn(SnapBuild *builder)
969969
pfree(workspace);
970970

971971
/*
972-
* Either all the xacts got purged or none. It is only possible to
973-
* partially remove the xids from this array if one or more of the xids
974-
* are still running but not all. That can happen if we start decoding
975-
* from a point (LSN where the snapshot state became consistent) where all
976-
* the xacts in this were running and then at least one of those got
977-
* committed and a few are still running. We will never start from such a
978-
* point because we won't move the slot's restart_lsn past the point where
979-
* the oldest running transaction's restart_decoding_lsn is.
972+
* Purge xids in ->catchange as well. The purged array must also be sorted
973+
* in xidComparator order.
980974
*/
981-
if (builder->catchange.xcnt == 0 ||
982-
TransactionIdFollowsOrEquals(builder->catchange.xip[0],
983-
builder->xmin))
984-
return;
975+
if (builder->catchange.xcnt > 0)
976+
{
977+
/*
978+
* Since catchange.xip is sorted, we find the lower bound of xids that
979+
* are still interesting.
980+
*/
981+
for (off = 0; off < builder->catchange.xcnt; off++)
982+
{
983+
if (TransactionIdFollowsOrEquals(builder->catchange.xip[off],
984+
builder->xmin))
985+
break;
986+
}
985987

986-
Assert(TransactionIdFollows(builder->xmin,
987-
builder->catchange.xip[builder->catchange.xcnt - 1]));
988-
pfree(builder->catchange.xip);
989-
builder->catchange.xip = NULL;
990-
builder->catchange.xcnt = 0;
988+
surviving_xids = builder->catchange.xcnt - off;
991989

992-
elog(DEBUG3, "purged catalog modifying transactions, oldest running xid %u",
993-
builder->xmin);
990+
if (surviving_xids > 0)
991+
{
992+
memmove(builder->catchange.xip, &(builder->catchange.xip[off]),
993+
surviving_xids * sizeof(TransactionId));
994+
}
995+
else
996+
{
997+
pfree(builder->catchange.xip);
998+
builder->catchange.xip = NULL;
999+
}
1000+
1001+
elog(DEBUG3, "purged catalog modifying transactions from %u to %u, xmin: %u, xmax: %u",
1002+
(uint32) builder->catchange.xcnt, (uint32) surviving_xids,
1003+
builder->xmin, builder->xmax);
1004+
builder->catchange.xcnt = surviving_xids;
1005+
}
9941006
}
9951007

9961008
/*

0 commit comments

Comments
 (0)