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

On Thu, Jul 13, 2023 at 3:56 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.

Yeah, It seems that using pg_index tuples from relcache is not safe,
at least for updating the catalog tuples.  However, are there known
rules or do we need to add some comments saying that the pg_index
tuple from the relcache cannot be used to update the catalog tuple?
Or do we actually need to update all the tuple header information as
well in RelationReloadIndexInfo() in order to fix that invariant so
that it can be used for catalog tuple updates as well?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com