Re: [HACKERS] Assert failure when rechecking an exclusion constraint
Noah Misch n...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Assert failure when rechecking an exclusion constraint
I wrote:
Noah Misch n...@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
Re: [HACKERS] Assert failure when rechecking an exclusion constraint
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Assert failure when rechecking an exclusion constraint
Jeff Davis pg...@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 treated as 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Assert failure when rechecking an exclusion constraint
On Sun, Jun 05, 2011 at 02:17:00PM -0400, Tom Lane wrote: I wrote: Noah Misch n...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Assert failure when rechecking an exclusion constraint
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Assert failure when rechecking an exclusion constraint
Noah Misch n...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Assert failure when rechecking an exclusion constraint
Noah Misch n...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Assert failure when rechecking an exclusion constraint
On Sat, Jun 04, 2011 at 05:49:31PM -0400, Tom Lane wrote: Noah Misch n...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
