Thread
Commits
-
Unbreak index optimization for LIKE on bytea
- abb9c63b2c00 12.0 landed
-
Collations with nondeterministic comparison
- 5e1963fb764e 12.0 cited
-
cache lookup failed for collation 0
Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2019-04-11T15:04:37Z
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 -
Re: cache lookup failed for collation 0
Tom Lane <tgl@sss.pgh.pa.us> — 2019-04-11T15:37:17Z
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 -
Re: cache lookup failed for collation 0
Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2019-04-11T16:56:15Z
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 -
Re: cache lookup failed for collation 0
Tom Lane <tgl@sss.pgh.pa.us> — 2019-04-11T17:20:41Z
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
-
Re: cache lookup failed for collation 0
Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2019-04-12T06:13:36Z
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
-
Re: cache lookup failed for collation 0
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-04-12T07:56:09Z
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
-
Re: cache lookup failed for collation 0
Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2019-04-15T05:44:04Z
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
-
Re: cache lookup failed for collation 0
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-04-15T07:37:27Z
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