Feedback-ID: ie0a040ee:Fastmail
Message-ID: <c28f282d-1966-2a2f-927a-ea457887dd76@eisentraut.org>
Date: Thu, 6 Jul 2023 10:03:20 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.12.0
Subject: Re: Exclusion constraints on partitioned tables
Content-Language: en-US
To: Paul Jungwirth <pj@illuminatedcomputing.com>,
 pgsql-hackers@lists.postgresql.org
Cc: Ronan Dunklau <ronan.dunklau@aiven.io>, Tom Lane <tgl@sss.pgh.pa.us>
References: <ec8b1d9b-502e-d1f8-e909-1bf9dffe6fa5@illuminatedcomputing.com>
 <3585070.1671149527@sss.pgh.pa.us>
 <9ad3257d-db5b-e54e-f5e0-a54727b19327@illuminatedcomputing.com>
 <10212835.nUPlyArG6x@aivenlaptop>
 <608a9a52-0aab-e3a1-40fa-55536b4ef6a9@illuminatedcomputing.com>
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <608a9a52-0aab-e3a1-40fa-55536b4ef6a9@illuminatedcomputing.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/c28f282d-1966-2a2f-927a-ea457887dd76%40eisentraut.org>
Precedence: bulk

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.