Re: 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=<value optimized 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') at execUtils.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=<value optimized 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 optimized out>, 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(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)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

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(at)j-davis(dot)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

On Sun, Jun 05, 2011 at 02:17:00PM -0400, Tom Lane wrote:
> I wrote:
> > Noah Misch <noah(at)leadboat(dot)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(at)leadboat(dot)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