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

PG Bug reporting form <noreply(at)postgresql(dot)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