From 45e2aa24c3b5d550e8be6ace105f653f6d52e826 Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Wed, 19 Jun 2024 19:57:40 +0900 Subject: [PATCH v2] Check the validity of commutators for merge/hash clauses When creating merge or hash join plans in createplan.c, the merge or hash clauses may need to get commuted to ensure that the outer var is on the left and the inner var is on the right if they are not already in the expected form. This requires that their operators have commutators. Failing to find a commutator at this stage would result in 'ERROR: could not find commutator for operator xxx', with no opportunity to select an alternative plan. Typically, this is not an issue because mergejoinable or hashable operators are expected to always have valid commutators. But in some artificial cases this assumption may not hold true. So this patch adds checks to verify the validity of commutators for clauses in the form "inner op outer" when selecting mergejoin/hash clauses. --- src/backend/optimizer/path/joinpath.c | 27 ++++++++++ src/test/regress/expected/join.out | 71 +++++++++++++++++++++++++++ src/test/regress/sql/join.sql | 58 ++++++++++++++++++++++ 3 files changed, 156 insertions(+) diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c index 5be8da9e09..bef1944196 100644 --- a/src/backend/optimizer/path/joinpath.c +++ b/src/backend/optimizer/path/joinpath.c @@ -24,6 +24,7 @@ #include "optimizer/pathnode.h" #include "optimizer/paths.h" #include "optimizer/planmain.h" +#include "utils/lsyscache.h" #include "utils/typcache.h" /* Hook for plugins to get control in add_paths_to_joinrel() */ @@ -2131,6 +2132,17 @@ hash_inner_and_outer(PlannerInfo *root, if (!clause_sides_match_join(restrictinfo, outerrel, innerrel)) continue; /* no good for these input relations */ + /* + * If clause has the form "inner op outer", check if its operator has + * valid commutator. This is necessary because hashclauses will get + * commuted if needed in createplan.c to put the outer var on the left + * (see get_switched_clauses). This probably shouldn't ever fail, + * since hashable operators ought to have commutators, but be paranoid. + */ + if (!restrictinfo->outer_is_left && + !OidIsValid(get_commutator(((OpExpr *) restrictinfo->clause)->opno))) + continue; + hashclauses = lappend(hashclauses, restrictinfo); } @@ -2394,6 +2406,21 @@ select_mergejoin_clauses(PlannerInfo *root, continue; /* no good for these input relations */ } + /* + * If clause has the form "inner op outer", check if its operator has + * valid commutator. This is necessary because mergejoin clauses will + * get commuted if needed in createplan.c to put the outer var on the + * left (see get_switched_clauses). This probably shouldn't ever fail, + * since mergejoinable operators ought to have commutators, but be + * paranoid. + */ + if (!restrictinfo->outer_is_left && + !OidIsValid(get_commutator(((OpExpr *) restrictinfo->clause)->opno))) + { + have_nonmergeable_joinclause = true; + continue; + } + /* * Insist that each side have a non-redundant eclass. This * restriction is needed because various bits of the planner expect diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 6b16c3a676..fb2c88d2a6 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -7960,3 +7960,74 @@ SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS RESET enable_indexonlyscan; RESET enable_seqscan; +-- +-- test handling of merge/hash clauses that do not have valid commutators +-- +-- Typically there are not any such operators that are mergejoinable or +-- hashable but have no commutators; so we have to hack things up to create one. +create type int8nocommutator; +create function int8nocommutatorin(cstring) returns int8nocommutator + strict immutable language internal as 'int8in'; +NOTICE: return type int8nocommutator is only a shell +create function int8nocommutatorout(int8nocommutator) returns cstring + strict immutable language internal as 'int8out'; +NOTICE: argument type int8nocommutator is only a shell +create type int8nocommutator ( + input = int8nocommutatorin, + output = int8nocommutatorout, + like = int8 +); +create function int8nocommutatoreq(int8, int8nocommutator) returns bool + strict immutable language internal as 'int8eq'; +create operator = ( + procedure = int8nocommutatoreq, + leftarg = int8, rightarg = int8nocommutator, + restrict = eqsel, join = eqjoinsel, + hashes, merges +); +alter operator family integer_ops using btree add + operator 3 = (int8, int8nocommutator); +create function int8nocommutatorlt(int8nocommutator, int8nocommutator) returns bool + strict immutable language internal as 'int8lt'; +create operator < ( + procedure = int8nocommutatorlt, + leftarg = int8nocommutator, rightarg = int8nocommutator +); +alter operator family integer_ops using btree add + operator 1 < (int8nocommutator, int8nocommutator); +create function int8nocommutatorcmp(int8, int8nocommutator) returns int + strict immutable language internal as 'btint8cmp'; +alter operator family integer_ops using btree add + function 1 int8nocommutatorcmp (int8, int8nocommutator); +create temp table tbl_nocom(a int8, b int8nocommutator); +-- check that non-commutable merge clauses do not lead to error +set enable_hashjoin to off; +explain (costs off) +select * from tbl_nocom t1 full join tbl_nocom t2 on t2.a = t1.b; + QUERY PLAN +-------------------------------------- + Merge Full Join + Merge Cond: (t2.a = t1.b) + -> Sort + Sort Key: t2.a + -> Seq Scan on tbl_nocom t2 + -> Sort + Sort Key: t1.b USING < + -> Seq Scan on tbl_nocom t1 +(8 rows) + +-- check that non-commutable hash clauses do not lead to error +reset enable_hashjoin; +set enable_mergejoin to off; +explain (costs off) +select * from tbl_nocom t1 full join tbl_nocom t2 on t2.a = t1.b; + QUERY PLAN +-------------------------------------- + Hash Full Join + Hash Cond: (t2.a = t1.b) + -> Seq Scan on tbl_nocom t2 + -> Hash + -> Seq Scan on tbl_nocom t1 +(5 rows) + +reset enable_mergejoin; diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 8bfe3b7ba6..2a07d30a1e 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2928,3 +2928,61 @@ SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS RESET enable_indexonlyscan; RESET enable_seqscan; + +-- +-- test handling of merge/hash clauses that do not have valid commutators +-- + +-- Typically there are not any such operators that are mergejoinable or +-- hashable but have no commutators; so we have to hack things up to create one. + +create type int8nocommutator; +create function int8nocommutatorin(cstring) returns int8nocommutator + strict immutable language internal as 'int8in'; +create function int8nocommutatorout(int8nocommutator) returns cstring + strict immutable language internal as 'int8out'; +create type int8nocommutator ( + input = int8nocommutatorin, + output = int8nocommutatorout, + like = int8 +); + +create function int8nocommutatoreq(int8, int8nocommutator) returns bool + strict immutable language internal as 'int8eq'; +create operator = ( + procedure = int8nocommutatoreq, + leftarg = int8, rightarg = int8nocommutator, + restrict = eqsel, join = eqjoinsel, + hashes, merges +); +alter operator family integer_ops using btree add + operator 3 = (int8, int8nocommutator); + +create function int8nocommutatorlt(int8nocommutator, int8nocommutator) returns bool + strict immutable language internal as 'int8lt'; +create operator < ( + procedure = int8nocommutatorlt, + leftarg = int8nocommutator, rightarg = int8nocommutator +); +alter operator family integer_ops using btree add + operator 1 < (int8nocommutator, int8nocommutator); + +create function int8nocommutatorcmp(int8, int8nocommutator) returns int + strict immutable language internal as 'btint8cmp'; +alter operator family integer_ops using btree add + function 1 int8nocommutatorcmp (int8, int8nocommutator); + +create temp table tbl_nocom(a int8, b int8nocommutator); + +-- check that non-commutable merge clauses do not lead to error +set enable_hashjoin to off; +explain (costs off) +select * from tbl_nocom t1 full join tbl_nocom t2 on t2.a = t1.b; + +-- check that non-commutable hash clauses do not lead to error +reset enable_hashjoin; +set enable_mergejoin to off; +explain (costs off) +select * from tbl_nocom t1 full join tbl_nocom t2 on t2.a = t1.b; + +reset enable_mergejoin; -- 2.43.0