Re: Assert failure when rechecking an exclusion constraint - Mailing list pgsql-hackers

I wrote:
> Noah Misch <noah@leadboat.com> writes:
>> Sounds reasonable.  Need to remove the index from pendingReindexedIndexes, not
>> just call ResetReindexProcessing().

> [ looks again... ]  Uh, right.  I was thinking that the pending list was
> just "pending" and not "in progress" indexes.  I wonder if we should
> rejigger things so that that's actually true, ie, remove an index's OID
> from the pending list when we mark it as the current one?

Attached are two versions of a patch to fix this.  The second one
modifies the code that tracks what's "pending" as per the above thought.
I'm not entirely sure which one I like better ... any comments?

            regards, tom lane

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 53b4c3c59bf78cacf8def18ac4d46940a3a29b71..b046f38ecb8a47538cb12e036a6fca6c3509aec1 100644
*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
*************** static void validate_index_heapscan(Rela
*** 115,120 ****
--- 115,121 ----
                          Snapshot snapshot,
                          v_i_state *state);
  static Oid    IndexGetRelation(Oid indexId);
+ static bool ReindexIsCurrentlyProcessingIndex(Oid indexOid);
  static void SetReindexProcessing(Oid heapOid, Oid indexOid);
  static void ResetReindexProcessing(void);
  static void SetReindexPending(List *indexes);
*************** index_build(Relation heapRelation,
*** 1758,1776 ****
      }

      /*
-      * If it's for an exclusion constraint, make a second pass over the heap
-      * to verify that the constraint is satisfied.
-      */
-     if (indexInfo->ii_ExclusionOps != NULL)
-         IndexCheckExclusion(heapRelation, indexRelation, indexInfo);
-
-     /* Roll back any GUC changes executed by index functions */
-     AtEOXact_GUC(false, save_nestlevel);
-
-     /* Restore userid and security context */
-     SetUserIdAndSecContext(save_userid, save_sec_context);
-
-     /*
       * If we found any potentially broken HOT chains, mark the index as not
       * being usable until the current transaction is below the event horizon.
       * See src/backend/access/heap/README.HOT for discussion.
--- 1759,1764 ----
*************** index_build(Relation heapRelation,
*** 1824,1831 ****
                         InvalidOid,
                         stats->index_tuples);

!     /* Make the updated versions visible */
      CommandCounterIncrement();
  }


--- 1812,1834 ----
                         InvalidOid,
                         stats->index_tuples);

!     /* Make the updated catalog row versions visible */
      CommandCounterIncrement();
+
+     /*
+      * If it's for an exclusion constraint, make a second pass over the heap
+      * to verify that the constraint is satisfied.  We must not do this until
+      * the index is fully valid.  (Broken HOT chains shouldn't matter, though;
+      * see comments for IndexCheckExclusion.)
+      */
+     if (indexInfo->ii_ExclusionOps != NULL)
+         IndexCheckExclusion(heapRelation, indexRelation, indexInfo);
+
+     /* Roll back any GUC changes executed by index functions */
+     AtEOXact_GUC(false, save_nestlevel);
+
+     /* Restore userid and security context */
+     SetUserIdAndSecContext(save_userid, save_sec_context);
  }


