Re: Per-column collation

On Tue, Nov 16, 2010 at 04:42, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On mån, 2010-11-15 at 11:34 +0100, Pavel Stehule wrote:
>> I am checking a patch. I found a problem with initdb
> Ah, late night brain farts, it appears.  Here is a corrected version.

This version cannot be applied cleanly any more. Please update it.
(I think you don't have to include changes for catversion.h)
./src/backend/optimizer/util/plancat.c.rej
./src/backend/optimizer/plan/createplan.c.rej
./src/backend/optimizer/path/indxpath.c.rej
./src/include/catalog/catversion.h.rej

I didn't compile nor run the patched server, but I found a couple of
issues in the design and source code:

* COLLATE information must be explicitly passed by caller in the patch,
but we might forgot the handover when we write new codes. Is it possible
to pass it automatically, say using a global variable? If we could do so,
existing extensions might work with collation without rewritten.

* Did you check the regression test on Windows? We probably cannot use
en_US.utf8 on Windows. Also, some output of the test includes non-ASCII
characters. How will we test COLLATE feature on non-UTF8 databases?

[src/test/regress/sql/collate.sql]
+CREATE TABLE collate_test1 (
+ a int,
+ b text COLLATE "en_US.utf8" not null
+);

* Did you see any performance regression by collation?
I found a bug in lc_collate_is_c(); result >= 0 should be
checked before any other checks. SearchSysCache1() here
would be a performance regression.

[src/backend/utils/adt/pg_locale.c]
-lc_collate_is_c(void)
+lc_collate_is_c(Oid collation)
{
...
+ tp = SearchSysCache1(COLLOID, ObjectIdGetDatum(collation));
...
HERE => if (result >= 0)
return (bool) result;

--
Itagaki Takahiro