Re: cache lookup failed for collation 0
On 2019-04-15 07:44, Jeevan Chalke wrote: > The root cause is that the same code match_pattern_prefix() is being > used for text and bytea, but bytea does not use collations, so having > the collation 0 is expected, and we shouldn't call > get_collation_isdeterministic() in that case. > > Proposed patch attached. > > Looks fine to me. Committed, thanks. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: cache lookup failed for collation 0
On Fri, Apr 12, 2019 at 1:26 PM Peter Eisentraut < peter.eisentr...@2ndquadrant.com> wrote: > On 2019-04-11 17:04, Jeevan Chalke wrote: > > The error is coming from get_collation_isdeterministic() when colloid > > passed is 0. I think like we do in get_collation_name(), we should > > return false here when such collation oid does not exist. > > I'm not in favor of doing that. It would risk papering over errors of > omission at other call sites. > > The root cause is that the same code match_pattern_prefix() is being > used for text and bytea, but bytea does not use collations, so having > the collation 0 is expected, and we shouldn't call > get_collation_isdeterministic() in that case. > > Proposed patch attached. > Looks fine to me. > > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services > -- Jeevan Chalke Technical Architect, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
Re: cache lookup failed for collation 0
On 2019-04-11 17:04, Jeevan Chalke wrote: > The error is coming from get_collation_isdeterministic() when colloid > passed is 0. I think like we do in get_collation_name(), we should > return false here when such collation oid does not exist. I'm not in favor of doing that. It would risk papering over errors of omission at other call sites. The root cause is that the same code match_pattern_prefix() is being used for text and bytea, but bytea does not use collations, so having the collation 0 is expected, and we shouldn't call get_collation_isdeterministic() in that case. Proposed patch attached. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services From acb1542a1f8ebee9c9d6d9322c64c849b2f23e15 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Fri, 12 Apr 2019 09:49:10 +0200 Subject: [PATCH] Unbreak index optimization for LIKE on bytea The same code is used to handle both text and bytea, but bytea is not collation-aware, so we shouldn't call get_collation_isdeterministic() in that case. --- src/backend/utils/adt/like_support.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/backend/utils/adt/like_support.c b/src/backend/utils/adt/like_support.c index a65e63736c..7528c80f7c 100644 --- a/src/backend/utils/adt/like_support.c +++ b/src/backend/utils/adt/like_support.c @@ -267,8 +267,10 @@ match_pattern_prefix(Node *leftop, * precise error messages.) (It should be possible to support at least * Pattern_Prefix_Exact, but no point as along as the actual * pattern-matching implementations don't support it.) +* +* expr_coll is not set for a non-collation-aware data type such as bytea. */ - if (!get_collation_isdeterministic(expr_coll)) + if (expr_coll && !get_collation_isdeterministic(expr_coll)) return NIL; /* -- 2.21.0
Re: cache lookup failed for collation 0
On Thu, Apr 11, 2019 at 10:50 PM Tom Lane wrote: > Jeevan Chalke writes: > > Do you mean, the code in get_collation_isdeterministic() should look like > > something like below? > > > If colloid = InvalidOid then > > return TRUE > > ELSE IF tuple is valid then > > return collisdeterministic from the tuple > > ELSE > > return FALSE > > I think it's appropriate to fail if we don't find a tuple, for any > collation oid other than zero. Again, if you trace through the > behavior of the longstanding collation check functions like > lc_ctype_is_c(), you'll see that that's what happens (except for > some hardwired OIDs that they have fast paths for). > OK. Attached patch which treats "collation 0" as deterministic in get_collation_isdeterministic() and returns true, keeping rest of the code as is. > regards, tom lane > -- Jeevan Chalke Technical Architect, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c index b4f2d0f..79e6484 100644 --- a/src/backend/utils/cache/lsyscache.c +++ b/src/backend/utils/cache/lsyscache.c @@ -945,6 +945,10 @@ get_collation_isdeterministic(Oid colloid) Form_pg_collation colltup; bool result; + /* Treat "collation 0" as deterministic. */ + if (!OidIsValid(colloid)) + return true; + tp = SearchSysCache1(COLLOID, ObjectIdGetDatum(colloid)); if (!HeapTupleIsValid(tp)) elog(ERROR, "cache lookup failed for collation %u", colloid); diff --git a/src/test/regress/expected/collate.out b/src/test/regress/expected/collate.out index 0dee7d7..a8e1f6e 100644 --- a/src/test/regress/expected/collate.out +++ b/src/test/regress/expected/collate.out @@ -676,13 +676,19 @@ SELECT collation for ((SELECT b FROM collate_test1 LIMIT 1)); "C" (1 row) +CREATE TABLE byteatable(a bytea primary key, b int); +SELECT * FROM byteatable WHERE a LIKE '%1%'; + a | b +---+--- +(0 rows) + -- -- Clean up. Many of these table names will be re-used if the user is -- trying to run any platform-specific collation tests later, so we -- must get rid of them. -- DROP SCHEMA collate_tests CASCADE; -NOTICE: drop cascades to 17 other objects +NOTICE: drop cascades to 18 other objects DETAIL: drop cascades to table collate_test1 drop cascades to table collate_test_like drop cascades to table collate_test2 @@ -700,3 +706,4 @@ drop cascades to table collate_test21 drop cascades to table collate_test22 drop cascades to collation mycoll2 drop cascades to table collate_test23 +drop cascades to table byteatable diff --git a/src/test/regress/sql/collate.sql b/src/test/regress/sql/collate.sql index 89de26a..124daf6 100644 --- a/src/test/regress/sql/collate.sql +++ b/src/test/regress/sql/collate.sql @@ -259,6 +259,9 @@ SELECT collation for ((SELECT a FROM collate_test1 LIMIT 1)); -- non-collatable SELECT collation for ((SELECT b FROM collate_test1 LIMIT 1)); +CREATE TABLE byteatable(a bytea primary key, b int); +SELECT * FROM byteatable WHERE a LIKE '%1%'; + -- -- Clean up. Many of these table names will be re-used if the user is -- trying to run any platform-specific collation tests later, so we
Re: cache lookup failed for collation 0
Jeevan Chalke writes: > Do you mean, the code in get_collation_isdeterministic() should look like > something like below? > If colloid = InvalidOid then > return TRUE > ELSE IF tuple is valid then > return collisdeterministic from the tuple > ELSE > return FALSE I think it's appropriate to fail if we don't find a tuple, for any collation oid other than zero. Again, if you trace through the behavior of the longstanding collation check functions like lc_ctype_is_c(), you'll see that that's what happens (except for some hardwired OIDs that they have fast paths for). regards, tom lane
Re: cache lookup failed for collation 0
On Thu, Apr 11, 2019 at 9:07 PM Tom Lane wrote:
> Jeevan Chalke writes:
> > Following test-sequence causing an error "cache lookup failed for
> collation 0";
> > postgres:5432 [42106]=# create table foobar(a bytea primary key, b int);
> > CREATE TABLE
> > postgres:5432 [42106]=# insert into foobar
> > values('\x4c835521685c46ee827ab83d376cf028', 1);
> > INSERT 0 1
> > postgres:5432 [42106]=# select * from foobar where a like '%1%';
> > ERROR: cache lookup failed for collation 0
>
> Good catch!
>
> > The error is coming from get_collation_isdeterministic() when colloid
> > passed is 0. I think like we do in get_collation_name(), we should return
> > false here when such collation oid does not exist.
>
> Considering that e.g. lc_ctype_is_c() doesn't fail for InvalidOid, I agree
> that it's probably a bad idea for get_collation_isdeterministic to fail.
> There's a lot of code that thinks it can check for InvalidOid only in slow
> paths. However, I'd kind of expect the default result to be "true" not
> "false". Doing what you suggest would make match_pattern_prefix fail
> entirely, unless we also put a special case there.
>
Do you mean, the code in get_collation_isdeterministic() should look like
something like below?
If colloid = InvalidOid then
return TRUE
ELSE IF tuple is valid then
return collisdeterministic from the tuple
ELSE
return FALSE
I think for non-zero colloid which is not valid we should return false, but
I may be missing your point here.
>
> regards, tom lane
>
--
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Re: cache lookup failed for collation 0
Jeevan Chalke writes:
> Following test-sequence causing an error "cache lookup failed for collation
> 0";
> postgres:5432 [42106]=# create table foobar(a bytea primary key, b int);
> CREATE TABLE
> postgres:5432 [42106]=# insert into foobar
> values('\x4c835521685c46ee827ab83d376cf028', 1);
> INSERT 0 1
> postgres:5432 [42106]=# select * from foobar where a like '%1%';
> ERROR: cache lookup failed for collation 0
Good catch!
> The error is coming from get_collation_isdeterministic() when colloid
> passed is 0. I think like we do in get_collation_name(), we should return
> false here when such collation oid does not exist.
Considering that e.g. lc_ctype_is_c() doesn't fail for InvalidOid, I agree
that it's probably a bad idea for get_collation_isdeterministic to fail.
There's a lot of code that thinks it can check for InvalidOid only in slow
paths. However, I'd kind of expect the default result to be "true" not
"false". Doing what you suggest would make match_pattern_prefix fail
entirely, unless we also put a special case there.
regards, tom lane
cache lookup failed for collation 0
Hello hackers,
Following test-sequence causing an error "cache lookup failed for collation
0";
postgres:5432 [42106]=# create table foobar(a bytea primary key, b int);
CREATE TABLE
postgres:5432 [42106]=# insert into foobar
values('\x4c835521685c46ee827ab83d376cf028', 1);
INSERT 0 1
postgres:5432 [42106]=# \d+ foobar
Table "public.foobar"
Column | Type | Collation | Nullable | Default | Storage | Stats
target | Description
+-+---+--+-+--+--+-
a | bytea | | not null | | extended
| |
b | integer | | | | plain
| |
Indexes:
"foobar_pkey" PRIMARY KEY, btree (a)
Access method: heap
postgres:5432 [42106]=# select * from foobar where a like '%1%';
ERROR: cache lookup failed for collation 0
---
After debugging it, I have observed that the code in question was added by
commit 5e1963fb764e9cc092e0f7b58b28985c311431d9 which added support for the
collations with nondeterministic comparison.
The error is coming from get_collation_isdeterministic() when colloid
passed is 0. I think like we do in get_collation_name(), we should return
false here when such collation oid does not exist.
Attached patch doing that change and re-arranged the code to look similar
to get_collation_name(). Also, added small testcase.
---
However, I have not fully understood the code changes done by the said
commit and thus the current behavior i.e. cache lookup error, might be the
expected one. But if that's the case, I kindly request to please explain
why that is expected.
Thanks
--
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index b4f2d0f..69e5d88 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -942,16 +942,19 @@ bool
get_collation_isdeterministic(Oid colloid)
{
HeapTuple tp;
- Form_pg_collation colltup;
- bool result;
tp = SearchSysCache1(COLLOID, ObjectIdGetDatum(colloid));
- if (!HeapTupleIsValid(tp))
- elog(ERROR, "cache lookup failed for collation %u", colloid);
- colltup = (Form_pg_collation) GETSTRUCT(tp);
- result = colltup->collisdeterministic;
- ReleaseSysCache(tp);
- return result;
+ if (HeapTupleIsValid(tp))
+ {
+ Form_pg_collation colltup = (Form_pg_collation) GETSTRUCT(tp);
+ bool result;
+
+ result = colltup->collisdeterministic;
+ ReleaseSysCache(tp);
+ return result;
+ }
+ else
+ return false;
}
/*-- CONSTRAINT CACHE -- */
diff --git a/src/test/regress/expected/collate.out b/src/test/regress/expected/collate.out
index 0dee7d7..a8e1f6e 100644
--- a/src/test/regress/expected/collate.out
+++ b/src/test/regress/expected/collate.out
@@ -676,13 +676,19 @@ SELECT collation for ((SELECT b FROM collate_test1 LIMIT 1));
"C"
(1 row)
+CREATE TABLE byteatable(a bytea primary key, b int);
+SELECT * FROM byteatable WHERE a LIKE '%1%';
+ a | b
+---+---
+(0 rows)
+
--
-- Clean up. Many of these table names will be re-used if the user is
-- trying to run any platform-specific collation tests later, so we
-- must get rid of them.
--
DROP SCHEMA collate_tests CASCADE;
-NOTICE: drop cascades to 17 other objects
+NOTICE: drop cascades to 18 other objects
DETAIL: drop cascades to table collate_test1
drop cascades to table collate_test_like
drop cascades to table collate_test2
@@ -700,3 +706,4 @@ drop cascades to table collate_test21
drop cascades to table collate_test22
drop cascades to collation mycoll2
drop cascades to table collate_test23
+drop cascades to table byteatable
diff --git a/src/test/regress/sql/collate.sql b/src/test/regress/sql/collate.sql
index 89de26a..124daf6 100644
--- a/src/test/regress/sql/collate.sql
+++ b/src/test/regress/sql/collate.sql
@@ -259,6 +259,9 @@ SELECT collation for ((SELECT a FROM collate_test1 LIMIT 1)); -- non-collatable
SELECT collation for ((SELECT b FROM collate_test1 LIMIT 1));
+CREATE TABLE byteatable(a bytea primary key, b int);
+SELECT * FROM byteatable WHERE a LIKE '%1%';
+
--
-- Clean up. Many of these table names will be re-used if the user is
-- trying to run any platform-specific collation tests later, so we
