From 9ccf0fd191fadee911e470c277d16fd470fe225b Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Wed, 19 Jun 2024 19:57:40 +0900
Subject: [PATCH v3] 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.  Therefore, here in
this patch we check the validity of commutators for clauses in the form
"inner op outer" when selecting mergejoin/hash clauses, and consider a
clause unusable for the current pair of outer and inner relations if it
lacks a commutator.
---
 src/backend/optimizer/path/joinpath.c | 27 ++++++++++
 src/test/regress/expected/join.out    | 72 +++++++++++++++++++++++++++
 src/test/regress/sql/join.sql         | 59 ++++++++++++++++++++++
 3 files changed, 158 insertions(+)

diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index 5be8da9e09..5c8f436367 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 in this
+		 * form will get commuted 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 in
+		 * this form will get commuted 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..77d1df17d0 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -7960,3 +7960,75 @@ 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
+--
+-- There are not (and should not be) any such operators built into Postgres
+-- 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..9405654d54 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -2928,3 +2928,62 @@ 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
+--
+
+-- There are not (and should not be) any such operators built into Postgres
+-- 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

