Re: Exclusion constraints on partitioned tables

On 17.03.23 17:03, Paul Jungwirth wrote:
> Thank you for taking a look! I did some research on the history of the
> code here, and I think I understand Tom's concern about making sure the
> index uses the same equality operator as the partition. I was confused
> about his remarks about the opfamily, but I agree with you that if the
> operator is the same, we should be okay.
>
> I added the code about RTEqualStrategyNumber because that's what we need
> to find an equals operator when the index is GiST (except if it's using
> an opclass from btree_gist; then it needs to be BTEqual again). But then
> I realized that for exclusion constraints we have already figured out
> the operator (in RelationGetExclusionInfo) and put it in
> indexInfo->ii_ExclusionOps. So we can just compare against that. This
> works whether your index uses btree_gist or not.
>
> Here is an updated patch with that change (also rebased).
>
> I also included a more specific error message. If we find a matching
> column in the index but with the wrong operator, we should say so, and
> not say there is no matching column.

This looks all pretty good to me. A few more comments:

It seems to me that many of the test cases added in indexing.sql are
redundant with create_table.sql/alter_table.sql (or vice versa). Is
there a reason for this?

This is not really a problem in your patch, but I think in

- if (partitioned && (stmt->unique || stmt->primary))
+ if (partitioned && (stmt->unique || stmt->primary ||
stmt->excludeOpNames != NIL))

the stmt->primary is redundant and should be removed. Right now
"primary" is always a subset of "unique", but presumably a future patch
of yours wants to change that.

Furthermore, I think it would be more elegant in your patch if you wrote
stmt->excludeOpNames without the "== NIL" or "!= NIL", so that it
becomes a peer of stmt->unique. (I understand some people don't like
that style. But it is already used in that file.)

I would consider rearranging some of the conditionals more as a
selection of cases, like "is it a unique constraint?", "else, is it an
exclusion constraint?" -- rather than the current "is it an exclusion
constraint?, "else, various old code". For example, instead of

if (stmt->excludeOpNames != NIL)
idx_eqop = indexInfo->ii_ExclusionOps[j];
else
idx_eqop = get_opfamily_member(..., eq_strategy);

consider

if (stmt->unique)
idx_eqop = get_opfamily_member(..., eq_strategy);
else if (stmt->excludeOpNames)
idx_eqop = indexInfo->ii_ExclusionOps[j];
Assert(idx_eqop);

Also, I would push the code

if (accessMethodId == BTREE_AM_OID)
eq_strategy = BTEqualStrategyNumber;

further down into the loop, so that you don't have to remember in which
cases eq_strategy is assigned or not.

(It's also confusing that the eq_strategy variable is used for two
different things in this function, and that would clean that up.)

Finally, this code

+ att = TupleDescAttr(RelationGetDescr(rel),
+ key->partattrs[i] - 1);
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot match partition key
to index on column \"%s\" using non-equal operator \"%s\".",
+ NameStr(att->attname),
get_opname(indexInfo->ii_ExclusionOps[j]))));

could be simplified by using get_attname().

This is all just a bit of polishing. I think it would be good to go
after that.