Skip to content

Commit

Permalink
[BACKPORT 2.20.1][#20531] YSQL: Fix BNL local join lookup equality fu…
Browse files Browse the repository at this point in the history
…nction

Summary:
Original commit: 5923bb6 / 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 49ed106 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
  • Loading branch information
tanujnay112 committed Jan 12, 2024
1 parent 476d9c8 commit c503dd7
Show file tree
Hide file tree
Showing 9 changed files with 88 additions and 7 deletions.
5 changes: 4 additions & 1 deletion src/postgres/src/backend/executor/nodeYbBatchedNestloop.c
Original file line number Diff line number Diff line change
Expand Up @@ -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++)
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
8 changes: 5 additions & 3 deletions src/postgres/src/backend/nodes/copyfuncs.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
1 change: 1 addition & 0 deletions src/postgres/src/backend/nodes/outfuncs.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
4 changes: 4 additions & 0 deletions src/postgres/src/backend/nodes/readfuncs.c
Original file line number Diff line number Diff line change
Expand Up @@ -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++;
}

Expand Down
7 changes: 4 additions & 3 deletions src/postgres/src/backend/optimizer/plan/setrefs.c
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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;

Expand All @@ -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++;
}
Expand Down
1 change: 1 addition & 0 deletions src/postgres/src/include/nodes/execnodes.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions src/postgres/src/include/nodes/plannodes.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
43 changes: 43 additions & 0 deletions src/postgres/src/test/regress/expected/yb_join_batching.out
Original file line number Diff line number Diff line change
Expand Up @@ -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;

25 changes: 25 additions & 0 deletions src/postgres/src/test/regress/sql/yb_join_batching.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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;

0 comments on commit c503dd7

Please sign in to comment.