pg.ddx.io pgsql-hackers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
heap_abort_speculative() sets xmin to Invalid* without HEAP_XMIN_INVALID 3+ messages / 3 participants [nested] [flat]
* heap_abort_speculative() sets xmin to Invalid* without HEAP_XMIN_INVALID @ 2020-07-23 19:40 Andres Freund <andres@anarazel.de> 2020-07-23 21:49 ` Re: heap_abort_speculative() sets xmin to Invalid* without HEAP_XMIN_INVALID Alvaro Herrera <alvherre@2ndquadrant.com> 0 siblings, 1 reply; 3+ messages in thread From: Andres Freund @ 2020-07-23 19:40 UTC (permalink / raw) To: pgsql-hackers@postgresql.org, Peter Geoghegan <pg@bowt.ie> 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 ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: heap_abort_speculative() sets xmin to Invalid* without HEAP_XMIN_INVALID 2020-07-23 19:40 heap_abort_speculative() sets xmin to Invalid* without HEAP_XMIN_INVALID Andres Freund <andres@anarazel.de> @ 2020-07-23 21:49 ` Alvaro Herrera <alvherre@2ndquadrant.com> 2020-07-24 03:51 ` Re: heap_abort_speculative() sets xmin to Invalid* without HEAP_XMIN_INVALID Peter Geoghegan <pg@bowt.ie> 0 siblings, 1 reply; 3+ messages in thread From: Alvaro Herrera @ 2020-07-23 21:49 UTC (permalink / raw) To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-hackers@postgresql.org, Peter Geoghegan <pg@bowt.ie> 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 ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: heap_abort_speculative() sets xmin to Invalid* without HEAP_XMIN_INVALID 2020-07-23 19:40 heap_abort_speculative() sets xmin to Invalid* without HEAP_XMIN_INVALID Andres Freund <andres@anarazel.de> 2020-07-23 21:49 ` Re: heap_abort_speculative() sets xmin to Invalid* without HEAP_XMIN_INVALID Alvaro Herrera <alvherre@2ndquadrant.com> @ 2020-07-24 03:51 ` Peter Geoghegan <pg@bowt.ie> 0 siblings, 0 replies; 3+ messages in thread From: Peter Geoghegan @ 2020-07-24 03:51 UTC (permalink / raw) To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Andres Freund <andres@anarazel.de>; pgsql-hackers 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 ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2020-07-24 03:51 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-07-23 19:40 heap_abort_speculative() sets xmin to Invalid* without HEAP_XMIN_INVALID Andres Freund <andres@anarazel.de> 2020-07-23 21:49 ` Alvaro Herrera <alvherre@2ndquadrant.com> 2020-07-24 03:51 ` Peter Geoghegan <pg@bowt.ie>
This inbox is served by DDX for PostgreSQL; see mirroring instructions for how to clone and mirror all data and code used for this inbox