*************** IndexCheckExclusion(Relation heapRelatio
*** 2270,2275 ****
--- 2273,2290 ----
      ExprContext *econtext;

      /*
+      * If we are reindexing the target index, mark it as no longer being
+      * reindexed, to forestall an Assert in index_beginscan when we try to
+      * use the index for probes.  This is OK because the index is now
+      * fully valid.
+      */
+     if (ReindexIsCurrentlyProcessingIndex(RelationGetRelid(indexRelation)))
+     {
+         ResetReindexProcessing();
+         RemoveReindexPending(RelationGetRelid(indexRelation));
+     }
+
+     /*
       * Need an EState for evaluation of index expressions and partial-index
       * predicates.    Also a slot to hold the current tuple.
       */
*************** reindex_relation(Oid relid, int flags)
*** 3030,3036 ****
   *        System index reindexing support
   *
   * When we are busy reindexing a system index, this code provides support
!  * for preventing catalog lookups from using that index.
   * ----------------------------------------------------------------
   */

--- 3045,3053 ----
   *        System index reindexing support
   *
   * When we are busy reindexing a system index, this code provides support
!  * for preventing catalog lookups from using that index.  We also make use
!  * of this to catch attempted uses of user indexes during reindexing of
!  * those indexes.
   * ----------------------------------------------------------------
   */

*************** ReindexIsProcessingHeap(Oid heapOid)
*** 3049,3054 ****
--- 3066,3081 ----
  }

  /*
+  * ReindexIsCurrentlyProcessingIndex
+  *        True if index specified by OID is currently being reindexed.
+  */
+ static bool
+ ReindexIsCurrentlyProcessingIndex(Oid indexOid)
+ {
+     return indexOid == currentlyReindexedIndex;
+ }
+
+ /*
   * ReindexIsProcessingIndex
   *        True if index specified by OID is currently being reindexed,
   *        or should be treated as invalid because it is awaiting reindex.
diff --git a/src/test/regress/input/constraints.source b/src/test/regress/input/constraints.source
index 0d278212c02a4801cd0919b4c2a134b448a2ac42..b84d51e9e5216f8e5899f6bb10d87e12a78e4dc1 100644
*** a/src/test/regress/input/constraints.source
--- b/src/test/regress/input/constraints.source
*************** INSERT INTO circles VALUES('<(20,20), 10
*** 397,402 ****
--- 397,405 ----
  ALTER TABLE circles ADD EXCLUDE USING gist
    (c1 WITH &&, (c2::circle) WITH &&);

+ -- try reindexing an existing constraint
+ REINDEX INDEX circles_c1_c2_excl;
+
  DROP TABLE circles;

  -- Check deferred exclusion constraint
diff --git a/src/test/regress/output/constraints.source b/src/test/regress/output/constraints.source
index d164b90af78e5cb818803e1f1bbadb90fe87c05a..e2f2939931580e1796fc3fde7f4cebf25553d02e 100644
*** a/src/test/regress/output/constraints.source
--- b/src/test/regress/output/constraints.source
*************** ALTER TABLE circles ADD EXCLUDE USING gi
*** 543,548 ****
--- 543,550 ----
  NOTICE:  ALTER TABLE / ADD EXCLUDE will create implicit index "circles_c1_c2_excl1" for table "circles"
  ERROR:  could not create exclusion constraint "circles_c1_c2_excl1"
  DETAIL:  Key (c1, (c2::circle))=(<(0,0),5>, <(0,0),5>) conflicts with key (c1, (c2::circle))=(<(0,0),5>, <(0,0),4>).
+ -- try reindexing an existing constraint
+ REINDEX INDEX circles_c1_c2_excl;
  DROP TABLE circles;
  -- Check deferred exclusion constraint
  CREATE TABLE deferred_excl (
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 53b4c3c59bf78cacf8def18ac4d46940a3a29b71..1b39e1683c709ab866de16ccd199ef92ed2db8ec 100644
*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
*************** static void validate_index_heapscan(Rela
*** 115,120 ****
--- 115,121 ----
                          Snapshot snapshot,
                          v_i_state *state);
  static Oid    IndexGetRelation(Oid indexId);
+ static bool ReindexIsCurrentlyProcessingIndex(Oid indexOid);
  static void SetReindexProcessing(Oid heapOid, Oid indexOid);
  static void ResetReindexProcessing(void);
  static void SetReindexPending(List *indexes);
*************** index_build(Relation heapRelation,
*** 1758,1776 ****
      }

      /*
-      * If it's for an exclusion constraint, make a second pass over the heap
-      * to verify that the constraint is satisfied.
-      */
-     if (indexInfo->ii_ExclusionOps != NULL)
-         IndexCheckExclusion(heapRelation, indexRelation, indexInfo);
-
-     /* Roll back any GUC changes executed by index functions */
-     AtEOXact_GUC(false, save_nestlevel);
-
-     /* Restore userid and security context */
-     SetUserIdAndSecContext(save_userid, save_sec_context);
-
-     /*
       * If we found any potentially broken HOT chains, mark the index as not
       * being usable until the current transaction is below the event horizon.
       * See src/backend/access/heap/README.HOT for discussion.
--- 1759,1764 ----
*************** index_build(Relation heapRelation,
*** 1824,1831 ****
                         InvalidOid,
                         stats->index_tuples);

!     /* Make the updated versions visible */
      CommandCounterIncrement();
  }


--- 1812,1834 ----
                         InvalidOid,
                         stats->index_tuples);

!     /* Make the updated catalog row versions visible */
      CommandCounterIncrement();
+
+     /*
+      * If it's for an exclusion constraint, make a second pass over the heap
+      * to verify that the constraint is satisfied.  We must not do this until
+      * the index is fully valid.  (Broken HOT chains shouldn't matter, though;
+      * see comments for IndexCheckExclusion.)
+      */
+     if (indexInfo->ii_ExclusionOps != NULL)
+         IndexCheckExclusion(heapRelation, indexRelation, indexInfo);
+
+     /* Roll back any GUC changes executed by index functions */
+     AtEOXact_GUC(false, save_nestlevel);
+
+     /* Restore userid and security context */
+     SetUserIdAndSecContext(save_userid, save_sec_context);
  }


