Re: Exclusion constraints on partitioned tables - Mailing list pgsql-hackers

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.