From c503dd7887f3ad4b4e8eb1d44c099d4883dbb10d Mon Sep 17 00:00:00 2001 From: tnayak Date: Thu, 11 Jan 2024 13:04:12 -0800 Subject: [PATCH] [BACKPORT 2.20.1][#20531] YSQL: Fix BNL local join lookup equality function Summary: Original commit: 5923bb67993ebbd1b6b483afceec8e0df560bc6d / D31596 During a BNL we find matching outer tuples for a particular inner tuple using a local hash table. This hash table derives its hash function from all the equality predicates of the join condition. Any function looking into the hash table needs to be using the same equality function to find the right tuples. The lookup condition was made to be the raw join condition before this change which may be stricter than the actual hash insertion equality condition. This caused a bug where if the join condition contained more than just hashable equality filters, matching outer tuples would not be found. This diff changes the lookup function to be just the hashable part of the joinqual that is used when inserting outer tuples into the hash table. This bug was introduced with the original BNL commit 49ed1065dabd2a9456b830c69b4bb8d584491ff8 and backports are needed up to at least 2.18 when BNL usage was made public. Jira: DB-9535 Test Plan: Jenkins: urgent ./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressJoin' Reviewers: mtakahara, smishra Reviewed By: smishra Subscribers: yql, smishra Tags: #jenkins-ready Differential Revision: https://phorge.dev.yugabyte.com/D31655 --- .../backend/executor/nodeYbBatchedNestloop.c | 5 ++- src/postgres/src/backend/nodes/copyfuncs.c | 8 ++-- src/postgres/src/backend/nodes/outfuncs.c | 1 + src/postgres/src/backend/nodes/readfuncs.c | 4 ++ .../src/backend/optimizer/plan/setrefs.c | 7 +-- src/postgres/src/include/nodes/execnodes.h | 1 + src/postgres/src/include/nodes/plannodes.h | 1 + .../regress/expected/yb_join_batching.out | 43 +++++++++++++++++++ .../src/test/regress/sql/yb_join_batching.sql | 25 +++++++++++ 9 files changed, 88 insertions(+), 7 deletions(-) diff --git a/src/postgres/src/backend/executor/nodeYbBatchedNestloop.c b/src/postgres/src/backend/executor/nodeYbBatchedNestloop.c index 50d9b91d64c3..b4ead5496461 100644 --- a/src/postgres/src/backend/executor/nodeYbBatchedNestloop.c +++ b/src/postgres/src/backend/executor/nodeYbBatchedNestloop.c @@ -348,6 +348,7 @@ InitHash(YbBatchedNestLoopState *bnlstate) palloc(num_hashClauseInfos * sizeof(AttrNumber)); ExprState **keyexprs = palloc(num_hashClauseInfos * (sizeof(ExprState*))); List *outerParamExprs = NULL; + List *hashExprs = NULL; YbBNLHashClauseInfo *current_hinfo = plan->hashClauseInfos; for (int i = 0; i < num_hashClauseInfos; i++) @@ -359,6 +360,7 @@ InitHash(YbBatchedNestLoopState *bnlstate) Expr *outerExpr = current_hinfo->outerParamExpr; keyexprs[i] = ExecInitExpr(outerExpr, (PlanState *) bnlstate); outerParamExprs = lappend(outerParamExprs, outerExpr); + hashExprs = lappend(hashExprs, current_hinfo->orig_expr); current_hinfo++; } Oid *eqFuncOids; @@ -387,6 +389,7 @@ InitHash(YbBatchedNestLoopState *bnlstate) econtext->ecxt_per_query_memory, tablecxt, econtext->ecxt_per_tuple_memory, econtext, false); + bnlstate->ht_lookup_fn = ExecInitQual(hashExprs, (PlanState *) bnlstate); bnlstate->hashiterinit = false; bnlstate->current_hash_entry = NULL; @@ -439,7 +442,7 @@ GetNewOuterTupleHash(YbBatchedNestLoopState *bnlstate, ExprContext *econtext) { TupleTableSlot *inner = econtext->ecxt_innertuple; TupleHashTable ht = bnlstate->hashtable; - ExprState *eq = bnlstate->js.joinqual; + ExprState *eq = bnlstate->ht_lookup_fn; TupleHashEntry data; data = FindTupleHashEntry(ht, diff --git a/src/postgres/src/backend/nodes/copyfuncs.c b/src/postgres/src/backend/nodes/copyfuncs.c index 5cc7f61da388..0e6bcb446c9e 100644 --- a/src/postgres/src/backend/nodes/copyfuncs.c +++ b/src/postgres/src/backend/nodes/copyfuncs.c @@ -912,10 +912,12 @@ _copyYbBatchedNestLoop(const YbBatchedNestLoop *from) from->num_hashClauseInfos * sizeof(YbBNLHashClauseInfo)); for (int i = 0; i < from->num_hashClauseInfos; i++) - { - newnode->hashClauseInfos[i].outerParamExpr = + newnode->hashClauseInfos[i].outerParamExpr = (Expr *) copyObject(from->hashClauseInfos[i].outerParamExpr); - } + + for (int i = 0; i < from->num_hashClauseInfos; i++) + newnode->hashClauseInfos[i].orig_expr = (Expr *) + copyObject(from->hashClauseInfos[i].orig_expr); return newnode; } diff --git a/src/postgres/src/backend/nodes/outfuncs.c b/src/postgres/src/backend/nodes/outfuncs.c index 88c9b877ad8d..a42a8dcac792 100644 --- a/src/postgres/src/backend/nodes/outfuncs.c +++ b/src/postgres/src/backend/nodes/outfuncs.c @@ -788,6 +788,7 @@ _outYbBatchedNestLoop(StringInfo str, const YbBatchedNestLoop *node) appendStringInfo(str, "%d", current_hinfo->innerHashAttNo); appendStringInfoString(str, " :" "outerParamExpr" " "), outNode(str, current_hinfo->outerParamExpr); + outNode(str, current_hinfo->orig_expr); } } diff --git a/src/postgres/src/backend/nodes/readfuncs.c b/src/postgres/src/backend/nodes/readfuncs.c index bf90c7411c9d..cb99bbb6de97 100644 --- a/src/postgres/src/backend/nodes/readfuncs.c +++ b/src/postgres/src/backend/nodes/readfuncs.c @@ -2117,6 +2117,10 @@ _readYbBatchedNestLoop(void) tok = pg_strtok(&length); (void) tok; current_hinfo->outerParamExpr = nodeRead(NULL, 0); + + tok = pg_strtok(&length); + (void) tok; + current_hinfo->orig_expr = nodeRead(NULL, 0); current_hinfo++; } diff --git a/src/postgres/src/backend/optimizer/plan/setrefs.c b/src/postgres/src/backend/optimizer/plan/setrefs.c index a316c2cf4aec..3b7d8c1e24bd 100644 --- a/src/postgres/src/backend/optimizer/plan/setrefs.c +++ b/src/postgres/src/backend/optimizer/plan/setrefs.c @@ -1713,7 +1713,7 @@ set_join_references(PlannerInfo *root, Join *join, int rtoffset) { Expr *clause = (Expr *) lfirst(l); Oid hashOp = current_hinfo->hashOp; - + if (OidIsValid(hashOp)) { Assert(IsA(clause, OpExpr)); @@ -1724,7 +1724,7 @@ set_join_references(PlannerInfo *root, Join *join, int rtoffset) if (IsA(leftArg, RelabelType)) leftArg = ((RelabelType *) leftArg)->arg; - + if (IsA(rightArg, RelabelType)) rightArg = ((RelabelType *) rightArg)->arg; @@ -1742,12 +1742,13 @@ set_join_references(PlannerInfo *root, Join *join, int rtoffset) outerArg = leftArg; innerArg = (Var *) rightArg; } - + Assert(innerArg->varno = INNER_VAR); current_hinfo->innerHashAttNo = ((Var *) innerArg)->varattno; current_hinfo->outerParamExpr = outerArg; + current_hinfo->orig_expr = clause; } current_hinfo++; } diff --git a/src/postgres/src/include/nodes/execnodes.h b/src/postgres/src/include/nodes/execnodes.h index d43c45e0cbd5..4f255eb1613c 100644 --- a/src/postgres/src/include/nodes/execnodes.h +++ b/src/postgres/src/include/nodes/execnodes.h @@ -1906,6 +1906,7 @@ typedef struct YbBatchedNestLoopState FmgrInfo *hashFunctions; int numLookupAttrs; AttrNumber *innerAttrs; + ExprState *ht_lookup_fn; /* Function pointers to local join methods */ FlushTupleFn_t FlushTupleImpl; diff --git a/src/postgres/src/include/nodes/plannodes.h b/src/postgres/src/include/nodes/plannodes.h index c7fdb6bf08fb..e3f6884bc10f 100644 --- a/src/postgres/src/include/nodes/plannodes.h +++ b/src/postgres/src/include/nodes/plannodes.h @@ -763,6 +763,7 @@ typedef struct YbBNLHashClauseInfo with. */ int innerHashAttNo; /* Attno of inner side variable. */ Expr *outerParamExpr; /* Outer expression of this clause. */ + Expr *orig_expr; } YbBNLHashClauseInfo; typedef struct YbBatchedNestLoop diff --git a/src/postgres/src/test/regress/expected/yb_join_batching.out b/src/postgres/src/test/regress/expected/yb_join_batching.out index 58d572b28133..80c12d68c30a 100644 --- a/src/postgres/src/test/regress/expected/yb_join_batching.out +++ b/src/postgres/src/test/regress/expected/yb_join_batching.out @@ -2069,3 +2069,46 @@ on (x1 = xx1) where (xx2 is not null) order by 1; 2 | 22 | 2 | 222 | 2 | 22 4 | 44 | 4 | | 4 | 44 (3 rows) + +create table ss1 (a int, b int); +create table ss2(a int, b int); +create table ss3(a int, b int); +create index on ss3(a asc); +insert into ss1 values (1,1), (1,2); +insert into ss2 values (1,1), (1,2); +insert into ss3 values (1,1), (1,3); +explain (costs off) /*+Set(enable_hashjoin off) +Set(enable_material off) +Set(enable_mergejoin off) +Set(yb_bnl_batch_size 1024) +NestLoop(ss1 ss2) Rows(ss1 ss2 #1024)*/select ss1.*, p.* from ss1, ss2, ss3 p where ss1.a = ss2.a and ss1.b = ss2.b and p.b <= ss2.b + 1 and p.a = ss1.a order by 1,2,3,4; + QUERY PLAN +------------------------------------------------------------------------ + Sort + Sort Key: ss1.a, ss1.b, p.b + -> YB Batched Nested Loop Join + Join Filter: ((ss1.a = p.a) AND (p.b <= (ss2.b + 1))) + -> Nested Loop + Join Filter: ((ss1.a = ss2.a) AND (ss1.b = ss2.b)) + -> Seq Scan on ss1 + -> Seq Scan on ss2 + -> Index Scan using ss3_a_idx on ss3 p + Index Cond: (a = ANY (ARRAY[ss1.a, $1, $2, ..., $1023])) +(10 rows) + +/*+Set(enable_hashjoin off) +Set(enable_material off) +Set(enable_mergejoin off) +Set(yb_bnl_batch_size 1024) +NestLoop(ss1 ss2) Rows(ss1 ss2 #1024)*/select ss1.*, p.* from ss1, ss2, ss3 p where ss1.a = ss2.a and ss1.b = ss2.b and p.b <= ss2.b + 1 and p.a = ss1.a order by 1,2,3,4; + a | b | a | b +---+---+---+--- + 1 | 1 | 1 | 1 + 1 | 2 | 1 | 1 + 1 | 2 | 1 | 3 +(3 rows) + +drop table ss1; +drop table ss2; +drop table ss3; + diff --git a/src/postgres/src/test/regress/sql/yb_join_batching.sql b/src/postgres/src/test/regress/sql/yb_join_batching.sql index 02714bcfae25..20fcd74936fa 100644 --- a/src/postgres/src/test/regress/sql/yb_join_batching.sql +++ b/src/postgres/src/test/regress/sql/yb_join_batching.sql @@ -649,3 +649,28 @@ select * from (x left join y on (x1 = y1)) left join x xx(xx1,xx2) on (x1 = xx1) where (y2 is not null) order by 1; select * from (x left join y on (x1 = y1)) left join x xx(xx1,xx2) on (x1 = xx1) where (xx2 is not null) order by 1; + +create table ss1 (a int, b int); +create table ss2(a int, b int); +create table ss3(a int, b int); +create index on ss3(a asc); +insert into ss1 values (1,1), (1,2); +insert into ss2 values (1,1), (1,2); +insert into ss3 values (1,1), (1,3); + +explain (costs off) /*+Set(enable_hashjoin off) +Set(enable_material off) +Set(enable_mergejoin off) +Set(yb_bnl_batch_size 1024) +NestLoop(ss1 ss2) Rows(ss1 ss2 #1024)*/select ss1.*, p.* from ss1, ss2, ss3 p where ss1.a = ss2.a and ss1.b = ss2.b and p.b <= ss2.b + 1 and p.a = ss1.a order by 1,2,3,4; + +/*+Set(enable_hashjoin off) +Set(enable_material off) +Set(enable_mergejoin off) +Set(yb_bnl_batch_size 1024) +NestLoop(ss1 ss2) Rows(ss1 ss2 #1024)*/select ss1.*, p.* from ss1, ss2, ss3 p where ss1.a = ss2.a and ss1.b = ss2.b and p.b <= ss2.b + 1 and p.a = ss1.a order by 1,2,3,4; + +drop table ss1; +drop table ss2; +drop table ss3; +