Re: [BUG?] check_exclusion_or_unique_constraint false negative

Hello Mikhail, Amit, everyone,

The commitfest entry is tagged "Help - Stuck Rebasing", so I started
there. I ended up with a rebase, a negative control for the tests,
and one behaviour change that I did not expect and that I think is
worth deciding on before this moves forward.

Everything below was measured on master 09a579abaca, with clean builds
in fresh directories (--enable-cassert --enable-injection-points).
Scripts and logs are attached.

1. Why it stopped applying
--------------------------

Not your fault, and not a trivial conflict:

* v18-0001 attaches its injection point inside index_getnext_slot(),
and that function is gone. ddce1da5b1b ("Add slot-based table AM
index scan interface", Sep 15) removed index_getnext_tid(),
index_fetch_heap() and index_getnext_slot() from indexam.c.
* v18-0002 fails for a much smaller reason: index_beginscan() grew a
bool argument in dcd8cc1c852.

Attached v19-0001/0002 is your patch rebased, with your authorship
untouched. The injection point now lives in
heapam_index_getnext_slot() (heapam_indexscan.c), at the same point of
the scan as before: right after the index AM returned a TID and before
the heap fetch. The four TAP tests needed no changes at all, which is
a good sign for where the point was placed.

Two things I dropped while rebasing, both unrelated to the fix:

* v18-0002 contained a stray hunk in catalog/index.c that only turns
one space into two inside index_concurrently_swap().
* v18-0001 adds INJECTION_POINT("check_exclusion_or_unique_constraint
_no_conflict") in execIndexing.c, three lines below the equivalent
point master already has from bc32a12e0db ("check-exclusion-or-
unique-constraint-no-conflict", with dashes), and no test in the
patch uses the new name.

2. The tests do prove something (negative control)
--------------------------------------------------

I ran the three deterministic TAP tests against a build that has
0001 (tests + injection points) but NOT 0002 (the fix):

without the fix: 3 files, 11 assertions, 9 failed
with the fix: 3 files, 11 assertions, all passed

So the tests fail for the reason they claim to, and the rebase did not
quietly defeat them.

Full suite with the fix, same commit: make check, isolation, recovery
and the whole src/test/subscription set all pass.

3. What the MVCC scan gives up: the wait
-----------------------------------------

This is the part I would like your opinion on, and Amit's.

SnapshotDirty sees rows written by transactions that are still in
flight, and that is why the current code takes snap.xmin/snap.xmax and
calls XactLockTableWait(): it waits for the inserter and then decides.
A fresh MVCC snapshot cannot see that row, so there is nobody to wait
for, and the caller is told the row does not exist.

That is observable without any injection point. The subscriber starts
empty (copy_data = false), a local session has an INSERT of the same
key open, and only then does the UPDATE arrive from the publisher:

sub: BEGIN; INSERT INTO t VALUES (1, 'fromsub'); -- left open
pub: UPDATE t SET data = 'frompubnew' WHERE a = 1;
sub: COMMIT;

Measured 5 runs per tree, identical every time:

master, apply worker waiting on a lock: yes
master, conflict reported: none
master, final row: frompubnew

v19, apply worker waiting on a lock: no
v19, conflict reported: update_missing
v19, final row: fromsub

So in this case master applies the publisher's UPDATE and the patch
loses it. To be fair to master, it is not blindly applying: if the
local session does ROLLBACK instead of COMMIT, master waits, finds
nothing, and reports the row as missing, exactly like the patch does.
It waits and then decides correctly in both outcomes.

I am not claiming this is worse overall than the bug you are fixing -
the race you found is real and I reproduced it. But it is the same
symptom, a lost update on the subscriber, moved to a different case,
and it would be a shame to trade one for the other silently.

This looks like a concrete answer to the question you and Amit left
open in August about whether SnapshotDirty was giving any real
guarantee here. For an in-flight INSERT it gives one, and it is a
guarantee the conflict-detection code depends on.

4. A variant that keeps the wait
---------------------------------

Attached as a .txt (nocfbot-wait-for-inflight-inserter.diff), on top
of v19-0002. The idea is small: scan with the fresh MVCC snapshot as
your patch does, and only if that finds nothing, do one extra pass
with SnapshotDirty purely to find out whether someone is inserting the
row right now. If so, wait for that transaction and start over with a
new MVCC snapshot.

The tuple found by the dirty pass is never returned to the caller, so
it cannot bring back the concurrent-update race that 0002 fixes: the
worst case is that the dirty scan misses the inserter and we fall back
to exactly the behaviour of v19.

Measured on the same commit:

in-flight INSERT test, 5 runs: same result as master every time
(waits, applies, final row frompubnew)
ROLLBACK variant: same as master
039/040/041 (your tests): all pass
make check, isolation: pass
src/test/subscription: 43 files, 619 tests, pass
recovery: pass

I did not benchmark the extra pass. It only runs when the MVCC scan
found nothing, which for a healthy subscriber is the path that is
already about to log a conflict, so I would expect it not to matter,
but I have not measured it and I would rather say so than guess.

Take it or leave it - it is your patch and you may well prefer to
handle the in-flight case some other way, or to argue that losing the
wait is acceptable. I mainly wanted the trade-off to be visible.

5. The test I used
------------------

Attached as 090_insert_inflight.pl.txt. It is not proposed for
commit as it is: it prints its measurement by failing a comparison,
which is handy for a review and wrong for the tree. If you find the
case worth covering I am happy to turn it into a proper test.

Regards,
Manu