Re: Fix race condition in SSI when reading PredXact->SxactGlobalXmin

Hi Josh, Andrey,

Andrey Borodin <x4mmm(at)yandex-team(dot)ru> 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