On Wed, Feb 17, 2021 at 8:04 PM Michael Paquier <mich...@paquier.xyz> wrote:
> On Wed, Feb 17, 2021 at 03:08:36PM +1300, Thomas Munro wrote:
> > 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;
> > }
>
> FWIW, we usually prefer using NULL instead of an error for the result
> of a system function if an object cannot be found because it allows
> users to not get failures in a middle of a full table scan if things
> like an InvalidOid is mixed in the data set. For example, we do that
> in the partition functions, for objectaddress functions, etc. That
> would make this patch set simpler, switching
> get_collation_version_for_oid() to just use a missing_ok argument.
> And that would be more consistent with the other syscache lookup
> functions we have here and there in the tree.
I guess I was trying to preserve a distinction between "unknown OID"
and "this is a collation OID, but I don't have version information for
it" (for example, "C.utf8"). But it hardly matters, and your
suggestion works for me. Thanks for looking!
From 36acff2cdb3dbde81b82ca425b61b0b8a62e27c9 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 v2 1/4] Hide internal error for
pg_collation_actual_version(<bad OID>).
Instead of an unsightly internal "cache lookup failed" message, just
return NULL for bad OIDs, as seems to be the convention for other
similar things.
Reported-by: Justin Pryzby <pry...@telsasoft.com>
Reviewed-by: Michael Paquier <mich...@paquier.xyz>
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 | 2 +-
src/backend/utils/adt/pg_locale.c | 9 +++++++--
src/include/utils/pg_locale.h | 2 +-
5 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index b4ab0b88ad..a9e3679631 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,
+ false);
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, false);
return true;
}
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index 63da24322d..362db7fe91 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,
+ false);
/*
* 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..a7ee452e19 100644
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -273,7 +273,7 @@ pg_collation_actual_version(PG_FUNCTION_ARGS)
Oid collid = PG_GETARG_OID(0);
char *version;
- version = get_collation_version_for_oid(collid);
+ version = get_collation_version_for_oid(collid, true);
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..34b82b9335 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -1726,10 +1726,11 @@ 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"). Unknown OIDs result in NULL if missing_ok is
+ * true.
*/
char *
-get_collation_version_for_oid(Oid oid)
+get_collation_version_for_oid(Oid oid, bool missing_ok)
{
HeapTuple tp;
char *version;
@@ -1751,7 +1752,11 @@ get_collation_version_for_oid(Oid oid)
tp = SearchSysCache1(COLLOID, ObjectIdGetDatum(oid));
if (!HeapTupleIsValid(tp))
+ {
+ if (missing_ok)
+ 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));
diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
index 34dff74bd1..5a37caefbe 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 missing_ok);
#ifdef USE_ICU
extern int32_t icu_to_uchar(UChar **buff_uchar, const char *buff, size_t nbytes);
--
2.30.0
From b52fad59748cf669c509552676907fcb87de8aec 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 v2 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 a7ee452e19..4b76a6051d 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 34b82b9335..2e4c6e9a26 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;
@@ -1743,8 +1743,8 @@ get_collation_version_for_oid(Oid oid, bool missing_ok)
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));
}
else
{
@@ -1758,8 +1758,8 @@ get_collation_version_for_oid(Oid oid, bool missing_ok)
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));
}
ReleaseSysCache(tp);
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 df2a4625f40c4469b2331f6485e607c9d25c23aa 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 v2 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 2e4c6e9a26..697d518e63 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, 6) != 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 721a943dd86071a93089b8c6edd6a77b93ec8b60 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 v2 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 b64db82f02..578e59ea2c 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2433,6 +2433,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
