Обсуждение: Inconsistency between try_mergejoin_path and create_mergejoin_plan

Hi.

There's the following inconsistency between try_mergejoin_path() and 
create_mergejoin_plan().
When clause operator has no commutator, we can end up with mergejoin 
path.
Later create_mergejoin_plan() will call get_switched_clauses(). This 
function can error out with

ERROR:  could not find commutator for operator XXX

The similar behavior seems to be present also for hash join.

Attaching a test case (in patch) and a possible fix.

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

On Mon, Jun 17, 2024 at 10:51 PM Alexander Pyhalov
<a.pyhalov@postgrespro.ru> wrote:
> There's the following inconsistency between try_mergejoin_path() and
> create_mergejoin_plan().
> When clause operator has no commutator, we can end up with mergejoin
> path.
> Later create_mergejoin_plan() will call get_switched_clauses(). This
> function can error out with
>
> ERROR:  could not find commutator for operator XXX

Interesting.  This error can be reproduced with table 'ec1' from
sql/equivclass.sql.

set enable_indexscan to off;

explain select * from ec1 t1 join ec1 t2 on t2.ff = t1.f1;
ERROR:  could not find commutator for operator 30450

The column ec1.f1 has a type of 'int8alias1', a new data type created in
this test file.  Additionally, there is also a newly created operator
'int8 = int8alias1' which is mergejoinable but lacks a valid commutator.
Therefore, there is no problem generating the mergejoin path, but when
we create the mergejoin plan, get_switched_clauses would notice the
absence of a valid commutator needed to commute the clause.

It seems to me that the new operator is somewhat artificial, since it is
designed to support a mergejoin but lacks a valid commutator.  So before
we proceed to discuss the fix, I'd like to know whether this is a valid
issue that needs fixing.

Any thoughts?

Thanks
Richard

Richard Guo <guofenglinux@gmail.com> writes:
> On Mon, Jun 17, 2024 at 10:51 PM Alexander Pyhalov
> <a.pyhalov@postgrespro.ru> wrote:
>> ERROR:  could not find commutator for operator XXX

> It seems to me that the new operator is somewhat artificial, since it is
> designed to support a mergejoin but lacks a valid commutator.  So before
> we proceed to discuss the fix, I'd like to know whether this is a valid
> issue that needs fixing.

Well, there's no doubt that the case is artificial: nobody who knew
what they were doing would install an incomplete opclass like this
in a production setting.  However, there are several parts of the
planner that take pains to avoid this type of failure.  I am pretty
sure that we are careful about flipping around candidate indexscan
quals for instance.  And the "broken equivalence class" mechanism
is all about that, which is what equivclass.sql is setting up this
opclass to test.  So I find it a bit sad if mergejoin creation is
tripping over this case.

I do not think we should add a great deal of complexity or extra
planner cycles to deal with this; but if it can be fixed at low
cost, we should.

            regards, tom lane

On Wed, Jun 19, 2024 at 12:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Richard Guo <guofenglinux@gmail.com> writes:
> > It seems to me that the new operator is somewhat artificial, since it is
> > designed to support a mergejoin but lacks a valid commutator.  So before
> > we proceed to discuss the fix, I'd like to know whether this is a valid
> > issue that needs fixing.

> I do not think we should add a great deal of complexity or extra
> planner cycles to deal with this; but if it can be fixed at low
> cost, we should.

I think we can simply verify the validity of commutators for clauses in
the form "inner op outer" when selecting mergejoin/hash clauses.  If a
clause lacks a commutator, we should consider it unusable for the given
pair of outer and inner rels.  Please see the attached patch.

Thanks
Richard

Richard Guo писал(а) 2024-06-19 16:30:
> On Wed, Jun 19, 2024 at 12:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Richard Guo <guofenglinux@gmail.com> writes:
>> > It seems to me that the new operator is somewhat artificial, since it is
>> > designed to support a mergejoin but lacks a valid commutator.  So before
>> > we proceed to discuss the fix, I'd like to know whether this is a valid
>> > issue that needs fixing.
> 
>> I do not think we should add a great deal of complexity or extra
>> planner cycles to deal with this; but if it can be fixed at low
>> cost, we should.
> 
> I think we can simply verify the validity of commutators for clauses in
> the form "inner op outer" when selecting mergejoin/hash clauses.  If a
> clause lacks a commutator, we should consider it unusable for the given
> pair of outer and inner rels.  Please see the attached patch.
> 

This seems to be working for my test cases.
-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

On Wed, Jun 19, 2024 at 10:24 PM Alexander Pyhalov
<a.pyhalov@postgrespro.ru> wrote:
> Richard Guo писал(а) 2024-06-19 16:30:
> > I think we can simply verify the validity of commutators for clauses in
> > the form "inner op outer" when selecting mergejoin/hash clauses.  If a
> > clause lacks a commutator, we should consider it unusable for the given
> > pair of outer and inner rels.  Please see the attached patch.

> This seems to be working for my test cases.

Thank you for confirming.  Here is an updated patch with some tweaks to
the comments and commit message.  I've parked this patch in the July
commitfest.

Thanks
Richard