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
Attachments:
fix_cache_lookup_error_collation.patchtext/x-patch; charset=US-ASCII; name=fix_cache_lookup_error_collation.patchDownload
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
Jeevan Chalke <jeevan.chalke@enterprisedb.com> 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
On Thu, Apr 11, 2019 at 9:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Jeevan Chalke <jeevan.chalke@enterprisedb.com> 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 0Good 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
Jeevan Chalke <jeevan.chalke@enterprisedb.com> 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
On Thu, Apr 11, 2019 at 10:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Jeevan Chalke <jeevan.chalke@enterprisedb.com> 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 FALSEI 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
Attachments:
fix_cache_lookup_error_collation_v2.patchtext/x-patch; charset=US-ASCII; name=fix_cache_lookup_error_collation_v2.patchDownload
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
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
Attachments:
0001-Unbreak-index-optimization-for-LIKE-on-bytea.patchtext/plain; charset=UTF-8; name=0001-Unbreak-index-optimization-for-LIKE-on-bytea.patch; x-mac-creator=0; x-mac-type=0Download
From acb1542a1f8ebee9c9d6d9322c64c849b2f23e15 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
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
On Fri, Apr 12, 2019 at 1:26 PM Peter Eisentraut <
peter.eisentraut@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
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