cache lookup failed for collation 0

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

cache lookup failed for collation 0

Jeevan Chalke
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


fix_cache_lookup_error_collation.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: cache lookup failed for collation 0

Tom Lane-2
Jeevan Chalke <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: cache lookup failed for collation 0

Jeevan Chalke


On Thu, Apr 11, 2019 at 9:07 PM Tom Lane <[hidden email]> wrote:
Jeevan Chalke <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: cache lookup failed for collation 0

Tom Lane-2
Jeevan Chalke <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: cache lookup failed for collation 0

Jeevan Chalke


On Thu, Apr 11, 2019 at 10:50 PM Tom Lane <[hidden email]> wrote:
Jeevan Chalke <[hidden email]> 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


fix_cache_lookup_error_collation_v2.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: cache lookup failed for collation 0

Peter Eisentraut-6
In reply to this post by Jeevan Chalke
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

0001-Unbreak-index-optimization-for-LIKE-on-bytea.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: cache lookup failed for collation 0

Jeevan Chalke


On Fri, Apr 12, 2019 at 1:26 PM Peter Eisentraut <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: cache lookup failed for collation 0

Peter Eisentraut-6
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