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.
