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

Andres Freund <andres@anarazel.de> writes:
> On 2019-05-01 19:41:24 -0400, Tom Lane wrote:
>> OK, so per the other thread, it seems like the error recovery problem
>> isn't going to affect this directly.  However, I still don't like this
>> proposal much; the reason being that it's a rather fundamental change
>> in the API of RelationSetNewRelfilenode.  This will certainly break
>> any external callers of that function --- and silently, too.

> Couldn't we just address that by adding a new
> RelationSetNewRelfilenodeInternal() that's then wrapped by
> RelationSetNewRelfilenode() which just does
> RelationSetNewRelfilenodeInternal();CCI();?

That's just adding more ugliness ...

>> The solution I'm thinking of should have much more localized effects,
>> basically just in reindex_index and RelationSetNewRelfilenode, which is
>> why I like it better.

> 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.
Perhaps there'd be value in rewriting things so that we don't need
RelationSetIndexList at all, but it's not real clear to me what we'd do
instead, and in any case I don't agree with back-patching such a change.
In the near term it seems better to me to make "REINDEX INDEX
some-pg_class-index" handle this problem the same way "REINDEX TABLE
pg_class" has been doing for many years.

Attached is a draft patch for this.  It passes check-world with
xxx_FORCE_RELEASE, and gets through reindexing pg_class with
CLOBBER_CACHE_ALWAYS, but I've not completed a full CCA run.

            regards, tom lane

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index ce02410..1af959c 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3261,6 +3261,9 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
                 heapRelation;
     Oid            heapId;
     IndexInfo  *indexInfo;
+    bool        is_pg_class;
+    List       *allIndexIds = NIL;
+    List       *otherIndexIds = NIL;
     volatile bool skipped_constraint = false;
     PGRUsage    ru0;

@@ -3331,19 +3334,52 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
     }

     /*
-     * Build a new physical relation for the index. Need to do that before
-     * "officially" starting the reindexing with SetReindexProcessing -
-     * otherwise the necessary pg_class changes cannot be made with
-     * encountering assertions.
+     * RelationSetNewRelfilenode will need to update the index's pg_class row.
+     * If we are reindexing some index of pg_class, that creates a problem;
+     * once we call SetReindexProcessing, the index support will complain if
+     * we try to insert a new index entry.  But we can't do that in the other
+     * order without creating other problems.  We solve this by temporarily
+     * removing the target index from pg_class's index list.  It won't get
+     * updated during RelationSetNewRelfilenode, but that's fine because the
+     * subsequent index build will include the new entry.  (There are more
+     * comments about this in reindex_relation.)
+     *
+     * 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;
+    }

-    /* ensure SetReindexProcessing state isn't leaked */
+    /*
+     * Ensure SetReindexProcessing state isn't leaked.  (We don't have to
+     * clean up the RelationSetIndexList state on error, though; transaction
+     * abort knows about fixing that.)
+     */
     PG_TRY();
     {
         /* Suppress use of the target index while rebuilding it */
         SetReindexProcessing(heapId, indexId);

+        /* ... and suppress updating it too, if necessary */
+        if (is_pg_class)
+            RelationSetIndexList(heapRelation, otherIndexIds);
+
+        /* Build a new physical relation for the index */
+        RelationSetNewRelfilenode(iRel, persistence);
+
         /* Initialize the index and rebuild */
         /* Note: we do not need to re-establish pkey setting */
         index_build(heapRelation, iRel, indexInfo, true, true);
@@ -3357,6 +3393,9 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
     PG_END_TRY();
     ResetReindexProcessing();

+    if (is_pg_class)
+        RelationSetIndexList(heapRelation, allIndexIds);
+
     /*
      * If the index is marked invalid/not-ready/dead (ie, it's from a failed
      * CREATE INDEX CONCURRENTLY, or a DROP INDEX CONCURRENTLY failed midway),