[PATCH] Avoid collation lookup for "char" statistics

Feng Wu <wufengwufengwufeng@gmail.com> writes:

The internal "char" type is not collatable, and its btree operators
order the byte value directly. Return the one-byte string for CHAROID
without consulting collation state, avoiding a lookup of InvalidOid as
collation 0.

Can you demonstrate that there is a problem worth worrying about here?
We've had no field reports of failures in this code, and I don't see
a problem with, e.g.

explain select * from pg_type a join pg_type b using (typtype);

regards, tom lane

Hi Tom,

Thanks for looking at it. I should have included the exact reproducer in
the first email.

The failure needs histogram statistics on a "char" column, so simple
catalog joins such as the pg_type example do not necessarily reach the
problematic path. For example, I just rechecked that your query plans as
a hash join for me:

explain select * from pg_type a join pg_type b using (typtype);

To reach the failing path, this reproduces it for me on current master:

create temp table char_stats_1 (c "char");
create temp table char_stats_2 (c "char");

insert into char_stats_1
select v::"char"
from unnest(array['I','S','c','i','m','p','r','t','v']) as v,
generate_series(1,
case when v in ('i','v','r','t') then 50 else 1 end);

insert into char_stats_2
select v::"char"
from unnest(array['a','e','i']) as v,
generate_series(1, case when v = 'i' then 50 else 5 end);

analyze char_stats_1;
analyze char_stats_2;

set enable_hashjoin = off;
set enable_nestloop = off;

explain (costs off)
select count(*)
from char_stats_1 s1
join char_stats_2 s2 on s1.c = s2.c;

After ANALYZE, the first column has histogram statistics, e.g.:

tablename | attname | n_distinct | most_common_vals | histogram_bounds
-------------+---------+------------+------------------+------------------
char_stats_1 | c | 9 | {i,r,t,v} | {I,S,c,m,p}
char_stats_2 | c | 3 | {i,a,e} |

Without the patch, planning fails with:

ERROR: XX000: cache lookup failed for collation 0
LOCATION: pg_newlocale_from_collation, pg_locale.c:1211

The reason is that mergejoinscansel() calls scalarineqsel() while costing
the merge join. That can reach convert_to_scalar() for histogram bounds.
For CHAROID, convert_string_datum() builds a one-byte string, but then
still calls pg_newlocale_from_collation() with the clause input collation,
which is InvalidOid for the non-collatable "char" type.

The patch only skips that collation lookup for CHAROID. For text,
varchar, bpchar, and name, the existing locale handling is unchanged.

Regards,
Feng

Feng Wu <wufengwufengwufeng@gmail.com> writes:

Thanks for looking at it. I should have included the exact reproducer in
the first email.
The failure needs histogram statistics on a "char" column, so simple
catalog joins such as the pg_type example do not necessarily reach the
problematic path.

Ah, now I understand. The failing code is only reached if we have
a histogram on a "char" column. For almost all the catalogs with
"char" columns, there is no histogram because there are very few
distinct values and so the MCV list accounts for all of them.
Even where there is a histogram, it'd be unlikely for someone to
issue a query that would hit this code, so the lack of prior
reports isn't as surprising as I thought.

Your example could be simplified though. The failure is within
scalarineqsel, so we don't need a join at all, just an inequality
qual:

regression=# explain verbose select * from char_stats_1 where c < 'c';
ERROR: cache lookup failed for collation 0

I agree that just skipping the collation correction is the right
fix. We could make the patch a bit shorter by simply returning
out of the CHAROID case in the preceding switch. Will see to it.

regards, tom lane

I wrote:

Even where there is a histogram, it'd be unlikely for someone to
issue a query that would hit this code, so the lack of prior
reports isn't as surprising as I thought.

I had supposed that this was an old bug, but on attempting to
back-patch I found it wasn't broken before v18. So that's another
big reason for lack of prior reports. Commit 06421b084 replaced a
call to lc_collate_is_c(), which tolerated InvalidOid, with
pg_newlocale_from_collation() which doesn't.

Seeing this, I thought we'd be best off to put the test where
you had it but make it test for !OidIsValid(collid) rather than
hard-wiring typid == CHAROID. There probably aren't other cases
where we reach here with collid 0, but if there are, we want the
code to not fail, as it did not before.

Pushed with a test based on an inequality comparison (in HEAD
only, because there didn't seem to be a suitable test file in 18).

regards, tom lane

Thanks Tom, the generalized InvalidOid check makes sense to me.

Regards,
Feng