jsonb crash
Hi,
I found a crash (segmentation fault) on jsonb.
This is the best I could do to reduce the query:
"""
select
75 as c1
from
public.pagg_tab_ml as ref_0,
lateral (select
ref_0.a as c5
from generate_series(1, 300) as sample_0
fetch first 78 rows only
) as subq_0
where case when (subq_0.c5 < 2)
then cast(null as jsonb)
else cast(null as jsonb)
end ? ref_0.c
"""
And because it needs pagg_tab_ml it should be run a regression database.
This affects at least 14 and 15.
Attached is the backtrace.
--
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL
Attachments:
Em qua., 29 de set. de 2021 às 15:55, Jaime Casanova <
jcasanov@systemguards.com.ec> escreveu:
Hi,
I found a crash (segmentation fault) on jsonb.
This is the best I could do to reduce the query:"""
select
75 as c1
from
public.pagg_tab_ml as ref_0,
lateral (select
ref_0.a as c5
from generate_series(1, 300) as sample_0
fetch first 78 rows only
) as subq_0
where case when (subq_0.c5 < 2)
then cast(null as jsonb)
else cast(null as jsonb)
end ? ref_0.c
"""And because it needs pagg_tab_ml it should be run a regression database.
This affects at least 14 and 15.Attached is the backtrace.
Yeah, Coverity has a report about this at function:
JsonbValue *
pushJsonbValue(JsonbParseState **pstate, JsonbIteratorToken seq,
JsonbValue *jbval)
1. CID undefined: Dereference after null check (FORWARD_NULL)
return pushJsonbValueScalar(pstate, seq, jbval);
2. CID undefined (#1 of 1): Dereference after null check (FORWARD_NULL)16.
var_deref_model:
Passing pstate to pushJsonbValueScalar, which dereferences null *pstate
res = pushJsonbValueScalar(pstate, tok,
tok <
WJB_BEGIN_ARRAY ||
(tok ==
WJB_BEGIN_ARRAY &&
v.
val.array.rawScalar) ? &v : NULL);
regards,
Ranier Vilela
Jaime Casanova <jcasanov@systemguards.com.ec> writes:
I found a crash (segmentation fault) on jsonb.
This is the best I could do to reduce the query:
"""
select
75 as c1
from
public.pagg_tab_ml as ref_0,
lateral (select
ref_0.a as c5
from generate_series(1, 300) as sample_0
fetch first 78 rows only
) as subq_0
where case when (subq_0.c5 < 2)
then cast(null as jsonb)
else cast(null as jsonb)
end ? ref_0.c
"""
I think this must be a memoize bug. AFAICS, nowhere in this query
can we be processing a non-null JSONB value, so what are we doing
in jsonb_hash? Something down-stack must have lost the information
that the Datum is actually null.
regards, tom lane
I wrote:
I think this must be a memoize bug. AFAICS, nowhere in this query
can we be processing a non-null JSONB value, so what are we doing
in jsonb_hash? Something down-stack must have lost the information
that the Datum is actually null.
After further inspection, "what are we doing in jsonb_hash?" is
indeed a relevant question, but it seems like it's a type mismatch
not a nullness issue. EXPLAIN VERBOSE shows
-> Memoize (cost=0.01..1.96 rows=1 width=4)
Output: subq_0.c5
Cache Key: ref_0.c, ref_0.a
-> Subquery Scan on subq_0 (cost=0.00..1.95 rows=1 width=4)
Output: subq_0.c5
Filter: (CASE WHEN (subq_0.c5 < 2) THEN NULL::jsonb ELSE NULL::jsonb END ? ref_0.c)
-> Limit (cost=0.00..0.78 rows=78 width=4)
Output: (ref_0.a)
-> Function Scan on pg_catalog.generate_series sample_0 (cost=0.00..3.00 rows=300 width=4)
Output: ref_0.a
Function Call: generate_series(1, 300)
so unless the "Cache Key" output is a complete lie, the cache key
types we should be concerned with are text and integer. The Datum
that's being passed to jsonb_hash looks suspiciously like it is a
text value '0000', too, which matches the "c" value from the first
row of pagg_tab_ml. I now think some part of Memoize is looking in
completely the wrong place to discover the cache key datatypes.
regards, tom lane
On 9/29/21 4:00 PM, Tom Lane wrote:
Jaime Casanova <jcasanov@systemguards.com.ec> writes:
I found a crash (segmentation fault) on jsonb.
This is the best I could do to reduce the query:
"""
select
75 as c1
from
public.pagg_tab_ml as ref_0,
lateral (select
ref_0.a as c5
from generate_series(1, 300) as sample_0
fetch first 78 rows only
) as subq_0
where case when (subq_0.c5 < 2)
then cast(null as jsonb)
else cast(null as jsonb)
end ? ref_0.c
"""I think this must be a memoize bug. AFAICS, nowhere in this query
can we be processing a non-null JSONB value, so what are we doing
in jsonb_hash? Something down-stack must have lost the information
that the Datum is actually null.
Yeah, confirmed that this is not failing in release 13.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Thu, 30 Sept 2021 at 09:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
After further inspection, "what are we doing in jsonb_hash?" is
indeed a relevant question, but it seems like it's a type mismatch
not a nullness issue. EXPLAIN VERBOSE shows
I think you're right here. It should be hashing text. That seems to
be going wrong in check_memoizable() because it assumes it's always
fine to use the left side's type of the OpExpr to figure out the hash
function to use.
Maybe we can cache the left and the right type's hash function and use
the correct one in paraminfo_get_equal_hashops().
David
David Rowley <dgrowleyml@gmail.com> writes:
On Thu, 30 Sept 2021 at 09:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
After further inspection, "what are we doing in jsonb_hash?" is
indeed a relevant question, but it seems like it's a type mismatch
not a nullness issue. EXPLAIN VERBOSE shows
I think you're right here. It should be hashing text. That seems to
be going wrong in check_memoizable() because it assumes it's always
fine to use the left side's type of the OpExpr to figure out the hash
function to use.
Maybe we can cache the left and the right type's hash function and use
the correct one in paraminfo_get_equal_hashops().
Um ... it seems to have correctly identified the cache key expressions,
so why isn't it just doing exprType on those? The jsonb_exists operator
seems entirely irrelevant here.
regards, tom lane
On Thu, 30 Sept 2021 at 10:09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Rowley <dgrowleyml@gmail.com> writes:
Maybe we can cache the left and the right type's hash function and use
the correct one in paraminfo_get_equal_hashops().Um ... it seems to have correctly identified the cache key expressions,
so why isn't it just doing exprType on those? The jsonb_exists operator
seems entirely irrelevant here.
This is down to the caching stuff I added to RestrictInfo to minimise
the amount of work done during the join search. I cached the hash
equal function in RestrictInfo so I didn't have to check what that was
each time we consider a join. The problem is, that I did a bad job of
taking inspiration from check_hashjoinable() which just looks at the
left type.
David
David Rowley <dgrowleyml@gmail.com> writes:
On Thu, 30 Sept 2021 at 10:09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Um ... it seems to have correctly identified the cache key expressions,
so why isn't it just doing exprType on those? The jsonb_exists operator
seems entirely irrelevant here.
This is down to the caching stuff I added to RestrictInfo to minimise
the amount of work done during the join search. I cached the hash
equal function in RestrictInfo so I didn't have to check what that was
each time we consider a join. The problem is, that I did a bad job of
taking inspiration from check_hashjoinable() which just looks at the
left type.
I'm still confused. AFAICS, the top-level operator of the qual clause has
exactly nada to do with the cache keys, as this example makes plain.
regards, tom lane
On Thu, 30 Sept 2021 at 10:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Rowley <dgrowleyml@gmail.com> writes:
On Thu, 30 Sept 2021 at 10:09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Um ... it seems to have correctly identified the cache key expressions,
so why isn't it just doing exprType on those? The jsonb_exists operator
seems entirely irrelevant here.This is down to the caching stuff I added to RestrictInfo to minimise
the amount of work done during the join search. I cached the hash
equal function in RestrictInfo so I didn't have to check what that was
each time we consider a join. The problem is, that I did a bad job of
taking inspiration from check_hashjoinable() which just looks at the
left type.I'm still confused. AFAICS, the top-level operator of the qual clause has
exactly nada to do with the cache keys, as this example makes plain.
You're right that it does not. The lateral join condition could be
anything. We just need to figure out the hash function and which
equality function so that we can properly find any cached tuples when
we're probing the hash table. We need the equal function too as we
can't just return any old cache tuples that match the same hash value.
Maybe recording the operator is not the best thing to do. Maybe I
should have just recorded the regproc's Oid for the equal function.
That would save from calling get_opcode() in ExecInitMemoize().
David
On Thu, 30 Sept 2021 at 09:48, David Rowley <dgrowleyml@gmail.com> wrote:
Maybe we can cache the left and the right type's hash function and use
the correct one in paraminfo_get_equal_hashops().
Here's a patch of what I had in mind for the fix. It's just hot off
the press, so really only intended to assist discussion at this stage.
David
Attachments:
fix_incorrect_hashfunction_in_memoize.patchapplication/octet-stream; name=fix_incorrect_hashfunction_in_memoize.patchDownload
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 228387eaee..eb9939ea5d 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2362,7 +2362,8 @@ _copyRestrictInfo(const RestrictInfo *from)
COPY_SCALAR_FIELD(right_bucketsize);
COPY_SCALAR_FIELD(left_mcvfreq);
COPY_SCALAR_FIELD(right_mcvfreq);
- COPY_SCALAR_FIELD(hasheqoperator);
+ COPY_SCALAR_FIELD(lefthasheqoperator);
+ COPY_SCALAR_FIELD(righthasheqoperator);
return newnode;
}
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 2e5ed77e18..38f3f5a803 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2565,7 +2565,8 @@ _outRestrictInfo(StringInfo str, const RestrictInfo *node)
WRITE_NODE_FIELD(right_em);
WRITE_BOOL_FIELD(outer_is_left);
WRITE_OID_FIELD(hashjoinoperator);
- WRITE_OID_FIELD(hasheqoperator);
+ WRITE_OID_FIELD(lefthasheqoperator);
+ WRITE_OID_FIELD(righthasheqoperator);
}
static void
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index 6407ede12a..82654f1b7d 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -394,9 +394,15 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
OpExpr *opexpr;
Node *expr;
+ Oid hasheqoperator;
- /* can't use a memoize node without a valid hash equals operator */
- if (!OidIsValid(rinfo->hasheqoperator) ||
+ opexpr = (OpExpr *) rinfo->clause;
+
+ /*
+ * Bail if the rinfo is not compatible. We need a join OpExpr
+ * with 2 args.
+ */
+ if (!IsA(opexpr, OpExpr) || list_length(opexpr->args) != 2 ||
!clause_sides_match_join(rinfo, outerrel, innerrel))
{
list_free(*operators);
@@ -406,15 +412,28 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
/*
* We already checked that this is an OpExpr with 2 args when
- * setting hasheqoperator.
+ * setting the lefthasheqoperator and righthasheqoperator.
*/
- opexpr = (OpExpr *) rinfo->clause;
if (rinfo->outer_is_left)
- expr = (Node *) linitial(opexpr->args);
+ {
+ expr = (Node *)linitial(opexpr->args);
+ hasheqoperator = rinfo->lefthasheqoperator;
+ }
else
- expr = (Node *) lsecond(opexpr->args);
+ {
+ expr = (Node *)lsecond(opexpr->args);
+ hasheqoperator = rinfo->righthasheqoperator;
+ }
+
+ /* can't do memoize if we can't hash the outer type */
+ if (!OidIsValid(hasheqoperator))
+ {
+ list_free(*operators);
+ list_free(*param_exprs);
+ return false;
+ }
- *operators = lappend_oid(*operators, rinfo->hasheqoperator);
+ *operators = lappend_oid(*operators, hasheqoperator);
*param_exprs = lappend(*param_exprs, expr);
}
}
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index e25dc9a7ca..607ae284a5 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -2711,8 +2711,8 @@ check_hashjoinable(RestrictInfo *restrictinfo)
/*
* check_memoizable
* If the restrictinfo's clause is suitable to be used for a Memoize node,
- * set the hasheqoperator to the hash equality operator that will be needed
- * during caching.
+ * set the lefthasheqoperator and righthasheqoperator to the hash equality
+ * operator that will be needed during caching.
*/
static void
check_memoizable(RestrictInfo *restrictinfo)
@@ -2720,6 +2720,7 @@ check_memoizable(RestrictInfo *restrictinfo)
TypeCacheEntry *typentry;
Expr *clause = restrictinfo->clause;
Node *leftarg;
+ Node *rightarg;
if (restrictinfo->pseudoconstant)
return;
@@ -2733,8 +2734,14 @@ check_memoizable(RestrictInfo *restrictinfo)
typentry = lookup_type_cache(exprType(leftarg), TYPECACHE_HASH_PROC |
TYPECACHE_EQ_OPR);
- if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr))
- return;
+ if (OidIsValid(typentry->hash_proc) && OidIsValid(typentry->eq_opr))
+ restrictinfo->lefthasheqoperator = typentry->eq_opr;
+
+ rightarg = lsecond(((OpExpr *) clause)->args);
+
+ typentry = lookup_type_cache(exprType(rightarg), TYPECACHE_HASH_PROC |
+ TYPECACHE_EQ_OPR);
- restrictinfo->hasheqoperator = typentry->eq_opr;
+ if (OidIsValid(typentry->hash_proc) && OidIsValid(typentry->eq_opr))
+ restrictinfo->righthasheqoperator = typentry->eq_opr;
}
diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c
index aa9fb3a9fa..c2380fef6c 100644
--- a/src/backend/optimizer/util/restrictinfo.c
+++ b/src/backend/optimizer/util/restrictinfo.c
@@ -217,7 +217,8 @@ make_restrictinfo_internal(PlannerInfo *root,
restrictinfo->left_mcvfreq = -1;
restrictinfo->right_mcvfreq = -1;
- restrictinfo->hasheqoperator = InvalidOid;
+ restrictinfo->lefthasheqoperator = InvalidOid;
+ restrictinfo->righthasheqoperator = InvalidOid;
return restrictinfo;
}
@@ -368,7 +369,8 @@ commute_restrictinfo(RestrictInfo *rinfo, Oid comm_op)
result->right_bucketsize = rinfo->left_bucketsize;
result->left_mcvfreq = rinfo->right_mcvfreq;
result->right_mcvfreq = rinfo->left_mcvfreq;
- result->hasheqoperator = InvalidOid;
+ result->lefthasheqoperator = InvalidOid;
+ result->righthasheqoperator = InvalidOid;
return result;
}
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 2a53a6e344..46d3d5582c 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -2122,8 +2122,9 @@ typedef struct RestrictInfo
Selectivity left_mcvfreq; /* left side's most common val's freq */
Selectivity right_mcvfreq; /* right side's most common val's freq */
- /* hash equality operator used for memoize nodes, else InvalidOid */
- Oid hasheqoperator;
+ /* hash equality operators used for memoize nodes, else InvalidOid */
+ Oid lefthasheqoperator;
+ Oid righthasheqoperator;
} RestrictInfo;
/*
David Rowley <dgrowley@gmail.com> writes:
On Thu, 30 Sept 2021 at 10:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'm still confused. AFAICS, the top-level operator of the qual clause has
exactly nada to do with the cache keys, as this example makes plain.
You're right that it does not. The lateral join condition could be
anything.
Actually, the more I look at this the more unhappy I get, because
it's becoming clear that you have made unfounded semantic
assumptions. The hash functions generally only promise that they
will distinguish values that are distinguishable by the associated
equality operator. We have plenty of data types in which that does
not map to bitwise equality ... you need not look further than
float8 for an example. And in turn, that means that there are lots
of functions/operators that *can* distinguish hash-equal values.
The fact that you're willing to treat this example as cacheable
means that memoize will fail on such clauses.
So I'm now thinking you weren't that far wrong to be looking at
hashability of the top-level qual operator. What is missing is
that you have to restrict candidate cache keys to be the *direct*
arguments of such an operator. Looking any further down in the
expression introduces untenable assumptions.
regards, tom lane
I wrote:
So I'm now thinking you weren't that far wrong to be looking at
hashability of the top-level qual operator. What is missing is
that you have to restrict candidate cache keys to be the *direct*
arguments of such an operator. Looking any further down in the
expression introduces untenable assumptions.
Hmm ... I think that actually, a correct statement of the semantic
restriction is
To be eligible for memoization, the inside of a join can use the
passed-in parameters *only* as direct arguments of hashable equality
operators.
In order to exploit RestrictInfo-based caching, you could make the
further restriction that all such equality operators appear at the
top level of RestrictInfo clauses. But that's not semantically
necessary.
As an example, assuming p1 and p2 are the path parameters,
(p1 = x) xor (p2 = y)
is semantically safe to memoize, despite the upper-level xor
operator. But the example we started with, with a parameter
used as an argument of jsonb_exists, is not safe to memoize
because we have no grounds to suppose that two hash-equal values
will act the same in jsonb_exists.
regards, tom lane
On Thu, 30 Sept 2021 at 11:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
So I'm now thinking you weren't that far wrong to be looking at
hashability of the top-level qual operator. What is missing is
that you have to restrict candidate cache keys to be the *direct*
arguments of such an operator. Looking any further down in the
expression introduces untenable assumptions.Hmm ... I think that actually, a correct statement of the semantic
restriction isTo be eligible for memoization, the inside of a join can use the
passed-in parameters *only* as direct arguments of hashable equality
operators.In order to exploit RestrictInfo-based caching, you could make the
further restriction that all such equality operators appear at the
top level of RestrictInfo clauses. But that's not semantically
necessary.As an example, assuming p1 and p2 are the path parameters,
(p1 = x) xor (p2 = y)
is semantically safe to memoize, despite the upper-level xor
operator. But the example we started with, with a parameter
used as an argument of jsonb_exists, is not safe to memoize
because we have no grounds to suppose that two hash-equal values
will act the same in jsonb_exists.
I'm not really sure if I follow your comment about the top-level qual
operator. I'm not really sure why that has anything to do with it.
Remember that we *never* do any hashing of any values from the inner
side of the join. If we're doing a parameterized nested loop and say
our parameter has the value of 1, the first time through we don't find
any cached tuples, so we run the plan from the inner side of the
nested loop join and cache all the tuples that we get from it. When
the parameter changes, we check if the current value of the parameter
has any tuples cached. This is what the hashing and equality
comparison does. If the new parameter value is 2, then we'll hash that
and probe the hash table. Since we've only seen value 1 so far, we
won't get a cache hit. If at some later point in time we see the
parameter value of 1 again, we hash that, find something in the hash
bucket for that value then do an equality test to ensure the values
are actually the same and not just the same hash bucket or hash value.
At no point do we do any hashing on the actual cached tuples.
This allows us to memoize any join expression, not just equality
expressions. e.g if the SQL is: SELECT * FROM t1 INNER JOIN t2 on t1.a
t2.a; assuming t2 is on the inner side of the nested loop join,
then the only thing we hash is the t1.a parameter and the only thing
we do an equality comparison on is the current value of t1.a vs some
previous value of t1.a that is stored in the hash table. You can see
here that if t1.a and t2.a are not the same data type then that's of
no relevance as we *never* hash or do any equality comparisons on t2.a
in the memoize code.
The whole thing just hangs together by the assumption that parameters
with the same value will always yield the same tuples. If that's
somehow a wrong assumption, then we have a problem.
I'm not sure if this helps explain how it's meant to work, or if I
just misunderstood you.
David
David Rowley <dgrowleyml@gmail.com> writes:
On Thu, 30 Sept 2021 at 11:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Hmm ... I think that actually, a correct statement of the semantic
restriction is
To be eligible for memoization, the inside of a join can use the
passed-in parameters *only* as direct arguments of hashable equality
operators.
I'm not really sure if I follow your comment about the top-level qual
operator. I'm not really sure why that has anything to do with it.
Remember that we *never* do any hashing of any values from the inner
side of the join. If we're doing a parameterized nested loop and say
our parameter has the value of 1, the first time through we don't find
any cached tuples, so we run the plan from the inner side of the
nested loop join and cache all the tuples that we get from it. When
the parameter changes, we check if the current value of the parameter
has any tuples cached.
Right, and the point is that if you *do* get a hit, you are assuming
that the inner side would return the same values as it returned for
the previous hash-equal value. You are doing yourself no good by
thinking about simple cases like integers. Think about float8,
and ask yourself whether, if you cached a result for +0, that result
is still good for -0. In general we can only assume that for applications
of the hash equality operator itself (or members of its hash opfamily).
Anything involving a cast to text, for example, would fail on such a case.
This allows us to memoize any join expression, not just equality
expressions.
I am clearly failing to get through to you. Do I need to build
an example?
regards, tom lane
On Thu, 30 Sept 2021 at 11:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Rowley <dgrowleyml@gmail.com> writes:
This allows us to memoize any join expression, not just equality
expressions.I am clearly failing to get through to you. Do I need to build
an example?
Taking your -0.0 / +0.0 float example, If I understand correctly due
to -0.0 and +0.0 hashing to the same value and being classed as equal,
we're really only guaranteed to get the same rows if the join
condition uses the float value (in this example) directly.
If for example there was something like a function we could pass that
float value through that was able to distinguish -0.0 from +0.0, then
that could cause issues as the return value of that function could be
anything and have completely different join partners on the other side
of the join.
A function something like:
create or replace function text_sign(f float) returns text as
$$
begin
if f::text like '-%' then
return 'neg';
else
return 'pos';
end if;
end;
$$ language plpgsql;
would be enough to do it. If the join condition was text_sign(t1.f) =
t2.col and the cache key was t1.f rather than text_sign(t1.f)
On Thu, 30 Sept 2021 at 10:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
So I'm now thinking you weren't that far wrong to be looking at
hashability of the top-level qual operator. What is missing is
that you have to restrict candidate cache keys to be the *direct*
arguments of such an operator. Looking any further down in the
expression introduces untenable assumptions.
I think I also follow you now with the above. The problem is that if
the join operator is able to distinguish something that the equality
operator and hash function are not then we have the same problem.
Restricting the join operator to hash equality ensures that the join
condition cannot distinguish anything that we cannot distinguish in
the cache hash table.
David
On Thu, 30 Sept 2021 at 07:55, Jaime Casanova
<jcasanov@systemguards.com.ec> wrote:
"""
select
75 as c1
from
public.pagg_tab_ml as ref_0,
lateral (select
ref_0.a as c5
from generate_series(1, 300) as sample_0
fetch first 78 rows only
) as subq_0
where case when (subq_0.c5 < 2)
then cast(null as jsonb)
else cast(null as jsonb)
end ? ref_0.c
"""
I've attached 2 patches that aim to fix this bug. One for master and
one for pg14. Unfortunately, for pg14, RestrictInfo lacks a field to
store the righthand type's hash equality operator. I don't think it's
going to be possible as [1]https://codesearch.debian.net/search?q=makeNode%28RestrictInfo%29&literal=1 shows me that there's at least one project
outside of core that does makeNode(RestrictInfo). The next best thing
I can think to do for pg14 is just to limit Memoization for
parameterizations where the RestrictInfo has the same types on the
left and right of an OpExpr. For pg14, the above query just does not
use Memoize anymore.
In theory, since this field is just caching the hash equality
operator, it might be possible to look up the hash equality function
each time when the left and right types don't match and we need to
know the hash equality operator of the right type. That'll probably
not be a super common case, but I wouldn't go as far as to say that
it'll be rare. I'd be a bit worried about the additional planning
time if we went that route. The extra lookup would have to be done
during the join search, so could happen thousands of times given a bit
more than a handful of tables in the join search.
For master, we can just add a new field to RestrictInfo. The master
version of the patch does that.
Does anyone have any thoughts on the proposed fixes?
David
[1]: https://codesearch.debian.net/search?q=makeNode%28RestrictInfo%29&literal=1
Attachments:
memoize_type_bug_master.patchapplication/octet-stream; name=memoize_type_bug_master.patchDownload
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 70e9e54d3e..18106901f4 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2362,7 +2362,8 @@ _copyRestrictInfo(const RestrictInfo *from)
COPY_SCALAR_FIELD(right_bucketsize);
COPY_SCALAR_FIELD(left_mcvfreq);
COPY_SCALAR_FIELD(right_mcvfreq);
- COPY_SCALAR_FIELD(hasheqoperator);
+ COPY_SCALAR_FIELD(left_hasheqoperator);
+ COPY_SCALAR_FIELD(right_hasheqoperator);
return newnode;
}
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 2e5ed77e18..23f23f11dc 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2565,7 +2565,8 @@ _outRestrictInfo(StringInfo str, const RestrictInfo *node)
WRITE_NODE_FIELD(right_em);
WRITE_BOOL_FIELD(outer_is_left);
WRITE_OID_FIELD(hashjoinoperator);
- WRITE_OID_FIELD(hasheqoperator);
+ WRITE_OID_FIELD(left_hasheqoperator);
+ WRITE_OID_FIELD(right_hasheqoperator);
}
static void
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index 6407ede12a..0f3ad8aa65 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -394,9 +394,15 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
OpExpr *opexpr;
Node *expr;
+ Oid hasheqoperator;
- /* can't use a memoize node without a valid hash equals operator */
- if (!OidIsValid(rinfo->hasheqoperator) ||
+ opexpr = (OpExpr *) rinfo->clause;
+
+ /*
+ * Bail if the rinfo is not compatible. We need a join OpExpr
+ * with 2 args.
+ */
+ if (!IsA(opexpr, OpExpr) || list_length(opexpr->args) != 2 ||
!clause_sides_match_join(rinfo, outerrel, innerrel))
{
list_free(*operators);
@@ -404,17 +410,26 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
return false;
}
- /*
- * We already checked that this is an OpExpr with 2 args when
- * setting hasheqoperator.
- */
- opexpr = (OpExpr *) rinfo->clause;
if (rinfo->outer_is_left)
+ {
expr = (Node *) linitial(opexpr->args);
+ hasheqoperator = rinfo->left_hasheqoperator;
+ }
else
+ {
expr = (Node *) lsecond(opexpr->args);
+ hasheqoperator = rinfo->right_hasheqoperator;
+ }
+
+ /* can't do memoize if we can't hash the outer type */
+ if (!OidIsValid(hasheqoperator))
+ {
+ list_free(*operators);
+ list_free(*param_exprs);
+ return false;
+ }
- *operators = lappend_oid(*operators, rinfo->hasheqoperator);
+ *operators = lappend_oid(*operators, hasheqoperator);
*param_exprs = lappend(*param_exprs, expr);
}
}
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index e25dc9a7ca..f6a202d900 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -2711,15 +2711,16 @@ check_hashjoinable(RestrictInfo *restrictinfo)
/*
* check_memoizable
* If the restrictinfo's clause is suitable to be used for a Memoize node,
- * set the hasheqoperator to the hash equality operator that will be needed
- * during caching.
+ * set the lefthasheqoperator and righthasheqoperator to the hash equality
+ * operator that will be needed during caching.
*/
static void
check_memoizable(RestrictInfo *restrictinfo)
{
TypeCacheEntry *typentry;
Expr *clause = restrictinfo->clause;
- Node *leftarg;
+ Oid lefttype;
+ Oid righttype;
if (restrictinfo->pseudoconstant)
return;
@@ -2728,13 +2729,24 @@ check_memoizable(RestrictInfo *restrictinfo)
if (list_length(((OpExpr *) clause)->args) != 2)
return;
- leftarg = linitial(((OpExpr *) clause)->args);
+ lefttype = exprType(linitial(((OpExpr *) clause)->args));
- typentry = lookup_type_cache(exprType(leftarg), TYPECACHE_HASH_PROC |
+ typentry = lookup_type_cache(lefttype, TYPECACHE_HASH_PROC |
TYPECACHE_EQ_OPR);
- if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr))
- return;
+ if (OidIsValid(typentry->hash_proc) && OidIsValid(typentry->eq_opr))
+ restrictinfo->left_hasheqoperator = typentry->eq_opr;
+
+ righttype = exprType(lsecond(((OpExpr *) clause)->args));
+
+ /*
+ * Lookup the right type, unless it's the same as the left type, in which
+ * case typentry is already pointing to the required TypeCacheEntry.
+ */
+ if (lefttype != righttype)
+ typentry = lookup_type_cache(righttype, TYPECACHE_HASH_PROC |
+ TYPECACHE_EQ_OPR);
- restrictinfo->hasheqoperator = typentry->eq_opr;
+ if (OidIsValid(typentry->hash_proc) && OidIsValid(typentry->eq_opr))
+ restrictinfo->right_hasheqoperator = typentry->eq_opr;
}
diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c
index aa9fb3a9fa..ebfcf91826 100644
--- a/src/backend/optimizer/util/restrictinfo.c
+++ b/src/backend/optimizer/util/restrictinfo.c
@@ -217,7 +217,8 @@ make_restrictinfo_internal(PlannerInfo *root,
restrictinfo->left_mcvfreq = -1;
restrictinfo->right_mcvfreq = -1;
- restrictinfo->hasheqoperator = InvalidOid;
+ restrictinfo->left_hasheqoperator = InvalidOid;
+ restrictinfo->right_hasheqoperator = InvalidOid;
return restrictinfo;
}
@@ -368,7 +369,8 @@ commute_restrictinfo(RestrictInfo *rinfo, Oid comm_op)
result->right_bucketsize = rinfo->left_bucketsize;
result->left_mcvfreq = rinfo->right_mcvfreq;
result->right_mcvfreq = rinfo->left_mcvfreq;
- result->hasheqoperator = InvalidOid;
+ result->left_hasheqoperator = InvalidOid;
+ result->right_hasheqoperator = InvalidOid;
return result;
}
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 2a53a6e344..186e89905b 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -2122,8 +2122,9 @@ typedef struct RestrictInfo
Selectivity left_mcvfreq; /* left side's most common val's freq */
Selectivity right_mcvfreq; /* right side's most common val's freq */
- /* hash equality operator used for memoize nodes, else InvalidOid */
- Oid hasheqoperator;
+ /* hash equality operators used for memoize nodes, else InvalidOid */
+ Oid left_hasheqoperator;
+ Oid right_hasheqoperator;
} RestrictInfo;
/*
memoize_type_bug_pg14.patchapplication/octet-stream; name=memoize_type_bug_pg14.patchDownload
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index e25dc9a7ca..276e62b74b 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -2719,7 +2719,8 @@ check_memoizable(RestrictInfo *restrictinfo)
{
TypeCacheEntry *typentry;
Expr *clause = restrictinfo->clause;
- Node *leftarg;
+ Oid lefttype;
+ Oid righttype;
if (restrictinfo->pseudoconstant)
return;
@@ -2728,10 +2729,20 @@ check_memoizable(RestrictInfo *restrictinfo)
if (list_length(((OpExpr *) clause)->args) != 2)
return;
- leftarg = linitial(((OpExpr *) clause)->args);
+ lefttype = exprType(linitial(((OpExpr *) clause)->args));
+ righttype = exprType(lsecond(((OpExpr *) clause)->args));
+
+ /*
+ * Really there should be a field for both the left and right hash
+ * equality operator, however, in v14, there's only a single field in
+ * RestrictInfo to record the operator in, so we must insist that the left
+ * and right types match.
+ */
+ if (lefttype != righttype)
+ return;
- typentry = lookup_type_cache(exprType(leftarg), TYPECACHE_HASH_PROC |
- TYPECACHE_EQ_OPR);
+ typentry = lookup_type_cache(lefttype, TYPECACHE_HASH_PROC |
+ TYPECACHE_EQ_OPR);
if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr))
return;
On Tue, Oct 26, 2021 at 07:07:01PM +1300, David Rowley wrote:
Does anyone have any thoughts on the proposed fixes?
I don't have any thoughts, but I want to be sure it isn't forgotten.
--
Justin
On Sat, 6 Nov 2021 at 11:38, Justin Pryzby <pryzby@telsasoft.com> wrote:
On Tue, Oct 26, 2021 at 07:07:01PM +1300, David Rowley wrote:
Does anyone have any thoughts on the proposed fixes?
I don't have any thoughts, but I want to be sure it isn't forgotten.
Not forgotten. I was just hoping to get some feedback.
I've now pushed the fix to restrict v14 to only allow Memoize when the
left and right types are the same. For master, since it's possible to
add a field to RestrictInfo, I've changed that to cache the left and
right hash equality operators.
This does not fix the binary / logical issue mentioned by Tom. I have
ideas about allowing Memoize to operate in a binary equality mode or
logical equality mode. I'll need to run in binary mode when there are
lateral vars or when any join operator is not hashable.
David
David Rowley <dgrowleyml@gmail.com> writes:
I've now pushed the fix to restrict v14 to only allow Memoize when the
left and right types are the same. For master, since it's possible to
add a field to RestrictInfo, I've changed that to cache the left and
right hash equality operators.
If you were going to push this fix before 14.1, you should have done it
days ago. At this point it's not possible to get a full set of buildfarm
results before the wrap. The valgrind and clobber-cache animals, in
particular, are unlikely to report back in time.
regards, tom lane
On Mon, 8 Nov 2021 at 15:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Rowley <dgrowleyml@gmail.com> writes:
I've now pushed the fix to restrict v14 to only allow Memoize when the
left and right types are the same. For master, since it's possible to
add a field to RestrictInfo, I've changed that to cache the left and
right hash equality operators.If you were going to push this fix before 14.1, you should have done it
days ago. At this point it's not possible to get a full set of buildfarm
results before the wrap. The valgrind and clobber-cache animals, in
particular, are unlikely to report back in time.
Sorry, I was under the impression that it was ok until you'd done the
stamp for the release. As far as I can see, that's not done yet.
Do you want me to back out the change I made to 14?
David
On Thu, 30 Sept 2021 at 10:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Actually, the more I look at this the more unhappy I get, because
it's becoming clear that you have made unfounded semantic
assumptions. The hash functions generally only promise that they
will distinguish values that are distinguishable by the associated
equality operator. We have plenty of data types in which that does
not map to bitwise equality ... you need not look further than
float8 for an example.
I think this part might be best solved by allowing Memoize to work in
a binary mode. We already have datum_image_eq() for performing a
binary comparison on a Datum. We'll also need to supplement that with
a function that generates a hash value based on the binary value too.
If we do that and put Memoize in binary mode when join operators are
not hashable or when we're doing LATERAL joins, I think it should fix
this.
It might be possible to work a bit harder and allow the logical mode
for some LATERAL joins. e.g. something like: SELECT * FROM a, LATERAL
(SELECT * FROM b WHERE a.a = b.b LIMIT 1) b; could use the logical
mode (assuming the join operator is hashable), however, we really only
know the lateral_vars. We don't really collect their context
currently, or the full Expr that they're contained in. That case gets
more complex if the join condition had contained a mix of lateral and
non-lateral vars on one side of the qual, e.g WHERE a.a = b.b + a.z.
Certainly if the lateral part of the query was a function call, then
we'd be forced into binary mode as we'd have no idea what the function
is doing with the lateral vars being passed to it.
I've my proposed patch.
An easier way out of this would be to disable Memoize for lateral
joins completely and only allow it for normal joins when the join
operators are hashable. I don't want to do this as people are already
seeing good wins in PG14 with Memoize and lateral joins [1]https://twitter.com/RPorsager/status/1455660236375826436. I think
quite a few people would be upset if we removed that ability.
David
[1]: https://twitter.com/RPorsager/status/1455660236375826436
Attachments:
memoize_binary_cache.patchapplication/octet-stream; name=memoize_binary_cache.patchDownload
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index fd141a0fa5..382783ac87 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2241,6 +2241,7 @@ SELECT t1."C 1" FROM "S 1"."T 1" t1, LATERAL (SELECT DISTINCT t2.c1, t3.c1 FROM
Output: t1."C 1", t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7, t1.c8
-> Memoize
Cache Key: t1.c2
+ Cache Mode: binary
-> Subquery Scan on q
-> HashAggregate
Output: t2.c1, t3.c1
@@ -2249,7 +2250,7 @@ SELECT t1."C 1" FROM "S 1"."T 1" t1, LATERAL (SELECT DISTINCT t2.c1, t3.c1 FROM
Output: t2.c1, t3.c1
Relations: (public.ft1 t2) INNER JOIN (public.ft2 t3)
Remote SQL: SELECT r1."C 1", r2."C 1" FROM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1")) AND ((r1.c2 = $1::integer))))
-(16 rows)
+(17 rows)
SELECT t1."C 1" FROM "S 1"."T 1" t1, LATERAL (SELECT DISTINCT t2.c1, t3.c1 FROM ft1 t2, ft2 t3 WHERE t2.c1 = t3.c1 AND t2.c2 = t1.c2) q ORDER BY t1."C 1" OFFSET 10 LIMIT 10;
C 1
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 10644dfac4..09f5253abb 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -3127,11 +3127,14 @@ show_memoize_info(MemoizeState *mstate, List *ancestors, ExplainState *es)
if (es->format != EXPLAIN_FORMAT_TEXT)
{
ExplainPropertyText("Cache Key", keystr.data, es);
+ ExplainPropertyText("Cache Mode", mstate->binary_mode ? "binary" : "logical", es);
}
else
{
ExplainIndentText(es);
appendStringInfo(es->str, "Cache Key: %s\n", keystr.data);
+ ExplainIndentText(es);
+ appendStringInfo(es->str, "Cache Mode: %s\n", mstate->binary_mode ? "binary" : "logical");
}
pfree(keystr.data);
diff --git a/src/backend/executor/nodeMemoize.c b/src/backend/executor/nodeMemoize.c
index bec588b3a0..c5887c022f 100644
--- a/src/backend/executor/nodeMemoize.c
+++ b/src/backend/executor/nodeMemoize.c
@@ -71,6 +71,7 @@
#include "executor/nodeMemoize.h"
#include "lib/ilist.h"
#include "miscadmin.h"
+#include "utils/datum.h"
#include "utils/lsyscache.h"
/* States of the ExecMemoize state machine */
@@ -131,7 +132,7 @@ typedef struct MemoizeEntry
static uint32 MemoizeHash_hash(struct memoize_hash *tb,
const MemoizeKey *key);
-static int MemoizeHash_equal(struct memoize_hash *tb,
+static bool MemoizeHash_equal(struct memoize_hash *tb,
const MemoizeKey *params1,
const MemoizeKey *params2);
@@ -140,7 +141,7 @@ static int MemoizeHash_equal(struct memoize_hash *tb,
#define SH_KEY_TYPE MemoizeKey *
#define SH_KEY key
#define SH_HASH_KEY(tb, key) MemoizeHash_hash(tb, key)
-#define SH_EQUAL(tb, a, b) (MemoizeHash_equal(tb, a, b) == 0)
+#define SH_EQUAL(tb, a, b) MemoizeHash_equal(tb, a, b)
#define SH_SCOPE static inline
#define SH_STORE_HASH
#define SH_GET_HASH(tb, a) a->hash
@@ -160,21 +161,46 @@ MemoizeHash_hash(struct memoize_hash *tb, const MemoizeKey *key)
TupleTableSlot *pslot = mstate->probeslot;
uint32 hashkey = 0;
int numkeys = mstate->nkeys;
- FmgrInfo *hashfunctions = mstate->hashfunctions;
- Oid *collations = mstate->collations;
- for (int i = 0; i < numkeys; i++)
+
+ if (mstate->binary_mode)
+ {
+ for (int i = 0; i < numkeys; i++)
+ {
+ /* rotate hashkey left 1 bit at each step */
+ hashkey = (hashkey << 1) | ((hashkey & 0x80000000) ? 1 : 0);
+
+ if (!pslot->tts_isnull[i]) /* treat nulls as having hash key 0 */
+ {
+ FormData_pg_attribute *attr;
+ uint32 hkey;
+
+ attr = &pslot->tts_tupleDescriptor->attrs[i];
+
+ hkey = datum_image_hash(pslot->tts_values[i], attr->attbyval, attr->attlen);
+
+ hashkey ^= hkey;
+ }
+ }
+ }
+ else
{
- /* rotate hashkey left 1 bit at each step */
- hashkey = (hashkey << 1) | ((hashkey & 0x80000000) ? 1 : 0);
+ FmgrInfo *hashfunctions = mstate->hashfunctions;
+ Oid *collations = mstate->collations;
- if (!pslot->tts_isnull[i]) /* treat nulls as having hash key 0 */
+ for (int i = 0; i < numkeys; i++)
{
- uint32 hkey;
+ /* rotate hashkey left 1 bit at each step */
+ hashkey = (hashkey << 1) | ((hashkey & 0x80000000) ? 1 : 0);
- hkey = DatumGetUInt32(FunctionCall1Coll(&hashfunctions[i],
- collations[i], pslot->tts_values[i]));
- hashkey ^= hkey;
+ if (!pslot->tts_isnull[i]) /* treat nulls as having hash key 0 */
+ {
+ uint32 hkey;
+
+ hkey = DatumGetUInt32(FunctionCall1Coll(&hashfunctions[i],
+ collations[i], pslot->tts_values[i]));
+ hashkey ^= hkey;
+ }
}
}
@@ -187,7 +213,7 @@ MemoizeHash_hash(struct memoize_hash *tb, const MemoizeKey *key)
* table lookup. 'key2' is never used. Instead the MemoizeState's
* probeslot is always populated with details of what's being looked up.
*/
-static int
+static bool
MemoizeHash_equal(struct memoize_hash *tb, const MemoizeKey *key1,
const MemoizeKey *key2)
{
@@ -199,9 +225,38 @@ MemoizeHash_equal(struct memoize_hash *tb, const MemoizeKey *key1,
/* probeslot should have already been prepared by prepare_probe_slot() */
ExecStoreMinimalTuple(key1->params, tslot, false);
- econtext->ecxt_innertuple = tslot;
- econtext->ecxt_outertuple = pslot;
- return !ExecQualAndReset(mstate->cache_eq_expr, econtext);
+ if (mstate->binary_mode)
+ {
+ int numkeys = mstate->nkeys;
+
+ slot_getallattrs(tslot);
+ slot_getallattrs(pslot);
+
+ for (int i = 0; i < numkeys; i++)
+ {
+ FormData_pg_attribute *attr;
+
+ if (tslot->tts_isnull[i] != pslot->tts_isnull[i])
+ return false;
+
+ /* both NULL? they're equal */
+ if (tslot->tts_isnull[i])
+ continue;
+
+ /* perform binary comparison on the two datums */
+ attr = &tslot->tts_tupleDescriptor->attrs[i];
+ if (!datum_image_eq(tslot->tts_values[i], pslot->tts_values[i],
+ attr->attbyval, attr->attlen))
+ return false;
+ }
+ return true;
+ }
+ else
+ {
+ econtext->ecxt_innertuple = tslot;
+ econtext->ecxt_outertuple = pslot;
+ return ExecQualAndReset(mstate->cache_eq_expr, econtext);
+ }
}
/*
@@ -926,6 +981,12 @@ ExecInitMemoize(Memoize *node, EState *estate, int eflags)
*/
mstate->singlerow = node->singlerow;
+ /*
+ * Record if the cache keys should be compared bit by bit, or logically
+ * using the type's hash equality operator
+ */
+ mstate->binary_mode = node->binary_mode;
+
/* Zero the statistics counters */
memset(&mstate->stats, 0, sizeof(MemoizeInstrumentation));
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index ad1ea2ff2f..7d55fd69ab 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -971,6 +971,7 @@ _copyMemoize(const Memoize *from)
COPY_POINTER_FIELD(collations, sizeof(Oid) * from->numKeys);
COPY_NODE_FIELD(param_exprs);
COPY_SCALAR_FIELD(singlerow);
+ COPY_SCALAR_FIELD(binary_mode);
COPY_SCALAR_FIELD(est_entries);
return newnode;
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 23f23f11dc..be374a0d70 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -866,6 +866,7 @@ _outMemoize(StringInfo str, const Memoize *node)
WRITE_OID_ARRAY(collations, node->numKeys);
WRITE_NODE_FIELD(param_exprs);
WRITE_BOOL_FIELD(singlerow);
+ WRITE_BOOL_FIELD(binary_mode);
WRITE_UINT_FIELD(est_entries);
}
@@ -1966,6 +1967,7 @@ _outMemoizePath(StringInfo str, const MemoizePath *node)
WRITE_NODE_FIELD(hash_operators);
WRITE_NODE_FIELD(param_exprs);
WRITE_BOOL_FIELD(singlerow);
+ WRITE_BOOL_FIELD(binary_mode);
WRITE_FLOAT_FIELD(calls, "%.0f");
WRITE_UINT_FIELD(est_entries);
}
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index abf08b7a2f..a82c53ec0d 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -2230,6 +2230,7 @@ _readMemoize(void)
READ_OID_ARRAY(collations, local_node->numKeys);
READ_NODE_FIELD(param_exprs);
READ_BOOL_FIELD(singlerow);
+ READ_BOOL_FIELD(binary_mode);
READ_UINT_FIELD(est_entries);
READ_DONE();
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index 0f3ad8aa65..987a677858 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -371,19 +371,21 @@ allow_star_schema_join(PlannerInfo *root,
* Returns true the hashing is possible, otherwise return false.
*
* Additionally we also collect the outer exprs and the hash operators for
- * each parameter to innerrel. These set in 'param_exprs' and 'operators'
- * when we return true.
+ * each parameter to innerrel. These set in 'param_exprs', 'operators' and
+ * 'binary_mode' when we return true.
*/
static bool
paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
RelOptInfo *outerrel, RelOptInfo *innerrel,
- List **param_exprs, List **operators)
+ List **param_exprs, List **operators,
+ bool *binary_mode)
{
ListCell *lc;
*param_exprs = NIL;
*operators = NIL;
+ *binary_mode = false;
if (param_info != NULL)
{
@@ -431,6 +433,20 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
*operators = lappend_oid(*operators, hasheqoperator);
*param_exprs = lappend(*param_exprs, expr);
+
+ /*
+ * When the join operator is not hashable then it's possible that
+ * the operator will be able to distinguish something that the
+ * hash equality operator could not. For example with floating
+ * point types -0.0 and +0.0 are classed as equal by the hash
+ * function and equality function, but some other operator may be
+ * able to tell those values apart. This means that we must put
+ * memoize into binary comparison mode so that it does bit-by-bit
+ * comparisons rather than a "logical" comparison as it would
+ * using the hash equality operator.
+ */
+ if (!OidIsValid(rinfo->hashjoinoperator))
+ *binary_mode = true;
}
}
@@ -461,6 +477,17 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
*operators = lappend_oid(*operators, typentry->eq_opr);
*param_exprs = lappend(*param_exprs, expr);
+
+ /*
+ * We must go into binary mode as we don't have too much of an idea
+ * of how these lateral Vars are being used. See comment above when
+ * we set *binary_mode for the non-lateral Var case. This could be
+ * relaxed a bit if we had the RestrictInfos and knew the operators
+ * being used, however for cases like Vars that are arguments to
+ * functions we must operate in binary mode as we don't have
+ * visibility into what the function is doing with the Vars.
+ */
+ *binary_mode = true;
}
/* We're okay to use memoize */
@@ -481,6 +508,7 @@ get_memoize_path(PlannerInfo *root, RelOptInfo *innerrel,
List *param_exprs;
List *hash_operators;
ListCell *lc;
+ bool binary_mode;
/* Obviously not if it's disabled */
if (!enable_memoize)
@@ -572,7 +600,8 @@ get_memoize_path(PlannerInfo *root, RelOptInfo *innerrel,
outerrel,
innerrel,
¶m_exprs,
- &hash_operators))
+ &hash_operators,
+ &binary_mode))
{
return (Path *) create_memoize_path(root,
innerrel,
@@ -580,6 +609,7 @@ get_memoize_path(PlannerInfo *root, RelOptInfo *innerrel,
param_exprs,
hash_operators,
extra->inner_unique,
+ binary_mode,
outer_path->parent->rows);
}
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 3dc0176a51..866f19f64c 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -279,7 +279,8 @@ static Sort *make_sort_from_groupcols(List *groupcls,
static Material *make_material(Plan *lefttree);
static Memoize *make_memoize(Plan *lefttree, Oid *hashoperators,
Oid *collations, List *param_exprs,
- bool singlerow, uint32 est_entries);
+ bool singlerow, bool binary_mode,
+ uint32 est_entries);
static WindowAgg *make_windowagg(List *tlist, Index winref,
int partNumCols, AttrNumber *partColIdx, Oid *partOperators, Oid *partCollations,
int ordNumCols, AttrNumber *ordColIdx, Oid *ordOperators, Oid *ordCollations,
@@ -1617,7 +1618,8 @@ create_memoize_plan(PlannerInfo *root, MemoizePath *best_path, int flags)
}
plan = make_memoize(subplan, operators, collations, param_exprs,
- best_path->singlerow, best_path->est_entries);
+ best_path->singlerow, best_path->binary_mode,
+ best_path->est_entries);
copy_generic_path_info(&plan->plan, (Path *) best_path);
@@ -6417,7 +6419,8 @@ materialize_finished_plan(Plan *subplan)
static Memoize *
make_memoize(Plan *lefttree, Oid *hashoperators, Oid *collations,
- List *param_exprs, bool singlerow, uint32 est_entries)
+ List *param_exprs, bool singlerow, bool binary_mode,
+ uint32 est_entries)
{
Memoize *node = makeNode(Memoize);
Plan *plan = &node->plan;
@@ -6432,6 +6435,7 @@ make_memoize(Plan *lefttree, Oid *hashoperators, Oid *collations,
node->collations = collations;
node->param_exprs = param_exprs;
node->singlerow = singlerow;
+ node->binary_mode = binary_mode;
node->est_entries = est_entries;
return node;
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index e53d381e19..af5e8df26b 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -1583,7 +1583,7 @@ create_material_path(RelOptInfo *rel, Path *subpath)
MemoizePath *
create_memoize_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath,
List *param_exprs, List *hash_operators,
- bool singlerow, double calls)
+ bool singlerow, bool binary_mode, double calls)
{
MemoizePath *pathnode = makeNode(MemoizePath);
@@ -1603,6 +1603,7 @@ create_memoize_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath,
pathnode->hash_operators = hash_operators;
pathnode->param_exprs = param_exprs;
pathnode->singlerow = singlerow;
+ pathnode->binary_mode = binary_mode;
pathnode->calls = calls;
/*
@@ -3942,6 +3943,7 @@ reparameterize_path(PlannerInfo *root, Path *path,
mpath->param_exprs,
mpath->hash_operators,
mpath->singlerow,
+ mpath->binary_mode,
mpath->calls);
}
default:
diff --git a/src/backend/utils/adt/datum.c b/src/backend/utils/adt/datum.c
index 6a317fc0a6..95d43bd7d4 100644
--- a/src/backend/utils/adt/datum.c
+++ b/src/backend/utils/adt/datum.c
@@ -43,6 +43,7 @@
#include "postgres.h"
#include "access/detoast.h"
+#include "common/hashfn.h"
#include "fmgr.h"
#include "utils/builtins.h"
#include "utils/datum.h"
@@ -324,6 +325,54 @@ datum_image_eq(Datum value1, Datum value2, bool typByVal, int typLen)
return result;
}
+/*-------------------------------------------------------------------------
+ * datum_image_hash
+ *
+ * Generate a hash value based on the binary representation of 'value'. Most
+ * use cases will want to use the hash function specific to the Datum's type,
+ * however, some corner cases require generating a hash value based on the
+ * actual bits rather than the logical value.
+ *-------------------------------------------------------------------------
+ */
+uint32
+datum_image_hash(Datum value, bool typByVal, int typLen)
+{
+ Size len;
+ uint32 result;
+
+ if (typByVal)
+ result = hash_bytes((unsigned char *) &value, sizeof(Datum));
+ else if (typLen > 0)
+ result = hash_bytes((unsigned char *) DatumGetPointer(value), typLen);
+ else if (typLen == -1)
+ {
+ struct varlena *val;
+
+ len = toast_raw_datum_size(value);
+
+ val = PG_DETOAST_DATUM_PACKED(value);
+
+ result = hash_bytes((unsigned char *) VARDATA_ANY(val), len - VARHDRSZ);
+
+ /* Only free memory if it's a copy made here. */
+ if ((Pointer) val != (Pointer) value)
+ pfree(val);
+ }
+ else if (typLen == -2)
+ {
+ char *s;
+
+ s = DatumGetCString(value);
+ len = strlen(s) + 1;
+
+ result = hash_bytes((unsigned char *) s, len);
+ }
+ else
+ elog(ERROR, "unexpected typLen: %d", typLen);
+
+ return result;
+}
+
/*-------------------------------------------------------------------------
* btequalimage
*
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 2e8cbee69f..2a05b553fa 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2109,6 +2109,9 @@ typedef struct MemoizeState
* NULL if 'last_tuple' is NULL. */
bool singlerow; /* true if the cache entry is to be marked as
* complete after caching the first tuple. */
+ bool binary_mode; /* true when cache key should be compared bit
+ * by bit, false when using hash equality ops
+ */
MemoizeInstrumentation stats; /* execution statistics */
SharedMemoizeInfo *shared_info; /* statistics for parallel workers */
} MemoizeState;
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 186e89905b..c99eab509f 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -1515,6 +1515,9 @@ typedef struct MemoizePath
List *param_exprs; /* cache keys */
bool singlerow; /* true if the cache entry is to be marked as
* complete after caching the first record. */
+ bool binary_mode; /* true when cache key should be compared bit
+ * by bit, false when using hash equality ops
+ */
Cardinality calls; /* expected number of rescans */
uint32 est_entries; /* The maximum number of entries that the
* planner expects will fit in the cache, or 0
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 01a246d50e..acf3fe7bbd 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -799,6 +799,9 @@ typedef struct Memoize
bool singlerow; /* true if the cache entry should be marked as
* complete after we store the first tuple in
* it. */
+ bool binary_mode; /* true when cache key should be compared bit
+ * by bit, false when using hash equality ops
+ */
uint32 est_entries; /* The maximum number of entries that the
* planner expects will fit in the cache, or 0
* if unknown */
diff --git a/src/include/optimizer/pathnode.h b/src/include/optimizer/pathnode.h
index f704d39980..2922c0cdc1 100644
--- a/src/include/optimizer/pathnode.h
+++ b/src/include/optimizer/pathnode.h
@@ -88,6 +88,7 @@ extern MemoizePath *create_memoize_path(PlannerInfo *root,
List *param_exprs,
List *hash_operators,
bool singlerow,
+ bool binary_mode,
double calls);
extern UniquePath *create_unique_path(PlannerInfo *root, RelOptInfo *rel,
Path *subpath, SpecialJoinInfo *sjinfo);
diff --git a/src/include/utils/datum.h b/src/include/utils/datum.h
index d4cf62bed7..8a59f11006 100644
--- a/src/include/utils/datum.h
+++ b/src/include/utils/datum.h
@@ -55,6 +55,14 @@ extern bool datumIsEqual(Datum value1, Datum value2,
extern bool datum_image_eq(Datum value1, Datum value2,
bool typByVal, int typLen);
+/*
+ * datum_image_hash
+ *
+ * Generates hash value for 'value' based on its bits rather than logical
+ * value.
+ */
+extern uint32 datum_image_hash(Datum value, bool typByVal, int typLen);
+
/*
* Serialize and restore datums so that we can transfer them to parallel
* workers.
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 84331659e7..d5b5b775fd 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -3686,9 +3686,10 @@ where t1.unique1 = 1;
Index Cond: (hundred = t1.hundred)
-> Memoize
Cache Key: t2.thousand
+ Cache Mode: logical
-> Index Scan using tenk1_unique2 on tenk1 t3
Index Cond: (unique2 = t2.thousand)
-(13 rows)
+(14 rows)
explain (costs off)
select * from tenk1 t1 left join
@@ -3708,9 +3709,10 @@ where t1.unique1 = 1;
Index Cond: (hundred = t1.hundred)
-> Memoize
Cache Key: t2.thousand
+ Cache Mode: logical
-> Index Scan using tenk1_unique2 on tenk1 t3
Index Cond: (unique2 = t2.thousand)
-(13 rows)
+(14 rows)
explain (costs off)
select count(*) from
@@ -4238,11 +4240,12 @@ where t1.f1 = ss.f1;
-> Memoize
Output: (i8.q1), t2.f1
Cache Key: i8.q1
+ Cache Mode: binary
-> Limit
Output: (i8.q1), t2.f1
-> Seq Scan on public.text_tbl t2
Output: i8.q1, t2.f1
-(19 rows)
+(20 rows)
select * from
text_tbl t1
@@ -4282,6 +4285,7 @@ where t1.f1 = ss2.f1;
-> Memoize
Output: (i8.q1), t2.f1
Cache Key: i8.q1
+ Cache Mode: binary
-> Limit
Output: (i8.q1), t2.f1
-> Seq Scan on public.text_tbl t2
@@ -4289,11 +4293,12 @@ where t1.f1 = ss2.f1;
-> Memoize
Output: ((i8.q1)), (t2.f1)
Cache Key: (i8.q1), t2.f1
+ Cache Mode: binary
-> Limit
Output: ((i8.q1)), (t2.f1)
-> Seq Scan on public.text_tbl t3
Output: (i8.q1), t2.f1
-(28 rows)
+(30 rows)
select * from
text_tbl t1
@@ -4342,6 +4347,7 @@ where tt1.f1 = ss1.c0;
-> Memoize
Output: ss1.c0
Cache Key: tt4.f1
+ Cache Mode: binary
-> Subquery Scan on ss1
Output: ss1.c0
Filter: (ss1.c0 = 'foo'::text)
@@ -4349,7 +4355,7 @@ where tt1.f1 = ss1.c0;
Output: (tt4.f1)
-> Seq Scan on public.text_tbl tt5
Output: tt4.f1
-(32 rows)
+(33 rows)
select 1 from
text_tbl as tt1
@@ -5058,8 +5064,9 @@ explain (costs off)
-> Seq Scan on tenk1 a
-> Memoize
Cache Key: a.two
+ Cache Mode: binary
-> Function Scan on generate_series g
-(6 rows)
+(7 rows)
explain (costs off)
select count(*) from tenk1 a cross join lateral generate_series(1,two) g;
@@ -5070,8 +5077,9 @@ explain (costs off)
-> Seq Scan on tenk1 a
-> Memoize
Cache Key: a.two
+ Cache Mode: binary
-> Function Scan on generate_series g
-(6 rows)
+(7 rows)
-- don't need the explicit LATERAL keyword for functions
explain (costs off)
@@ -5083,8 +5091,9 @@ explain (costs off)
-> Seq Scan on tenk1 a
-> Memoize
Cache Key: a.two
+ Cache Mode: binary
-> Function Scan on generate_series g
-(6 rows)
+(7 rows)
-- lateral with UNION ALL subselect
explain (costs off)
@@ -5145,9 +5154,10 @@ explain (costs off)
-> Values Scan on "*VALUES*"
-> Memoize
Cache Key: "*VALUES*".column1
+ Cache Mode: logical
-> Index Only Scan using tenk1_unique2 on tenk1 b
Index Cond: (unique2 = "*VALUES*".column1)
-(9 rows)
+(10 rows)
select count(*) from tenk1 a,
tenk1 b join lateral (values(a.unique1),(-1)) ss(x) on b.unique2 = ss.x;
diff --git a/src/test/regress/expected/memoize.out b/src/test/regress/expected/memoize.out
index 9a025c4a7a..0ed5d8474a 100644
--- a/src/test/regress/expected/memoize.out
+++ b/src/test/regress/expected/memoize.out
@@ -44,11 +44,12 @@ WHERE t2.unique1 < 1000;', false);
Rows Removed by Filter: 9000
-> Memoize (actual rows=1 loops=N)
Cache Key: t2.twenty
+ Cache Mode: logical
Hits: 980 Misses: 20 Evictions: Zero Overflows: 0 Memory Usage: NkB
-> Index Only Scan using tenk1_unique1 on tenk1 t1 (actual rows=1 loops=N)
Index Cond: (unique1 = t2.twenty)
Heap Fetches: N
-(11 rows)
+(12 rows)
-- And check we get the expected results.
SELECT COUNT(*),AVG(t1.unique1) FROM tenk1 t1
@@ -73,11 +74,12 @@ WHERE t1.unique1 < 1000;', false);
Rows Removed by Filter: 9000
-> Memoize (actual rows=1 loops=N)
Cache Key: t1.twenty
+ Cache Mode: logical
Hits: 980 Misses: 20 Evictions: Zero Overflows: 0 Memory Usage: NkB
-> Index Only Scan using tenk1_unique1 on tenk1 t2 (actual rows=1 loops=N)
Index Cond: (unique1 = t1.twenty)
Heap Fetches: N
-(11 rows)
+(12 rows)
-- And check we get the expected results.
SELECT COUNT(*),AVG(t2.unique1) FROM tenk1 t1,
@@ -107,12 +109,94 @@ WHERE t2.unique1 < 1200;', true);
Rows Removed by Filter: 8800
-> Memoize (actual rows=1 loops=N)
Cache Key: t2.thousand
+ Cache Mode: logical
Hits: N Misses: N Evictions: N Overflows: 0 Memory Usage: NkB
-> Index Only Scan using tenk1_unique1 on tenk1 t1 (actual rows=1 loops=N)
Index Cond: (unique1 = t2.thousand)
Heap Fetches: N
-(11 rows)
+(12 rows)
+CREATE TABLE flt (f float);
+CREATE INDEX flt_f_idx ON flt (f);
+INSERT INTO flt VALUES('-0.0'::float),('+0.0'::float);
+ANALYZE flt;
+SET enable_seqscan TO off;
+-- Ensure memoize operates in logical mode
+SELECT explain_memoize('
+SELECT * FROM flt f1 INNER JOIN flt f2 ON f1.f = f2.f;', false);
+ explain_memoize
+-------------------------------------------------------------------------------
+ Nested Loop (actual rows=4 loops=N)
+ -> Index Only Scan using flt_f_idx on flt f1 (actual rows=2 loops=N)
+ Heap Fetches: N
+ -> Memoize (actual rows=2 loops=N)
+ Cache Key: f1.f
+ Cache Mode: logical
+ Hits: 1 Misses: 1 Evictions: Zero Overflows: 0 Memory Usage: NkB
+ -> Index Only Scan using flt_f_idx on flt f2 (actual rows=2 loops=N)
+ Index Cond: (f = f1.f)
+ Heap Fetches: N
+(10 rows)
+
+-- Ensure memoize operates in binary mode
+SELECT explain_memoize('
+SELECT * FROM flt f1 INNER JOIN flt f2 ON f1.f >= f2.f;', false);
+ explain_memoize
+-------------------------------------------------------------------------------
+ Nested Loop (actual rows=4 loops=N)
+ -> Index Only Scan using flt_f_idx on flt f1 (actual rows=2 loops=N)
+ Heap Fetches: N
+ -> Memoize (actual rows=2 loops=N)
+ Cache Key: f1.f
+ Cache Mode: binary
+ Hits: 0 Misses: 2 Evictions: Zero Overflows: 0 Memory Usage: NkB
+ -> Index Only Scan using flt_f_idx on flt f2 (actual rows=2 loops=N)
+ Index Cond: (f <= f1.f)
+ Heap Fetches: N
+(10 rows)
+
+DROP TABLE flt;
+-- Exercise Memoize in binary mode with a large fixed width type and a
+-- varlena type.
+CREATE TABLE strtest (n name, t text);
+CREATE INDEX strtest_n_idx ON strtest (n);
+CREATE INDEX strtest_t_idx ON strtest (t);
+INSERT INTO strtest VALUES('one','one'),('two','two'),('three',repeat(md5('three'),100));
+-- duplicate rows so we get some cache hits
+INSERT INTO strtest SELECT * FROM strtest;
+ANALYZE strtest;
+-- Ensure we get 3 hits and 3 misses
+SELECT explain_memoize('
+SELECT * FROM strtest s1 INNER JOIN strtest s2 ON s1.n >= s2.n;', false);
+ explain_memoize
+----------------------------------------------------------------------------------
+ Nested Loop (actual rows=24 loops=N)
+ -> Seq Scan on strtest s1 (actual rows=6 loops=N)
+ -> Memoize (actual rows=4 loops=N)
+ Cache Key: s1.n
+ Cache Mode: binary
+ Hits: 3 Misses: 3 Evictions: Zero Overflows: 0 Memory Usage: NkB
+ -> Index Scan using strtest_n_idx on strtest s2 (actual rows=4 loops=N)
+ Index Cond: (n <= s1.n)
+(8 rows)
+
+-- Ensure we get 3 hits and 3 misses
+SELECT explain_memoize('
+SELECT * FROM strtest s1 INNER JOIN strtest s2 ON s1.t >= s2.t;', false);
+ explain_memoize
+----------------------------------------------------------------------------------
+ Nested Loop (actual rows=24 loops=N)
+ -> Seq Scan on strtest s1 (actual rows=6 loops=N)
+ -> Memoize (actual rows=4 loops=N)
+ Cache Key: s1.t
+ Cache Mode: binary
+ Hits: 3 Misses: 3 Evictions: Zero Overflows: 0 Memory Usage: NkB
+ -> Index Scan using strtest_t_idx on strtest s2 (actual rows=4 loops=N)
+ Index Cond: (t <= s1.t)
+(8 rows)
+
+DROP TABLE strtest;
+RESET enable_seqscan;
RESET enable_mergejoin;
RESET work_mem;
RESET enable_bitmapscan;
@@ -140,9 +224,10 @@ WHERE t1.unique1 < 1000;
Index Cond: (unique1 < 1000)
-> Memoize
Cache Key: t1.twenty
+ Cache Mode: logical
-> Index Only Scan using tenk1_unique1 on tenk1 t2
Index Cond: (unique1 = t1.twenty)
-(13 rows)
+(14 rows)
-- And ensure the parallel plan gives us the correct results.
SELECT COUNT(*),AVG(t2.unique1) FROM tenk1 t1,
diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
index 0742626033..4e8ddc7061 100644
--- a/src/test/regress/expected/subselect.out
+++ b/src/test/regress/expected/subselect.out
@@ -1139,13 +1139,14 @@ where o.ten = 1;
Filter: (ten = 1)
-> Memoize
Cache Key: o.four
+ Cache Mode: binary
-> CTE Scan on x
CTE x
-> Recursive Union
-> Result
-> WorkTable Scan on x x_1
Filter: (a < 10)
-(12 rows)
+(13 rows)
select sum(o.four), sum(ss.a) from
onek o cross join lateral (
diff --git a/src/test/regress/sql/memoize.sql b/src/test/regress/sql/memoize.sql
index 548cc3eee3..3c7360adf9 100644
--- a/src/test/regress/sql/memoize.sql
+++ b/src/test/regress/sql/memoize.sql
@@ -65,6 +65,45 @@ SELECT explain_memoize('
SELECT COUNT(*),AVG(t1.unique1) FROM tenk1 t1
INNER JOIN tenk1 t2 ON t1.unique1 = t2.thousand
WHERE t2.unique1 < 1200;', true);
+
+CREATE TABLE flt (f float);
+CREATE INDEX flt_f_idx ON flt (f);
+INSERT INTO flt VALUES('-0.0'::float),('+0.0'::float);
+ANALYZE flt;
+
+SET enable_seqscan TO off;
+
+-- Ensure memoize operates in logical mode
+SELECT explain_memoize('
+SELECT * FROM flt f1 INNER JOIN flt f2 ON f1.f = f2.f;', false);
+
+-- Ensure memoize operates in binary mode
+SELECT explain_memoize('
+SELECT * FROM flt f1 INNER JOIN flt f2 ON f1.f >= f2.f;', false);
+
+DROP TABLE flt;
+
+-- Exercise Memoize in binary mode with a large fixed width type and a
+-- varlena type.
+CREATE TABLE strtest (n name, t text);
+CREATE INDEX strtest_n_idx ON strtest (n);
+CREATE INDEX strtest_t_idx ON strtest (t);
+INSERT INTO strtest VALUES('one','one'),('two','two'),('three',repeat(md5('three'),100));
+-- duplicate rows so we get some cache hits
+INSERT INTO strtest SELECT * FROM strtest;
+ANALYZE strtest;
+
+-- Ensure we get 3 hits and 3 misses
+SELECT explain_memoize('
+SELECT * FROM strtest s1 INNER JOIN strtest s2 ON s1.n >= s2.n;', false);
+
+-- Ensure we get 3 hits and 3 misses
+SELECT explain_memoize('
+SELECT * FROM strtest s1 INNER JOIN strtest s2 ON s1.t >= s2.t;', false);
+
+DROP TABLE strtest;
+
+RESET enable_seqscan;
RESET enable_mergejoin;
RESET work_mem;
RESET enable_bitmapscan;
On Thu, 11 Nov 2021 at 18:08, David Rowley <dgrowleyml@gmail.com> wrote:
On Thu, 30 Sept 2021 at 10:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Actually, the more I look at this the more unhappy I get, because
it's becoming clear that you have made unfounded semantic
assumptions. The hash functions generally only promise that they
will distinguish values that are distinguishable by the associated
equality operator. We have plenty of data types in which that does
not map to bitwise equality ... you need not look further than
float8 for an example.I think this part might be best solved by allowing Memoize to work in
a binary mode. We already have datum_image_eq() for performing a
binary comparison on a Datum. We'll also need to supplement that with
a function that generates a hash value based on the binary value too.
Since I really don't want to stop Memoize from working with LATERAL
joined function calls, I see no other way other than to make use of a
binary key'd cache for cases where we can't be certain that it's safe
to make work in non-binary mode.
I've had thoughts about if we should just make it work in binary mode
all the time, but my thoughts are that that's not exactly a great idea
since it could have a negative effect on cache hits due to there being
the possibility that some types such as case insensitive text where
the number of variations in the binary representation might be vast.
The reason this could be bad is that the estimated cache hit ratio is
calculated by looking at n_distinct, which is obviously not looking
for distinctions in the binary representation. So in binary mode, we
may get a lower cache hit ratio than we might think we'll get, even
with accurate statistics. I'd like to minimise those times by only
using binary mode when we can't be certain that logical mode is safe.
The patch does add new fields to the Memoize plan node type, the
MemoizeState executor node and also MemoizePath. The new fields do
fit inside the padding of the existing structs. I've also obviously
had to modify the read/write/copy functions for Memoize to add the new
field there too. My understanding is that this should be ok since
those are only used for parallel query to send plans to the working
and to deserialise it on the worker side. There should never be any
version mismatches there.
If anyone wants to chime in about my proposed patch for this, then
please do so soon. I'm planning to look at this in my Tuesday morning
(UTC+13).
David