On Mon, Jan 18, 2021 at 11:22 AM Thomas Munro <thomas.mu...@gmail.com> wrote: > On Mon, Jan 18, 2021 at 10:59 AM Justin Pryzby > |postgres=# SELECT > pg_collation_actual_version(123); > > |ERROR: cache lookup failed for collation 123 > > Yeah, not a great user experience. Will fix next week; perhaps > get_collation_version_for_oid() needs missing_ok and found flags, or > something like that.
Here's a patch that gives: postgres=# select pg_collation_actual_version(123); ERROR: no collation found for OID 123 It's a bit of an odd function, it's user-facing yet deals in OIDs. > I'm also wondering if it would be better to name that thing with > "current" rather than "actual". Here's a patch to do that (note to self: would need a catversion bump). While tidying up around here, I was dissatisfied with the fact that there are three completely different ways of excluding "C[.XXX]" and "POSIX" for three OSes, so here's a patch to merge them. Also, here's the missing tab completion for CREATE COLLATION, since it's rare enough to be easy to forget the incantations required.
From 992608eb81265856af3b9cbcb63597d91876090a Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Wed, 17 Feb 2021 14:10:40 +1300 Subject: [PATCH 1/4] Improve error message for pg_collation_actual_version(). Instead of an internal "cache lookup failed" message, show a message intended for end user consumption, considering this is a user-exposed function. Reported-by: Justin Pryzby <pry...@telsasoft.com> Discussion: https://postgr.es/m/20210117215940.GE8560%40telsasoft.com --- src/backend/catalog/index.c | 5 +++-- src/backend/catalog/pg_depend.c | 3 ++- src/backend/commands/collationcmds.c | 8 +++++++- src/backend/utils/adt/pg_locale.c | 17 +++++++++++++++-- src/include/utils/pg_locale.h | 2 +- 5 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 1514937748..1c808f55c5 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1290,7 +1290,8 @@ do_collation_version_check(const ObjectAddress *otherObject, return false; /* Ask the provider for the current version. Give up if unsupported. */ - current_version = get_collation_version_for_oid(otherObject->objectId); + current_version = get_collation_version_for_oid(otherObject->objectId, + NULL); if (!current_version) return false; @@ -1369,7 +1370,7 @@ do_collation_version_update(const ObjectAddress *otherObject, if (OidIsValid(*coll) && otherObject->objectId != *coll) return false; - *new_version = get_collation_version_for_oid(otherObject->objectId); + *new_version = get_collation_version_for_oid(otherObject->objectId, NULL); return true; } diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c index 63da24322d..aee59eef39 100644 --- a/src/backend/catalog/pg_depend.c +++ b/src/backend/catalog/pg_depend.c @@ -116,7 +116,8 @@ recordMultipleDependencies(const ObjectAddress *depender, referenced->objectId == POSIX_COLLATION_OID) continue; - version = get_collation_version_for_oid(referenced->objectId); + version = get_collation_version_for_oid(referenced->objectId, + NULL); /* * Default collation is pinned, so we need to force recording diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c index 9634ae6809..b3c59e6e9f 100644 --- a/src/backend/commands/collationcmds.c +++ b/src/backend/commands/collationcmds.c @@ -272,8 +272,14 @@ pg_collation_actual_version(PG_FUNCTION_ARGS) { Oid collid = PG_GETARG_OID(0); char *version; + bool found; - version = get_collation_version_for_oid(collid); + version = get_collation_version_for_oid(collid, &found); + + if (!found) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("no collation found for OID %u", collid))); if (version) PG_RETURN_TEXT_P(cstring_to_text(version)); diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index e9c1231f9b..3cd9257800 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -1726,10 +1726,12 @@ get_collation_actual_version(char collprovider, const char *collcollate) /* * Get provider-specific collation version string for a given collation OID. * Return NULL if the provider doesn't support versions, or the collation is - * unversioned (for example "C"). + * unversioned (for example "C"). If "found" is non-NULL, unknown OIDs are + * reported through this output parameter; otherwise, an internal error is + * raised for unknown OIDs. */ char * -get_collation_version_for_oid(Oid oid) +get_collation_version_for_oid(Oid oid, bool *found) { HeapTuple tp; char *version; @@ -1744,6 +1746,8 @@ get_collation_version_for_oid(Oid oid) dbform = (Form_pg_database) GETSTRUCT(tp); version = get_collation_actual_version(COLLPROVIDER_LIBC, NameStr(dbform->datcollate)); + if (found) + *found = true; } else { @@ -1751,10 +1755,19 @@ get_collation_version_for_oid(Oid oid) tp = SearchSysCache1(COLLOID, ObjectIdGetDatum(oid)); if (!HeapTupleIsValid(tp)) + { + if (found) + { + *found = false; + return NULL; + } elog(ERROR, "cache lookup failed for collation %u", oid); + } collform = (Form_pg_collation) GETSTRUCT(tp); version = get_collation_actual_version(collform->collprovider, NameStr(collform->collcollate)); + if (found) + *found = true; } ReleaseSysCache(tp); diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h index 34dff74bd1..9d73774a19 100644 --- a/src/include/utils/pg_locale.h +++ b/src/include/utils/pg_locale.h @@ -103,7 +103,7 @@ typedef struct pg_locale_struct *pg_locale_t; extern pg_locale_t pg_newlocale_from_collation(Oid collid); -extern char *get_collation_version_for_oid(Oid collid); +extern char *get_collation_version_for_oid(Oid collid, bool *found); #ifdef USE_ICU extern int32_t icu_to_uchar(UChar **buff_uchar, const char *buff, size_t nbytes); -- 2.30.0
From 190263b628cd48f5058f983c4ef33389a09afae8 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Wed, 17 Feb 2021 14:19:40 +1300 Subject: [PATCH 2/4] pg_collation_actual_version() -> pg_collation_current_version(). The new name seems a bit more natural. Discussion: https://postgr.es/m/20210117215940.GE8560%40telsasoft.com --- doc/src/sgml/func.sgml | 8 ++++---- src/backend/commands/collationcmds.c | 2 +- src/backend/utils/adt/pg_locale.c | 14 +++++++------- src/include/catalog/pg_proc.dat | 4 ++-- src/test/regress/expected/collate.icu.utf8.out | 2 +- src/test/regress/sql/collate.icu.utf8.sql | 2 +- 6 files changed, 16 insertions(+), 16 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 1ab31a9056..d8224272a5 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -26227,14 +26227,14 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); <row> <entry role="func_table_entry"><para role="func_signature"> <indexterm> - <primary>pg_collation_actual_version</primary> + <primary>pg_collation_current_version</primary> </indexterm> - <function>pg_collation_actual_version</function> ( <type>oid</type> ) + <function>pg_collation_current_version</function> ( <type>oid</type> ) <returnvalue>text</returnvalue> </para> <para> - Returns the actual version of the collation object as it is currently - installed in the operating system. <literal>null</literal> is returned + Returns the version of the collation object as reported by the ICU + library or operating system. <literal>null</literal> is returned on operating systems where <productname>PostgreSQL</productname> doesn't have support for versions. </para></entry> diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c index b3c59e6e9f..a694e19dd1 100644 --- a/src/backend/commands/collationcmds.c +++ b/src/backend/commands/collationcmds.c @@ -268,7 +268,7 @@ IsThereCollationInNamespace(const char *collname, Oid nspOid) } Datum -pg_collation_actual_version(PG_FUNCTION_ARGS) +pg_collation_current_version(PG_FUNCTION_ARGS) { Oid collid = PG_GETARG_OID(0); char *version; diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index 3cd9257800..1d8af6fb88 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -127,8 +127,8 @@ static char *IsoLocaleName(const char *); /* MSVC specific */ static void icu_set_collation_attributes(UCollator *collator, const char *loc); #endif -static char *get_collation_actual_version(char collprovider, - const char *collcollate); +static char *get_collation_current_version(char collprovider, + const char *collcollate); /* * pg_perm_setlocale @@ -1610,7 +1610,7 @@ pg_newlocale_from_collation(Oid collid) * the operating system/library. */ static char * -get_collation_actual_version(char collprovider, const char *collcollate) +get_collation_current_version(char collprovider, const char *collcollate) { char *collversion = NULL; @@ -1744,8 +1744,8 @@ get_collation_version_for_oid(Oid oid, bool *found) if (!HeapTupleIsValid(tp)) elog(ERROR, "cache lookup failed for database %u", MyDatabaseId); dbform = (Form_pg_database) GETSTRUCT(tp); - version = get_collation_actual_version(COLLPROVIDER_LIBC, - NameStr(dbform->datcollate)); + version = get_collation_current_version(COLLPROVIDER_LIBC, + NameStr(dbform->datcollate)); if (found) *found = true; } @@ -1764,8 +1764,8 @@ get_collation_version_for_oid(Oid oid, bool *found) elog(ERROR, "cache lookup failed for collation %u", oid); } collform = (Form_pg_collation) GETSTRUCT(tp); - version = get_collation_actual_version(collform->collprovider, - NameStr(collform->collcollate)); + version = get_collation_current_version(collform->collprovider, + NameStr(collform->collcollate)); if (found) *found = true; } diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 1487710d59..16044125ba 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -11321,9 +11321,9 @@ { oid => '3448', descr => 'get actual version of collation from operating system', - proname => 'pg_collation_actual_version', procost => '100', + proname => 'pg_collation_current_version', procost => '100', provolatile => 'v', prorettype => 'text', proargtypes => 'oid', - prosrc => 'pg_collation_actual_version' }, + prosrc => 'pg_collation_current_version' }, # system management/monitoring related functions { oid => '3353', descr => 'list files in the log directory', diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out index bc3752e923..970638ed37 100644 --- a/src/test/regress/expected/collate.icu.utf8.out +++ b/src/test/regress/expected/collate.icu.utf8.out @@ -2018,7 +2018,7 @@ SELECT objid::regclass::text collate "C", refobjid::regcollation::text collate " CASE WHEN refobjid = 'default'::regcollation THEN 'XXX' -- depends on libc version support WHEN refobjversion IS NULL THEN 'version not tracked' -WHEN refobjversion = pg_collation_actual_version(refobjid) THEN 'up to date' +WHEN refobjversion = pg_collation_current_version(refobjid) THEN 'up to date' ELSE 'out of date' END AS version FROM pg_depend d diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql index 0de2ed8d85..be5b464ffa 100644 --- a/src/test/regress/sql/collate.icu.utf8.sql +++ b/src/test/regress/sql/collate.icu.utf8.sql @@ -820,7 +820,7 @@ SELECT objid::regclass::text collate "C", refobjid::regcollation::text collate " CASE WHEN refobjid = 'default'::regcollation THEN 'XXX' -- depends on libc version support WHEN refobjversion IS NULL THEN 'version not tracked' -WHEN refobjversion = pg_collation_actual_version(refobjid) THEN 'up to date' +WHEN refobjversion = pg_collation_current_version(refobjid) THEN 'up to date' ELSE 'out of date' END AS version FROM pg_depend d -- 2.30.0
From 4885e32e5e978631b1232feee4c2078a589a35cd Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Wed, 17 Feb 2021 14:36:43 +1300 Subject: [PATCH 3/4] Refactor get_collation_current_version(). The code paths for three different OSes finished up with three different ways of excluding C[.xxx] and POSIX from consideration. Merge them. Discussion: https://postgr.es/m/20210117215940.GE8560%40telsasoft.com --- src/backend/utils/adt/pg_locale.c | 35 +++++-------------------------- 1 file changed, 5 insertions(+), 30 deletions(-) diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index 1d8af6fb88..7d2d1a9a04 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -1636,37 +1636,18 @@ get_collation_current_version(char collprovider, const char *collcollate) } else #endif - if (collprovider == COLLPROVIDER_LIBC) + if (collprovider == COLLPROVIDER_LIBC && + pg_strcasecmp("C", collcollate) != 0 && + pg_strncasecmp("C.", collcollate, 2) != 0 && + pg_strcasecmp("POSIX", collcollate) != 0 && + pg_strncasecmp("POSIX.", collcollate, 5) != 0) { #if defined(__GLIBC__) - char *copy = pstrdup(collcollate); - char *copy_suffix = strstr(copy, "."); - bool need_version = true; - - /* - * Check for names like C.UTF-8 by chopping off the encoding suffix on - * our temporary copy, so we can skip the version. - */ - if (copy_suffix) - *copy_suffix = '\0'; - if (pg_strcasecmp("c", copy) == 0 || - pg_strcasecmp("posix", copy) == 0) - need_version = false; - pfree(copy); - if (!need_version) - return NULL; - /* Use the glibc version because we don't have anything better. */ collversion = pstrdup(gnu_get_libc_version()); #elif defined(LC_VERSION_MASK) locale_t loc; - /* C[.encoding] and POSIX never change. */ - if (strcmp("C", collcollate) == 0 || - strncmp("C.", collcollate, 2) == 0 || - strcmp("POSIX", collcollate) == 0) - return NULL; - /* Look up FreeBSD collation version. */ loc = newlocale(LC_COLLATE, collcollate, NULL); if (loc) @@ -1687,12 +1668,6 @@ get_collation_current_version(char collprovider, const char *collcollate) NLSVERSIONINFOEX version = {sizeof(NLSVERSIONINFOEX)}; WCHAR wide_collcollate[LOCALE_NAME_MAX_LENGTH]; - /* These would be invalid arguments, but have no version. */ - if (pg_strcasecmp("c", collcollate) == 0 || - pg_strcasecmp("posix", collcollate) == 0) - return NULL; - - /* For all other names, ask the OS. */ MultiByteToWideChar(CP_ACP, 0, collcollate, -1, wide_collcollate, LOCALE_NAME_MAX_LENGTH); if (!GetNLSVersionEx(COMPARE_STRING, wide_collcollate, &version)) -- 2.30.0
From eb83951dbe44940e8e8239dac214cbab9d3e0e9e Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Wed, 17 Feb 2021 15:05:04 +1300 Subject: [PATCH 4/4] Tab-complete CREATE COLLATION. Discussion: https://postgr.es/m/20210117215940.GE8560%40telsasoft.com --- src/bin/psql/tab-complete.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 1e1c315bae..da45877ed4 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -2424,6 +2424,20 @@ psql_completion(const char *text, int start, int end) else if (Matches("CREATE", "ACCESS", "METHOD", MatchAny, "TYPE", MatchAny)) COMPLETE_WITH("HANDLER"); + /* CREATE COLLATION */ + else if (Matches("CREATE", "COLLATION", MatchAny)) + COMPLETE_WITH("("); + else if (HeadMatches("CREATE", "COLLATION")) + { + if (TailMatches("(|*,")) + COMPLETE_WITH("LOCALE =", "LC_COLLATE =", "LC_CTYPE =", + "PROVIDER =", "DETERMINISTIC ="); + else if (TailMatches("PROVIDER", "=")) + COMPLETE_WITH("libc", "icu"); + else if (TailMatches("DETERMINISTIC", "=")) + COMPLETE_WITH("true", "false"); + } + /* CREATE DATABASE */ else if (Matches("CREATE", "DATABASE", MatchAny)) COMPLETE_WITH("OWNER", "TEMPLATE", "ENCODING", "TABLESPACE", -- 2.30.0
