Thread: Assert failure when rechecking an exclusion constraint

Commit d2f60a3ab055fb61c8e1056a7c5652f1dec85e00 added an assert to indexam.c's
RELATION_CHECKS to block use of an index while it's being rebuilt.  This
assert trips while rechecking an exclusion constraint after an ALTER TABLE
rebuild:
CREATE TABLE t (    c    int,    EXCLUDE (c WITH =));
INSERT INTO t VALUES (1), (2);ALTER TABLE t ALTER c TYPE smallint USING 1;

Here's the end of the stack trace (at -O1):

#0  0x00007f3d2bae3095 in raise () from /lib/libc.so.6
#1  0x00007f3d2bae4af0 in abort () from /lib/libc.so.6
#2  0x0000000000705449 in ExceptionalCondition (conditionName=<value optimized out>, errorType=<value optimized out>,
fileName=<valueoptimized out>, lineNumber=<value optimized out>) at assert.c:57
 
#3  0x0000000000486148 in index_beginscan_internal (indexRelation=0x7f3d2c6990a0, nkeys=1, norderbys=0) at
indexam.c:283
#4  0x000000000048622c in index_beginscan (heapRelation=0x7f3d2c68fb48, indexRelation=0x6a73, snapshot=0x7fff679d17b0,
nkeys=-1,norderbys=0) at indexam.c:237
 
#5  0x000000000058a375 in check_exclusion_constraint (heap=0x7f3d2c68fb48, index=0x7f3d2c6990a0, indexInfo=0xc97968,
tupleid=0xc8c06c,values=0x7fff679d18d0, isnull=0x7fff679d19e0 "", estate=0xc98ac8, newIndex=1 '\001', errorOK=0 '\000')
atexecUtils.c:1222
 
#6  0x00000000004d164e in IndexCheckExclusion (heapRelation=0x7f3d2c68fb48, indexRelation=0x7f3d2c6990a0,
indexInfo=0xc97968,isprimary=0 '\000', isreindex=1 '\001') at index.c:2328
 
#7  index_build (heapRelation=0x7f3d2c68fb48, indexRelation=0x7f3d2c6990a0, indexInfo=0xc97968, isprimary=0 '\000',
isreindex=1'\001') at index.c:1765
 
#8  0x00000000004d21b8 in reindex_index (indexId=131529, skip_constraint_checks=0 '\000') at index.c:2814
#9  0x00000000004d2450 in reindex_relation (relid=<value optimized out>, flags=6) at index.c:2988
#10 0x000000000052a86a in finish_heap_swap (OIDOldHeap=131524, OIDNewHeap=131531, is_system_catalog=0 '\000',
swap_toast_by_content=<valueoptimized out>, check_constraints=1 '\001', frozenXid=<value optimized out>) at
cluster.c:1427
#11 0x000000000055fc1b in ATRewriteTables (rel=<value optimized out>, cmds=<value optimized out>, recurse=<value
optimizedout>, lockmode=8) at tablecmds.c:3329
 
#12 ATController (rel=<value optimized out>, cmds=<value optimized out>, recurse=<value optimized out>, lockmode=8) at
tablecmds.c:2738
...

I could not come up with an actual wrong behavior arising from this usage, so
I'll tentatively call it a false positive.  reindex_index() could instead
unconditionally clear indexInfo->ii_Exclusion* before calling index_build(),
then pop pendingReindexedIndexes and call IndexCheckExclusion() itself.  Popping
pendingReindexedIndexes as we go can make the success of a reindex_relation()
dependent on the order in which we choose to rebuild indexes, though.

Another option is to just remove the assert as not worth preserving.

Opinions, other ideas?

Thanks,
nm

Noah Misch <noah@leadboat.com> writes:
> Commit d2f60a3ab055fb61c8e1056a7c5652f1dec85e00 added an assert to indexam.c's
> RELATION_CHECKS to block use of an index while it's being rebuilt.  This
> assert trips while rechecking an exclusion constraint after an ALTER TABLE
> rebuild:

>     CREATE TABLE t (
>         c    int,
>         EXCLUDE (c WITH =)
>     );

>     INSERT INTO t VALUES (1), (2);
>     ALTER TABLE t ALTER c TYPE smallint USING 1;

Mph ... obviously not tested enough ...

> I could not come up with an actual wrong behavior arising from this usage, so
> I'll tentatively call it a false positive.  reindex_index() could instead
> unconditionally clear indexInfo->ii_Exclusion* before calling index_build(),
> then pop pendingReindexedIndexes and call IndexCheckExclusion() itself.  Popping
> pendingReindexedIndexes as we go can make the success of a reindex_relation()
> dependent on the order in which we choose to rebuild indexes, though.

> Another option is to just remove the assert as not worth preserving.

