Re: INSERT ... ON CONFLICT {UPDATE | IGNORE} 2.0

On Mon, Feb 16, 2015 at 4:11 PM, Peter Geoghegan <pg@heroku.com> wrote:
>>> Jim Nasby said something about setting the HEAP_XMIN_INVALID hint bit.
>>> Maybe he is right...if that can be made to be reliable (always
>>> WAL-logged), it could be marginally better than setting xmin to
>>> invalidTransactionId.
>>
>>
>> I'm not a big fan of that. The xmin-invalid bit is currently always just a
>> hint.
>
> Well, Andres makes the point that that isn't quite so.

Hmm. So the tqual.c routines actually check "if
(HeapTupleHeaderXminInvalid(tuple))". Which is:

#define HeapTupleHeaderXminInvalid(tup) \
( \
    ((tup)->t_infomask & (HEAP_XMIN_COMMITTED|HEAP_XMIN_INVALID)) == \
        HEAP_XMIN_INVALID \
)

What appears to happen if I try to only use the HEAP_XMIN_INVALID bit
(and not explicitly set xmin to InvalidTransactionId, and not
explicitly check that xmin isn't InvalidTransactionId in each
HeapTupleSatisfies* routine) is that after a little while, Jeff Janes'
tool shows up spurious unique violations, as if some tuple has become
visible when it shouldn't have. I guess this is because the
HEAP_XMIN_COMMITTED hint bit can still be set, which in effect
invalidates the HEAP_XMIN_INVALID hint bit.

It takes about 2 minutes before this happens for the first time when
fsync = off, following a fresh initdb, which is unacceptable. I'm not
sure if it's worth trying to figure out how HEAP_XMIN_COMMITTED gets
set. Not that I'm 100% sure that that's what this really is; it just
seems very likely.

Attached broken patch (broken_visible.patch) shows the changes made to
reveal this problem. Only the changes to tqual.c and heap_delete()
should matter here, since I did not test recovery.

I also thought about unifying the check for "if
(TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
InvalidTransactionId))" with the conventional
HeapTupleHeaderXminInvalid() macro, and leaving everything else as-is.
This is no good either, and for similar reasons - control often won't
reach the macro, which is behind a check of "if
(!HeapTupleHeaderXminCommitted(tuple))".

The best I think we can hope for is having a dedicated "if
(TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
InvalidTransactionId))" macro HeapTupleHeaderSuperDeleted() to do the
work each time, which does not need to be checked so often. A second
attached patch (compact_tqual_works.patch - which is non-broken,
AFAICT) shows how this is possible, while also moving the check
further down within each tqual.c routine (which seems more in keeping
with the fact that super deletion is a relatively obscure concept). I
haven't been able to break this variant using my existing test suite,
so this seems promising as a way of reducing tqual.c clutter. However,
as I said the other day, this is basically just polish.

--
Peter Geoghegan