Re: 'ERROR: attempted to update invisible tuple' from 'ALTER INDEX ... ATTACH PARTITION' on parent index

On Wed, Jul 12, 2023 at 04:02:23PM -0400, Robert Haas wrote:
> Oh, interesting. The fact that indisreplident isn't copied seems like
> a pretty clear mistake, but I'm guessing that the fact that t_self
> wasn't refreshed was deliberate and that the author of this code
> didn't really intend for callers to look at the t_self value. We could
> change our mind about whether that ought to be allowed, though. But,
> like, none of the other tuple header fields are copied either... xmax,
> xvac, infomask, etc.

See 3c84046 and 8ec9438, mainly, from Tom.  I didn't know that this is
used as a shortcut to reload index information in the cache because it
is much cheaper than a full index information rebuild.  I agree that
not copying indisreplident in this code path is a mistake as this bug
shows, because any follow-up command run in a transaction that changed
this field would get an incorrect information reference.

Now, I have to admit that I am not completely sure what the
consequences of this addition are when it comes to concurrent index
operations (CREATE/DROP INDEX, REINDEX):
        /* Copy xmin too, as that is needed to make sense of indcheckxmin */
        HeapTupleHeaderSetXmin(relation->rd_indextuple->t_data,
                               HeapTupleHeaderGetXmin(tuple->t_data));
+       ItemPointerCopy(&tuple->t_self, &relation->rd_indextuple->t_self);

Anyway, as I have pointed upthread, I think that the craziness is also
in validatePartitionedIndex() where this stuff thinks that it is OK to
use a copy the pg_index tuple coming from the relcache.  As this
report proves, it is *not* safe, because we may miss a lot of
information not updated by RelationReloadIndexInfo() that other
commands in the same transaction block may have updated, and the
code would insert into the catalog an inconsistent tuple for a
partitioned index switched to become valid.

I agree that updating indisreplident in this cheap index reload path
is necessary, as well.  Does my suggestion of using the syscache not
make sense for somebody here?  Note that this is what all the other
code paths do for catalog updates of pg_index when retrieving a copy
of its tuples.
--
Michael