Hi Josh, Andrey,
Andrey Borodin <[email protected]> wrote:
> I suspect a race in the test:
>
> +step s2_insert_wait_at_page_split: <... completed>
> +step s1_commit_wait_in_SetNewSxactGlobalXmin: <... completed>
> Could it instead calculate the new xmin and count in local variables,
> then publish them after scanning the active list?
I tried both, on master e8a3ee5b197 with --enable-cassert and
--enable-injection-points. v4 needs a rebase, but only in the
injection_points Makefile and meson.build lists.
The test. Run 50 times against one cluster (the attached
run_spec_n.sh), v4's spec gives the other completion order in 50 of 50
runs with v4 applied. And it doesn't catch the bug: with v4's test and
injection points but the readers as on master, it showed id 2 twice in 1
run of 50, the first one on a fresh cluster. Under make check in
injection_points it fails there only on the completion order; verify
has no duplicate. wakeup_s2_then_s1 wakes both sessions at once, and
the window only exists while s1 is still inside
SetNewSxactGlobalXmin(), so most of the time s1 is done before s2 reads
SxactGlobalXmin.
A second injection point right after the check in
PredicateLockPageSplit() makes it deterministic: wake s2 alone, so it
checks while s1 is still inside SetNewSxactGlobalXmin() and stops right
after the check, then wake s1, then s2 again, with markers for both.
If s2 saw InvalidTransactionId it returned early and never gets to the
second point.
duplicate id distinct outputs
readers as on master 13 of 13 1
Andrey's approach 0 of 30 1
v4 (reader locks) 0 of 15 2
With v4, s2 checks under SerializableXactHashLock, which s1 holds in
exclusive mode while it waits inside SetNewSxactGlobalXmin(), so after
the first wakeup s2 waits on an LWLock, which isolationtester doesn't
see, and the output depends on whether it looks at s2 before or after
that. In an earlier round one run of eleven stopped right after
wakeup_s2 until the 60 s timeout; in the 15 above none did. (Without
the fix, one run in fourteen hung the same way, which is why only the
13 before it are counted.) So the reader locks also keep the test from
being deterministic.
Andrey's approach. The attached diff, on top of v4, does it:
SetNewSxactGlobalXmin() computes the xmin and the count in local
variables and publishes them after the scan, SxactGlobalXmin last; the
three readers go back to the lockless check, with the comments that
explained why it is safe put back plus a sentence on why it is safe
again; and the spec is the deterministic one above. With it,
injection_points (make check, three times in a row), src/test/isolation
(133) and make check (239) pass, and pgindent leaves it unchanged.
I also tried to measure the reader locks, with the attached
page_split_bench.sh: 50-row inserts of random keys into a btree, so
page splits are frequent, 8 clients, 15 s, 7 runs round-robin between
release builds, median [min - max] TPS:
no serializable xact inside SERIALIZABLE
master 27859 [18240-30818] 27643 [23660-29449]
v4 27878 [22204-31266] 27497 [24554-28122]
Andrey's approach 28570 [18464-32231] 27033 [25626-28803]
So I could not measure the cost of the locks here: the differences are
smaller than the spread between runs on this machine. That's not an
argument for the locks, just one I can't make against them.
Josh, feel free to take any of it into a v5; I'm happy to review it.
Regards,
Manu
diff --git a/src/backend/storage/lmgr/predicate.c
b/src/backend/storage/lmgr/predicate.c
index b2509ac0765..82a9fcdd9f5 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -2879,14 +2879,15 @@ DropAllPredicateLocksFromTable(Relation relation, bool
transfer)
/*
* Bail out quickly if there are no serializable transactions running.
+ * It's safe to check this without taking locks because the caller is
+ * holding an ACCESS EXCLUSIVE lock on the relation. No new locks which
+ * would matter here can be acquired while that is held.
+ * SetNewSxactGlobalXmin() publishes a new value only after scanning the
+ * active list, so InvalidTransactionId is never seen here while
+ * serializable transactions are still active.
*/
- LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
- {
- LWLockRelease(SerializableXactHashLock);
return;
- }
- LWLockRelease(SerializableXactHashLock);
if (!PredicateLockingNeededForRelation(relation))
return;
@@ -3082,15 +3083,21 @@ PredicateLockPageSplit(Relation relation, BlockNumber
oldblkno,
/*
* Bail out quickly if there are no serializable transactions running.
+ *
+ * It's safe to do this check without taking any additional locks. Even
if
+ * a serializable transaction starts concurrently, we know it can't take
+ * any SIREAD locks on the page being split because the caller is
holding
+ * the associated buffer page lock. Memory reordering isn't an issue;
the
+ * memory barrier in the LWLock acquisition guarantees that this read
+ * occurs while the buffer page lock is held. SetNewSxactGlobalXmin()
+ * publishes a new value only after scanning the active list, so
+ * InvalidTransactionId is never seen here while serializable
transactions
+ * are still active.
*/
INJECTION_POINT("predicate-lock-page-split", NULL);
- LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
- {
- LWLockRelease(SerializableXactHashLock);
return;
- }
- LWLockRelease(SerializableXactHashLock);
+ INJECTION_POINT("predicate-lock-page-split-after-check", NULL);
if (!PredicateLockingNeededForRelation(relation))
return;
@@ -3182,12 +3189,17 @@ static void
SetNewSxactGlobalXmin(void)
{
dlist_iter iter;
+ TransactionId xmin = InvalidTransactionId;
+ int xmincount = 0;
Assert(LWLockHeldByMe(SerializableXactHashLock));
- PredXact->SxactGlobalXmin = InvalidTransactionId;
- PredXact->SxactGlobalXminCount = 0;
-
+ /*
+ * Compute the new values in local variables, and publish them only at
the
+ * end: some callers read SxactGlobalXmin without holding
+ * SerializableXactHashLock, and must not see a transient
+ * InvalidTransactionId while serializable transactions are still
active.
+ */
#ifdef USE_INJECTION_POINTS
INJECTION_POINT_CACHED("predicate-set-sxact-global-xmin-invalid", NULL);
#endif
@@ -3202,20 +3214,21 @@ SetNewSxactGlobalXmin(void)
&& sxact != OldCommittedSxact)
{
Assert(sxact->xmin != InvalidTransactionId);
- if (!TransactionIdIsValid(PredXact->SxactGlobalXmin)
- || TransactionIdPrecedes(sxact->xmin,
-
PredXact->SxactGlobalXmin))
+ if (!TransactionIdIsValid(xmin)
+ || TransactionIdPrecedes(sxact->xmin, xmin))
{
- PredXact->SxactGlobalXmin = sxact->xmin;
- PredXact->SxactGlobalXminCount = 1;
+ xmin = sxact->xmin;
+ xmincount = 1;
}
- else if (TransactionIdEquals(sxact->xmin,
-
PredXact->SxactGlobalXmin))
- PredXact->SxactGlobalXminCount++;
+ else if (TransactionIdEquals(sxact->xmin, xmin))
+ xmincount++;
}
}
- SerialSetActiveSerXmin(PredXact->SxactGlobalXmin);
+ PredXact->SxactGlobalXminCount = xmincount;
+ PredXact->SxactGlobalXmin = xmin;
+
+ SerialSetActiveSerXmin(xmin);
}
/*
@@ -4364,14 +4377,15 @@ CheckTableForSerializableConflictIn(Relation relation)
/*
* Bail out quickly if there are no serializable transactions running.
+ * It's safe to check this without taking locks because the caller is
+ * holding an ACCESS EXCLUSIVE lock on the relation. No new locks which
+ * would matter here can be acquired while that is held.
+ * SetNewSxactGlobalXmin() publishes a new value only after scanning the
+ * active list, so InvalidTransactionId is never seen here while
+ * serializable transactions are still active.
*/
- LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
- {
- LWLockRelease(SerializableXactHashLock);
return;
- }
- LWLockRelease(SerializableXactHashLock);
if (!SerializationNeededForWrite(relation))
return;
diff --git
a/src/test/modules/injection_points/expected/predicate-lock-page-split.out
b/src/test/modules/injection_points/expected/predicate-lock-page-split.out
index 8f5613c9d8e..d80353abf5e 100644
--- a/src/test/modules/injection_points/expected/predicate-lock-page-split.out
+++ b/src/test/modules/injection_points/expected/predicate-lock-page-split.out
@@ -1,6 +1,6 @@
Parsed test spec with 5 sessions
-starting permutation: s1_begin bump_xmin s2_begin s3_begin s1_insert
s2_insert_wait_at_page_split s1_commit_wait_in_SetNewSxactGlobalXmin
wakeup_s2_then_s1 s3_insert s3_commit s2_commit verify
+starting permutation: s1_begin bump_xmin s2_begin s3_begin s1_insert
s2_insert_wait_at_page_split s1_commit_wait_in_SetNewSxactGlobalXmin wakeup_s2
wakeup_s1 wakeup_s2_after_check s3_insert s3_commit s2_commit verify
injection_points_attach
-----------------------
@@ -67,24 +67,35 @@ step s2_insert_wait_at_page_split:
step s1_commit_wait_in_SetNewSxactGlobalXmin:
COMMIT;
<waiting ...>
-step wakeup_s2_then_s1:
+step wakeup_s2:
SELECT injection_points_wakeup('predicate-lock-page-split');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step wakeup_s1:
SELECT injection_points_wakeup('predicate-set-sxact-global-xmin-invalid');
<waiting ...>
-step s2_insert_wait_at_page_split: <... completed>
step s1_commit_wait_in_SetNewSxactGlobalXmin: <... completed>
+step wakeup_s1: <... completed>
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step wakeup_s2_after_check:
+ SELECT injection_points_wakeup('predicate-lock-page-split-after-check');
+ <waiting ...>
step s3_insert:
INSERT INTO test_table
SELECT max(id) + 1
FROM test_table;
ERROR: could not serialize access due to read/write dependencies among
transactions
-step wakeup_s2_then_s1: <... completed>
-injection_points_wakeup
------------------------
-
-(1 row)
-
+step s2_insert_wait_at_page_split: <... completed>
+step wakeup_s2_after_check: <... completed>
injection_points_wakeup
-----------------------
diff --git
a/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
b/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
index fd905a37b07..8ed3f0f4223 100644
--- a/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
+++ b/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
@@ -1,4 +1,6 @@
-# Test for race condition in PredicateLockPageSplit
+# Test for race condition in PredicateLockPageSplit (deterministic variant:
+# s2 decides whether to transfer the SIREAD locks while s1 is still inside
+# SetNewSxactGlobalXmin(), and stops right after that decision)
#
# When SetNewSxactGlobalXmin() temporarily sets SxactGlobalXmin to
# InvalidTransactionId, a concurrent PredicateLockPageSplit() can see
@@ -58,6 +60,7 @@ session s2
setup {
SELECT injection_points_set_local();
SELECT injection_points_attach('predicate-lock-page-split', 'wait');
+ SELECT injection_points_attach('predicate-lock-page-split-after-check',
'wait');
}
step s2_begin {
BEGIN ISOLATION LEVEL SERIALIZABLE;
@@ -96,28 +99,21 @@ step s3_commit {
}
session s4
-step wakeup_s2_then_s1 {
+step wakeup_s2 {
SELECT injection_points_wakeup('predicate-lock-page-split');
+}
+step wakeup_s2_after_check {
+ SELECT injection_points_wakeup('predicate-lock-page-split-after-check');
+}
+step wakeup_s1 {
SELECT injection_points_wakeup('predicate-set-sxact-global-xmin-invalid');
}
-# s1_begin: s1 reads from the table, establishing SIREAD locks on the index
-# bump_xmin: advance xmin so s2/s3 get a higher xmin than s1
-# s2_begin, s3_begin: s2 and s3 read from the table (same snapshot as s1)
-#
-# s1_insert: s1 inserts max(id)+1 = 2
-# s2_insert_wait_at_page_split: s2 inserts descending values until a real
-# btree page split happens, then waits in PredicateLockPageSplit before
-# checking SxactGlobalXmin. The values must be descending so that the
-# split moves ids 1 and 2 to the new page
-# s1_commit_wait_in_SetNewSxactGlobalXmin: after s2 is already waiting,
-# s1 commits and waits after SetNewSxactGlobalXmin sets SxactGlobalXmin
-# to InvalidTransactionId
-# wakeup_s2_then_s1: wake s2 (sees InvalidTransactionId, skips SIREAD
-# lock transfer), then wake s1
-# s3_insert: s3 inserts max(id)+1 = 2, computed from its snapshot
-# s3_commit: s3 commits (should have been aborted by SSI)
-# s2_commit: s2 aborts due to serialization failure
+# s1 waits inside SetNewSxactGlobalXmin(). s2 is woken alone: it reads
+# SxactGlobalXmin while s1 is still there and, if it sees a valid value, stops
+# right after the check (a lock-free check never waits on s1). Then s1 is
+# woken, and then s2. If s2 saw InvalidTransactionId, it skipped the SIREAD
+# lock transfer and never reaches the second point: the id is inserted twice.
permutation
s1_begin
bump_xmin
@@ -126,7 +122,9 @@ permutation
s1_insert
s2_insert_wait_at_page_split
s1_commit_wait_in_SetNewSxactGlobalXmin
-
wakeup_s2_then_s1(s2_insert_wait_at_page_split,s1_commit_wait_in_SetNewSxactGlobalXmin)
+ wakeup_s2
+ wakeup_s1(s1_commit_wait_in_SetNewSxactGlobalXmin)
+ wakeup_s2_after_check(s2_insert_wait_at_page_split)
s3_insert
s3_commit
s2_commit
#!/bin/bash
# usage: run_spec_n.sh <install prefix> <isolationtester> <spec file> <runs>
<port>
#
# Runs one isolation spec <runs> times against a single throwaway cluster
# and counts the distinct outputs, and the runs where verify shows id 2
# twice. Each run gets 60 seconds; a run that hangs leaves backends waiting
# on injection points, so the runs after it are not meaningful.
P=$1; IT=$2; SPEC=$3; RUNS=$4; PORT=$5
D=/tmp/specn_$PORT
rm -rf $D; mkdir -p $D/out
$P/bin/initdb -D $D/data -U postgres --no-sync >/dev/null 2>&1
$P/bin/pg_ctl -D $D/data -o "-p $PORT -k /tmp" -l $D/log -w start >/dev/null
trap '$P/bin/pg_ctl -D $D/data -m immediate -w stop >/dev/null 2>&1; rm -rf $D'
EXIT
for r in $(seq 1 $RUNS); do
timeout 60 $IT "host=/tmp port=$PORT dbname=postgres user=postgres" < $SPEC >
$D/out/$r 2>&1 ||
echo "run $r: exit $? (124 = timed out)"
done
echo "distinct outputs:"
md5sum $D/out/* | awk '{print $1}' | sort | uniq -c
echo "runs with id 2 twice: $(grep -lE '^ *2\| *2$' $D/out/* | wc -l)"
#!/bin/bash
# usage: page_split_bench.sh <clients> <seconds> <runs> <prefix> [<prefix> ...]
#
# Inserts of random keys into a btree (many page splits), 50 rows per
# transaction, with pgbench:
# A: read committed, no serializable transaction around
# B: the same inside BEGIN ISOLATION LEVEL SERIALIZABLE (--max-tries 50)
# One cluster per prefix (release builds), run round-robin, median TPS.
# The clusters live in $WORKDIR (default ./psbench, on disk): millions of
# inserted rows and their WAL would fill a tmpfs /tmp.
C=$1; T=$2; R=$3; shift 3
W=${WORKDIR:-$PWD/psbench}; rm -rf $W; mkdir -p $W
cat > $W/A.sql <<'EOF'
INSERT INTO t (k) SELECT (random() * 2000000000)::int FROM generate_series(1,
50);
EOF
cat > $W/B.sql <<'EOF'
BEGIN ISOLATION LEVEL SERIALIZABLE;
INSERT INTO t (k) SELECT (random() * 2000000000)::int FROM generate_series(1,
50);
COMMIT;
EOF
i=0
for P in "$@"; do
i=$((i + 1)); port=$((56500 + i))
$P/bin/initdb -D $W/d$i -U postgres --no-sync >/dev/null 2>&1
cat >> $W/d$i/postgresql.conf <<EOF
shared_buffers = 1GB
fsync = off
synchronous_commit = off
full_page_writes = off
autovacuum = off
EOF
$P/bin/pg_ctl -D $W/d$i -o "-p $port -k /tmp" -l $W/log$i -w start >/dev/null
done
trap 'i=0; for P in "$@"; do i=$((i + 1)); $P/bin/pg_ctl -D $W/d$i -m immediate
-w stop >/dev/null 2>&1; done; rm -rf $W' EXIT
for w in A B; do
for r in $(seq 1 $R); do
i=0
for P in "$@"; do
i=$((i + 1)); port=$((56500 + i))
$P/bin/psql -X -q -h /tmp -p $port -U postgres postgres \
-c "DROP TABLE IF EXISTS t" -c "CREATE TABLE t (k int)" -c "CREATE
INDEX ON t (k)" -c "CHECKPOINT"
$P/bin/pgbench -n -h /tmp -p $port -U postgres -c $C -j $C -T $T
--max-tries=50 \
-f $W/$w.sql postgres > $W/pgbench.out 2>&1
tps=$(grep -oE 'tps = [0-9.]+' $W/pgbench.out | grep -oE '[0-9.]+')
[ -n "$tps" ] || { echo "pgbench failed ($w, $P):" >&2; tail -3
$W/pgbench.out >&2; }
echo "$w $i $tps" | tee -a $W/raw
done
done
done
echo "TPS ($C clients, $T s, $R runs): median [min - max]"
for w in A B; do
i=0
for P in "$@"; do
i=$((i + 1))
awk -v w=$w -v n=$i '$1==w && $2==n && $3 != "" {print $3}' $W/raw | sort
-n |
awk -v w=$w -v p=$P '{a[NR]=$1} END {printf " %s %-40s %8.0f [%.0f -
%.0f] (%d runs)\n", w, p, a[int((NR+1)/2)], a[1], a[NR], NR}'
done
done
