pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedPossible to create a hidden collation
5+ messages / 3 participants
[nested] [flat]
* Possible to create a hidden collation
@ 2023-05-11 20:45 Jeff Davis <pgsql@j-davis.com>
0 siblings, 2 replies; 5+ messages in thread
From: Jeff Davis @ 2023-05-11 20:45 UTC (permalink / raw)
To: pgsql-bugs
Repro:
create collation test(provider=icu,
deterministic=false, locale='@colStrength=primary');
create collation "en_US"(provider=icu,
deterministic=false, locale='@colStrength=primary');
select 'a' = 'A' collate test; -- true
select 'a' = 'A' collate "en_US"; -- false
drop collation "en_US"; -- drops built-in collation
select 'a' = 'A' collate "en_US"; -- true
Explanation:
The second collation named "en_US" is hidden behind the built-in
collation "en_US" because the former is created with collencoding=-1
(as all icu collations are), and the latter is a libc collation with
collencoding equal to the current database encoding (which takes
precedence).
It's a minor bug, but could be surprising behavior.
Solution:
There's no reason to create user defined collations with collencoding=-
1. As far as I can tell, collencoding is only there to hide built-in
collations (inherited from template0) that aren't compatible with the
current database encoding. So we can just always create user-defined
collations with collencoding=GetDatabaseEncoding().
Patch attached (extracted from series posted elsewhere because this is
a bug in old versions). I don't intend to backport because the bug is
minor and it will affect catalog contents. If others agree with the
fix, then I'll also bump the catversion, though I'm not sure that it's
strictly necessary.
Regards,
Jeff Davis
Attachments:
v5-0001-For-user-defined-collations-never-set-collencodin.patch (text/x-patch, 4.0K) download
From fc66f02976bb11b629bcf71346c2858eccbcf1a3 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Thu, 11 May 2023 10:36:04 -0700
Subject: [PATCH v5 1/7] For user-defined collations, never set
collencoding=-1.
For new user-defined collations, always set collencoding to the
current database encoding so that it is never shadowed by a built-in
collation.
Built in collations that work with any encoding may have
collencoding=-1, and if a user defines a collation with the same name,
it will shadow the built-in collation.
Previously it was possible to create an ICU collation (which was
assigned collencoding=-1) that was shadowed by a built-in collation
and completely inaccessible.
---
src/backend/commands/collationcmds.c | 28 +++++++++++++------
.../regress/expected/collate.icu.utf8.out | 2 +-
2 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c
index c91fe66d9b..a53700256b 100644
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -302,16 +302,29 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("ICU rules cannot be specified unless locale provider is ICU")));
+ /*
+ * The collencoding is used to hide built-in collations that are
+ * incompatible with the current database encoding, allowing users to
+ * define a compatible collation with the same name if
+ * desired. Built-in collations that work with any encoding have
+ * collencoding=-1.
+ *
+ * A collation that's a match to the current database encoding will
+ * shadow a collation with the same name and collencoding=-1. We never
+ * want a user-created collation to be shadowed by a built-in
+ * collation, so for user-created collations, always set collencoding
+ * to the current database encoding.
+ */
+ collencoding = GetDatabaseEncoding();
+
if (collprovider == COLLPROVIDER_ICU)
{
#ifdef USE_ICU
/*
- * We could create ICU collations with collencoding == database
- * encoding, but it seems better to use -1 so that it matches the
- * way initdb would create ICU collations. However, only allow
- * one to be created when the current database's encoding is
- * supported. Otherwise the collation is useless, plus we get
- * surprising behaviors like not being able to drop the collation.
+ * Only allow an ICU collation to be created when the current
+ * database's encoding is supported. Otherwise the collation is
+ * useless, plus we get surprising behaviors like not being able
+ * to drop the collation.
*
* Skip this test when !USE_ICU, because the error we want to
* throw for that isn't thrown till later.
@@ -321,11 +334,10 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("current database's encoding is not supported with this provider")));
#endif
- collencoding = -1;
}
else
{
- collencoding = GetDatabaseEncoding();
+ Assert(collprovider == COLLPROVIDER_LIBC);
check_encoding_locale_matches(collencoding, collcollate, collctype);
}
}
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index b5a221b030..9c9e1e4f48 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -1062,7 +1062,7 @@ SELECT collname FROM pg_collation WHERE collname LIKE 'test%' ORDER BY 1;
ALTER COLLATION test1 RENAME TO test11;
ALTER COLLATION test0 RENAME TO test11; -- fail
-ERROR: collation "test11" already exists in schema "collate_tests"
+ERROR: collation "test11" for encoding "UTF8" already exists in schema "collate_tests"
ALTER COLLATION test1 RENAME TO test22; -- fail
ERROR: collation "test1" for encoding "UTF8" does not exist
ALTER COLLATION test11 OWNER TO regress_test_role;
--
2.34.1
* Re: Possible to create a hidden collation
@ 2023-05-12 00:02 Tom Lane <tgl@sss.pgh.pa.us>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 1 reply; 5+ messages in thread
From: Tom Lane @ 2023-05-12 00:02 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: pgsql-bugs
Jeff Davis <pgsql@j-davis.com> writes:
> There's no reason to create user defined collations with collencoding=-
> 1. As far as I can tell, collencoding is only there to hide built-in
> collations (inherited from template0) that aren't compatible with the
> current database encoding. So we can just always create user-defined
> collations with collencoding=GetDatabaseEncoding().
What happens if somebody makes a user-defined collation in template0
and then clones that into a different encoding? I guess the user-defined
collation would then be hidden/inaccessible in the clone DB, so maybe it's
fine.
regards, tom lane
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Possible to create a hidden collation
@ 2023-05-12 00:27 Jeff Davis <pgsql@j-davis.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 5+ messages in thread
From: Jeff Davis @ 2023-05-12 00:27 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs
On Thu, 2023-05-11 at 20:02 -0400, Tom Lane wrote:
> What happens if somebody makes a user-defined collation in template0
> and then clones that into a different encoding?
I didn't know changing template0 was supported behavior for normal
operation.
> I guess the user-defined
> collation would then be hidden/inaccessible in the clone DB, so maybe
> it's
> fine.
Assuming we do support changes to template0, the fix would prevent
users from making a new ICU collation in template0 that is visible to
all encodings, other than calling pg_import_system_collations(). That
doesn't sound like a big problem; it's already impossible to do that
with libc collations.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Possible to create a hidden collation
@ 2023-05-13 12:28 Daniel Verite <daniel@manitou-mail.org>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 1 reply; 5+ messages in thread
From: Daniel Verite @ 2023-05-13 12:28 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: pgsql-bugs
Jeff Davis wrote:
> create collation test(provider=icu,
> deterministic=false, locale='@colStrength=primary');
> create collation "en_US"(provider=icu,
> deterministic=false, locale='@colStrength=primary');
>
> select 'a' = 'A' collate test; -- true
> select 'a' = 'A' collate "en_US"; -- false
> drop collation "en_US"; -- drops built-in collation
> select 'a' = 'A' collate "en_US"; -- true
>
> Explanation:
>
> The second collation named "en_US" is hidden behind the built-in
> collation "en_US" because the former is created with collencoding=-1
> (as all icu collations are), and the latter is a libc collation with
> collencoding equal to the current database encoding (which takes
> precedence).
>
> It's a minor bug, but could be surprising behavior.
ISTM that this behavior is not due to collencoding=-1, but
to the custom "en_US" collation being in the "public" schema
whereas the built-in "en_US" is in "pg_catalog".
Assuming a default search_path,
- create collation "en_US"(...) means
create collation "public"."en_US"(...)
- select 'a' = 'A' collate "en_US" means
select 'a' = 'A' collate "pg_catalog"."en_US"
- drop collation "en_US" means
drop collation "pg_catalog"."en_US"
So in practice the new collation en_US is not being seen
until the system collation is dropped, independently of
collencoding.
Also, the proposed patch doesn't appear to change the outcome
of the sequence of statements given as an example, which is both
expected considering the above, and surprising because you imply
that it should improve the user experience.
What I'm seeing after applying it:
Initial state:
postgres=# select * from pg_collation where collname='en_US' \gx
-[ RECORD 1 ]-------+-----------
oid | 12419
collname | en_US
collnamespace | 11
collowner | 10
collprovider | c
collisdeterministic | t
collencoding | 6
collcollate | en_US.utf8
collctype | en_US.utf8
colliculocale |
collicurules |
collversion | 2.35
postgres=# create collation "en_US"(provider=icu,
postgres(# deterministic=false, locale='@colStrength=primary');
NOTICE: using standard form "und-u-ks-level1" for locale
"@colStrength=primary"
CREATE COLLATION
postgres=# select * from pg_collation where collname='en_US' \gx
-[ RECORD 1 ]-------+----------------
oid | 12419
collname | en_US
collnamespace | 11
collowner | 10
collprovider | c
collisdeterministic | t
collencoding | 6
collcollate | en_US.utf8
collctype | en_US.utf8
colliculocale |
collicurules |
collversion | 2.35
-[ RECORD 2 ]-------+----------------
oid | 16388
collname | en_US
collnamespace | 2200
collowner | 10
collprovider | i
collisdeterministic | f
collencoding | 6
collcollate |
collctype |
colliculocale | und-u-ks-level1
collicurules |
collversion | 153.112
(I notice collencoding=6 for the new collation in the public namespace)
postgres=# select 'a' = 'A' collate "en_US";
?column?
----------
f
(1 row)
(the libc collation is still used, because pg_catalog comes first)
postgres=# drop collation "en_US";
DROP COLLATION
postgres=# select * from pg_collation where collname='en_US' \gx
-[ RECORD 1 ]-------+----------------
oid | 16388
collname | en_US
collnamespace | 2200
collowner | 10
collprovider | i
collisdeterministic | f
collencoding | 6
collcollate |
collctype |
colliculocale | und-u-ks-level1
collicurules |
collversion | 153.112
(the collation in pg_catalog has been dropped, so we're left with only
the custom collation).
So maybe it's better to set collencoding to the db encoding as done
by the patch, but it's not clear what concrete problem it solves.
Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Possible to create a hidden collation
@ 2023-05-15 22:04 Jeff Davis <pgsql@j-davis.com>
parent: Daniel Verite <daniel@manitou-mail.org>
0 siblings, 0 replies; 5+ messages in thread
From: Jeff Davis @ 2023-05-15 22:04 UTC (permalink / raw)
To: Daniel Verite <daniel@manitou-mail.org>; +Cc: pgsql-bugs
On Sat, 2023-05-13 at 14:28 +0200, Daniel Verite wrote:
> ISTM that this behavior is not due to collencoding=-1, but
> to the custom "en_US" collation being in the "public" schema
> whereas the built-in "en_US" is in "pg_catalog".
My mistake -- it looks like this is not a bug and I don't see a real
problem here. Thank you for looking.
> So maybe it's better to set collencoding to the db encoding as done
> by the patch, but it's not clear what concrete problem it solves.
I was a bit bothered by the inconsistency between libc and icu here,
and I still feel like this patch is probably the right thing to do.
If nothing else, right now the error messages are inconsistent:
create collation test(provider='libc', locale='en_US.utf8');
CREATE COLLATION
create collation test(provider='libc', locale='en_US.utf8');
ERROR: collation "test" for encoding "UTF8" already exists
create collation test(provider='icu', locale='und');
ERROR: collation "test" already exists
The patch fixes that. Not terribly important, but having a consistent
catalog representation seems like a good idea.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-05-15 22:04 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-05-11 20:45 Possible to create a hidden collation Jeff Davis <pgsql@j-davis.com>
2023-05-12 00:02 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-05-12 00:27 ` Jeff Davis <pgsql@j-davis.com>
2023-05-13 12:28 ` Daniel Verite <daniel@manitou-mail.org>
2023-05-15 22:04 ` Jeff Davis <pgsql@j-davis.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox