Re: REINDEX INDEX results in a crash for an index of pg_class since 9.6

Hi,

On 2019-05-01 22:01:53 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > Well, as I said before, I think hiding the to-be-rebuilt index from the
> > list of indexes is dangerous too - if somebody added an actual
> > CatalogUpdate/Insert (rather than inplace_update) anywhere along the
> > index_build() path, we'd not get an assertion failure anymore, but just
> > an index without the new entry. And given the fragility with HOT hiding
> > that a lot of the time, that seems dangerous to me.
>
> I think that argument is pretty pointless considering that "REINDEX TABLE
> pg_class" does it this way, and that code is nearly old enough to
> vote.

IMO the reindex_relation() case isn't comparable. By my read the main
purpose there is to prevent inserting into not-yet-rebuilt indexes. The
relevant comment says:
* .... If we are processing pg_class itself, we want to make sure
* that the updates do not try to insert index entries into indexes we
* have not processed yet. (When we are trying to recover from corrupted
* indexes, that could easily cause a crash.)

Note the *not processed yet* bit. That's *not* comparable logic to
hiding the index that *already* has been rebuilt, in the middle of
reindex_index(). Yes, the way reindex_relation() is currently coded,
the RelationSetIndexList() *also* hides the already rebuilt index, but
that's hard for reindex_relation() to avoid, because it's outside of
reindex_index().

> + * If we are doing one index for reindex_relation, then we will find that
> + * the index is already not present in the index list. In that case we
> + * don't have to do anything to the index list here, which we mark by
> + * clearing is_pg_class.
> */

> - RelationSetNewRelfilenode(iRel, persistence);
> + is_pg_class = (RelationGetRelid(heapRelation) == RelationRelationId);
> + if (is_pg_class)
> + {
> + allIndexIds = RelationGetIndexList(heapRelation);
> + if (list_member_oid(allIndexIds, indexId))
> + {
> + otherIndexIds = list_delete_oid(list_copy(allIndexIds), indexId);
> + /* Ensure rd_indexattr is valid; see comments for RelationSetIndexList */
> + (void) RelationGetIndexAttrBitmap(heapRelation, INDEX_ATTR_BITMAP_ALL);
> + }
> + else
> + is_pg_class = false;
> + }

That's not pretty either :(

Greetings,

Andres Freund