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