*************** IndexCheckExclusion(Relation heapRelatio
*** 2270,2275 ****
--- 2273,2287 ----
      ExprContext *econtext;

      /*
+      * If we are reindexing the target index, mark it as no longer being
+      * reindexed, to forestall an Assert in index_beginscan when we try to
+      * use the index for probes.  This is OK because the index is now
+      * fully valid.
+      */
+     if (ReindexIsCurrentlyProcessingIndex(RelationGetRelid(indexRelation)))
+         ResetReindexProcessing();
+
+     /*
       * Need an EState for evaluation of index expressions and partial-index
       * predicates.    Also a slot to hold the current tuple.
       */
*************** reindex_relation(Oid relid, int flags)
*** 2989,2996 ****

              CommandCounterIncrement();

!             if (flags & REINDEX_REL_SUPPRESS_INDEX_USE)
!                 RemoveReindexPending(indexOid);

              if (is_pg_class)
                  doneIndexes = lappend_oid(doneIndexes, indexOid);
--- 3001,3008 ----

              CommandCounterIncrement();

!             /* Index should no longer be in the pending list */
!             Assert(!ReindexIsProcessingIndex(indexOid));

              if (is_pg_class)
                  doneIndexes = lappend_oid(doneIndexes, indexOid);
*************** reindex_relation(Oid relid, int flags)
*** 3030,3036 ****
   *        System index reindexing support
   *
   * When we are busy reindexing a system index, this code provides support
!  * for preventing catalog lookups from using that index.
   * ----------------------------------------------------------------
   */

--- 3042,3050 ----
   *        System index reindexing support
   *
   * When we are busy reindexing a system index, this code provides support
!  * for preventing catalog lookups from using that index.  We also make use
!  * of this to catch attempted uses of user indexes during reindexing of
!  * those indexes.
   * ----------------------------------------------------------------
   */

*************** ReindexIsProcessingHeap(Oid heapOid)
*** 3049,3054 ****
--- 3063,3078 ----
  }

  /*
+  * ReindexIsCurrentlyProcessingIndex
+  *        True if index specified by OID is currently being reindexed.
+  */
+ static bool
+ ReindexIsCurrentlyProcessingIndex(Oid indexOid)
+ {
+     return indexOid == currentlyReindexedIndex;
+ }
+
+ /*
   * ReindexIsProcessingIndex
   *        True if index specified by OID is currently being reindexed,
   *        or should be treated as invalid because it is awaiting reindex.
*************** SetReindexProcessing(Oid heapOid, Oid in
*** 3075,3080 ****
--- 3099,3106 ----
          elog(ERROR, "cannot reindex while reindexing");
      currentlyReindexedHeap = heapOid;
      currentlyReindexedIndex = indexOid;
+     /* Index is no longer "pending" reindex. */
+     RemoveReindexPending(indexOid);
  }

  /*
diff --git a/src/test/regress/input/constraints.source b/src/test/regress/input/constraints.source
index 0d278212c02a4801cd0919b4c2a134b448a2ac42..b84d51e9e5216f8e5899f6bb10d87e12a78e4dc1 100644
*** a/src/test/regress/input/constraints.source
--- b/src/test/regress/input/constraints.source
*************** INSERT INTO circles VALUES('<(20,20), 10
*** 397,402 ****
--- 397,405 ----
  ALTER TABLE circles ADD EXCLUDE USING gist
    (c1 WITH &&, (c2::circle) WITH &&);

+ -- try reindexing an existing constraint
+ REINDEX INDEX circles_c1_c2_excl;
+
  DROP TABLE circles;

  -- Check deferred exclusion constraint
diff --git a/src/test/regress/output/constraints.source b/src/test/regress/output/constraints.source
index d164b90af78e5cb818803e1f1bbadb90fe87c05a..e2f2939931580e1796fc3fde7f4cebf25553d02e 100644
*** a/src/test/regress/output/constraints.source
--- b/src/test/regress/output/constraints.source
*************** ALTER TABLE circles ADD EXCLUDE USING gi
*** 543,548 ****
--- 543,550 ----
  NOTICE:  ALTER TABLE / ADD EXCLUDE will create implicit index "circles_c1_c2_excl1" for table "circles"
  ERROR:  could not create exclusion constraint "circles_c1_c2_excl1"
  DETAIL:  Key (c1, (c2::circle))=(<(0,0),5>, <(0,0),5>) conflicts with key (c1, (c2::circle))=(<(0,0),5>, <(0,0),4>).
+ -- try reindexing an existing constraint
+ REINDEX INDEX circles_c1_c2_excl;
  DROP TABLE circles;
  -- Check deferred exclusion constraint
  CREATE TABLE deferred_excl (