Обсуждение: 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
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Вложения
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 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
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 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
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Вложения
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
Вложения
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
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