Removing the assert would be a seriously bad idea IMO.  I think we could
just allow index_build to call ResetReindexProcessing() midstream (ie,
before it calls IndexCheckExclusion).  This does raise the question of
whether the existing call to IndexCheckExclusion is badly placed and
we should move it to after the index is "fully" rebuilt.  That would
allow us to avoid doing ResetReindexProcessing until the index is
clearly safe to use.

So in short, I'm thinking move lines 1760-1772 (in HEAD) of index.c to
the end of index_build(), then insert a ResetReindexProcessing() call in
front of them; or maybe only do ResetReindexProcessing there if we
actually do call IndexCheckExclusion.
        regards, tom lane

On Sat, Jun 04, 2011 at 05:49:31PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > I could not come up with an actual wrong behavior arising from this usage, so
> > I'll tentatively call it a false positive.  reindex_index() could instead
> > unconditionally clear indexInfo->ii_Exclusion* before calling index_build(),
> > then pop pendingReindexedIndexes and call IndexCheckExclusion() itself.  Popping
> > pendingReindexedIndexes as we go can make the success of a reindex_relation()
> > dependent on the order in which we choose to rebuild indexes, though.
> 
> > Another option is to just remove the assert as not worth preserving.
> 
> Removing the assert would be a seriously bad idea IMO.  I think we could
> just allow index_build to call ResetReindexProcessing() midstream (ie,
> before it calls IndexCheckExclusion).  This does raise the question of
> whether the existing call to IndexCheckExclusion is badly placed and
> we should move it to after the index is "fully" rebuilt.  That would
> allow us to avoid doing ResetReindexProcessing until the index is
> clearly safe to use.
> 
> So in short, I'm thinking move lines 1760-1772 (in HEAD) of index.c to
> the end of index_build(), then insert a ResetReindexProcessing() call in
> front of them; or maybe only do ResetReindexProcessing there if we
> actually do call IndexCheckExclusion.

Sounds reasonable.  Need to remove the index from pendingReindexedIndexes, not
just call ResetReindexProcessing().  Also, wouldn't that specific construction
make the catalog updates fail due to running in the table owner's security
context?  But certainly something along those lines will do.

Noah Misch <noah@leadboat.com> writes:
> On Sat, Jun 04, 2011 at 05:49:31PM -0400, Tom Lane wrote:
>> So in short, I'm thinking move lines 1760-1772 (in HEAD) of index.c to
>> the end of index_build(), then insert a ResetReindexProcessing() call in
>> front of them; or maybe only do ResetReindexProcessing there if we
>> actually do call IndexCheckExclusion.

> 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?

> Also, wouldn't that specific construction
> make the catalog updates fail due to running in the table owner's security
> context?

AFAIR there's no security checks happening below this level.
        regards, tom lane

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 (

On Sun, 2011-06-05 at 14:17 -0400, Tom Lane wrote:
> 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?

I think I'm missing something simple: if it's not in the pending list,
what prevents systable_beginscan() from using it?

Regards,Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:
> On Sun, 2011-06-05 at 14:17 -0400, Tom Lane wrote:
>> 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?

> I think I'm missing something simple: if it's not in the pending list,
> what prevents systable_beginscan() from using it?

The test that uses is

/** ReindexIsProcessingIndex*        True if index specified by OID is currently being reindexed,*        or should be
treatedas invalid because it is awaiting reindex.*/
 
bool
ReindexIsProcessingIndex(Oid indexOid)
{return indexOid == currentlyReindexedIndex ||    list_member_oid(pendingReindexedIndexes, indexOid);
}

so once we've set the index as the currentlyReindexedIndex, there's
no need for it still to be in pendingReindexedIndexes.

(Arguably, this function should have been renamed when it was changed
to look at pendingReindexedIndexes too ...)
        regards, tom lane

On Sun, Jun 05, 2011 at 02:17:00PM -0400, Tom Lane wrote:
> 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?

+1 for the second approach.  It had bothered me that SetReindexProcessing() and
ResetReindexProcessing() manipulated one thing, but ReindexIsProcessingIndex()
checked something else besides.  (That's still technically true, but the overall
effect seems more natural.)

Thanks,
nm

On Sun, 2011-06-05 at 15:09 -0400, Tom Lane wrote:
> so once we've set the index as the currentlyReindexedIndex, there's
> no need for it still to be in pendingReindexedIndexes.

OK. The second version of the patch looks good to me.

Regards,Jeff Davis

Noah Misch <noah@leadboat.com> writes:
> On Sun, Jun 05, 2011 at 02:17:00PM -0400, Tom Lane wrote:
>> 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?

> +1 for the second approach.  It had bothered me that SetReindexProcessing() and
> ResetReindexProcessing() manipulated one thing, but ReindexIsProcessingIndex()
> checked something else besides.  (That's still technically true, but the overall
> effect seems more natural.)

OK, done that way.
        regards, tom lane