Re: BUG #18499: Reindexing spgist index concurrently triggers Assert("TransactionIdIsValid(state->myXid)") - Mailing list pgsql-bugs

From Tom Lane
Subject Re: BUG #18499: Reindexing spgist index concurrently triggers Assert("TransactionIdIsValid(state->myXid)")
Date
Msg-id 428421.1717887173@sss.pgh.pa.us
Whole thread Raw
In response to BUG #18499: Reindexing spgist index concurrently triggers Assert("TransactionIdIsValid(state->myXid)")  (PG Bug reporting form <noreply@postgresql.org>)
Responses Re: BUG #18499: Reindexing spgist index concurrently triggers Assert("TransactionIdIsValid(state->myXid)")
List pgsql-bugs
PG Bug reporting form <noreply@postgresql.org> writes:
> The following script:
> ...
> triggers an assertion failure with the following stack trace:
> TRAP: failed Assert("TransactionIdIsValid(state->myXid)"), File:
> "spgutils.c", Line: 1078, PID: 2975757

Thanks for the report!  The problem evidently starts in
initSpGistState, which does

    state->myXid = GetTopTransactionIdIfAny();

Ordinarily this would produce a valid myXid, since if we're creating
an index or inserting a tuple into an existing index, there would
already be an XID.  But REINDEX CONCURRENTLY reaches this code in a
new transaction that hasn't assigned an XID and probably never will.
So if we have to make a REDIRECT tuple, we have trouble.

> Without asserts enabled, REINDEX executed with no error.

It's still broken though, because creating a REDIRECT tuple with
an invalid XID is not OK; it'll cause trouble later on.

I experimented with

-   state->myXid = GetTopTransactionIdIfAny();
+   state->myXid = GetTopTransactionId();

but that causes the parallel-vacuum tests to fall over with

ERROR:  cannot assign XIDs during a parallel operation

Apparently, we never make REDIRECT tuples during VACUUM, or we'd
have seen this error reported before.  We could maybe make this
code read something like "if not VACUUM then assign an XID", but
that might still fail in parallelized concurrent REINDEX.

In any case, it feels like a bad idea that initSpGistState doesn't
always make myXid valid: that seems like it's assuming too much
about when we can create REDIRECT tuples.

What seems like it should work is

     state->myXid = GetTopTransactionIdIfAny();
+    if (!TransactionIdIsValid(state->myXid))
+        state->myXid = ReadNextTransactionId();

on the grounds that if we lack an XID, then the next-to-be-assigned
XID should be a safe expiry horizon for any REDIRECT we might need.

There are some comments to be updated, and in HEAD I'd be inclined to
rename "myXid" to something that doesn't imply that it's necessarily
our own XID.  Maybe "redirectXid", since it's not used for anything
but that.

            regards, tom lane



pgsql-bugs by date:

Previous
From: Tom Lane
Date:
Subject: Re: BUG #18483: Segmentation fault in tests modules
Next
From: PG Bug reporting form
Date:
Subject: BUG #18500: Detaching a partition with an index manually attached to the parent's index triggers Assert