Re: Index AM API cleanup - Mailing list pgsql-hackers

I have committed these four patches (squashed into three).  I made the 
error handling change in 0003 that you requested, and also the error 
handling change in 0002 discussed in an adjacent message.


On 12.03.25 16:52, Mark Dilger wrote:
> 
> 
> On Wed, Mar 12, 2025 at 7:00 AM Peter Eisentraut <peter@eisentraut.org 
> <mailto:peter@eisentraut.org>> wrote:
> 
>     And another small patch set extracted from the bigger one, to keep
>     things moving along:
> 
>     0001: Add get_opfamily_method() in lsyscache.c, from your patch set.
> 
> 
> Right, this came from v21-0006-*, with a slight code comment change and 
> one variable renaming.  It is ready for commit.
> 
>     0002: Add get_opfamily_member_for_cmptype().  This was called
>     get_opmethod_member() in your patch set, but I think that name wasn't
>     quite right.  I also removed the opmethod argument, which was rarely
>     used and is somewhat redundant.
> 
> 
> This is also taken from v21-0006-*.  The reason I had an opmethod 
> argument was that some of the callers of this function already know the 
> opmethod, and without the argument, this function has to look up the 
> opmethod from the syscache again.  Whether that makes a measurable 
> performance difference is an open question.
> Your version is ready for commit.  If we want to reintroduce the 
> opmethod argument for performance reasons, we can always circle back to 
> that in a later commit.
> 
>     0003 and 0004 are enabling non-btree unique indexes for partition keys
> 
> 
> You refactored v21-0011-* into v21.2-0003-*, in which an error gets 
> raised about a missing operator in a slightly different part of the 
> logic.  I am concerned that the new positioning of the check-and-error 
> might allow the flow of execution to reach the Assert(idx_eqop) 
> statement in situations where the user has defined an incomplete 
> opfamily or opclass.  Such a condition should raise an error about the 
> missing operator rather than asserting.
> 
> In particular, looking at the control logic:
> 
>                         if (stmt->unique && !stmt->iswithoutoverlaps)
>                          {
>                                 ....
>                          }
>                          else if (exclusion)
>                                 ....;
> 
>                          Assert(idx_eqop);
> 
> I cannot prove to myself that the assertion cannot trigger, because the 
> upstream logic before we reach this point *might* be filtering out all 
> cases where this could be a problem, but it is too complicated to 
> prove.  Even if it is impossible now, this is a pretty brittle piece of 
> code after applying the patch.
> 
> Any chance you'd like to keep the patch closer to how I had it in 
> v21-0011-* ?
> 
>     and materialized views.  These were v21-0011 and v21-0012, except that
>     I'm combining the switch from strategies to compare types (which was in
>     v21-0006 or so) with the removal of the btree requirements.
> 
> 
> v21.2-0004-* is ready for commit.
> 
> —
> Mark Dilger
> EnterpriseDB:http://www.enterprisedb.com <http://www.enterprisedb.com/>
> The Enterprise PostgreSQL Company