Thread: heap_abort_speculative() sets xmin to Invalid* without HEAP_XMIN_INVALID

Hi,

After adding a few assertions to validate the connection scalability
patch I saw failures that also apply to master:

I added an assertion to TransactionIdIsCurrentTransactionId(),
*IsInProgress(), ... ensuring that the xid is within an expected
range. Which promptly failed in isolation tests.

The reason for that is that heap_abort_speculative() sets xmin to
InvalidTransactionId but does *not* add HEAP_XMIN_INVALID to infomask.

The various HeapTupleSatisfies* routines avoid calling those routines
for invalid xmins by checking HeapTupleHeaderXminInvalid() first. But
since heap_abort_speculative() didn't set that, we end up calling
TransactionIdIsCurrentTransactionId(InvalidTransactionId) which then
triggered my assertion.


Obviously I can trivially fix that by adjusting the assertions to allow
InvalidTransactionId. But to me it seems fragile to only have xmin == 0
in one rare occasion, and to rely on TransactionIdIs* to return
precisely the right thing without those functions necessarily having
been designed with that in mind.


I think we should change heap_abort_speculative() to set
HEAP_XMIN_INVALID in master. But we can't really do anything about
existing tuples without it - therefore we will have to forever take care
about encountering that combination :(.


Perhaps we should instead or additionally make
HeapTupleHeaderXminInvalid() explicitly check for InvalidTransactionId?

Greetings,

Andres Freund

On 2020-Jul-23, Andres Freund wrote:

> I think we should change heap_abort_speculative() to set
> HEAP_XMIN_INVALID in master.

+1

> But we can't really do anything about
> existing tuples without it - therefore we will have to forever take care
> about encountering that combination :(.
> 
> Perhaps we should instead or additionally make
> HeapTupleHeaderXminInvalid() explicitly check for InvalidTransactionId?

+1 for doing it as an additional fix, with a fat comment somewhere
explaining where such tuples would come from.

Additionally, but perhaps not very usefully, maybe we could have a
mechanism to inject such tuples so that code can be hardened against the
condition.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Jul 23, 2020 at 2:49 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> On 2020-Jul-23, Andres Freund wrote:
>
> > I think we should change heap_abort_speculative() to set
> > HEAP_XMIN_INVALID in master.
>
> +1

+1

> +1 for doing it as an additional fix, with a fat comment somewhere
> explaining where such tuples would come from.

There could be an opportunity to put this on a formal footing by doing
something in the amcheck heap checker patch.

-- 
Peter Geoghegan