Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
On Mon, Feb 22, 2021 at 8:27 PM Michael Paquier wrote: > Looks good to me, thanks! Pushed, with one further small change: I realised that tab completion should use a "schema" query.
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
On Mon, Feb 22, 2021 at 06:34:22PM +1300, Thomas Munro wrote:
> On Thu, Feb 18, 2021 at 8:15 PM Michael Paquier wrote:
>> Could you just add a test with pg_collation_current_version(0)?
>
> Done.
>
>> + pg_strncasecmp("POSIX.", collcollate, 6) != 0)
>>
>> I didn't know that "POSIX." was possible.
>
> Yeah, that isn't valid on my (quite current) GNU or FreeBSD systems,
> and doesn't show up in their "locale -a" output, but I wondered about
> that theoretical possibility and googled it, and that showed that it
> does exist out there, though I don't know where/which versions,
> possibly only a long time ago. You know what, let's just forget that
> bit, it's not necessary. Removed.
>
>> While on it, I guess that you could add tab completion support for
>> CREATE COLLATION foo FROM.
>
> Good point. Added.
>
>> And shouldn't CREATE COLLATION complete
>> with the list of existing collation?
>
> Rght, fixed.
Looks good to me, thanks!
--
Michael
signature.asc
Description: PGP signature
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
On Thu, Feb 18, 2021 at 8:15 PM Michael Paquier wrote:
> Could you just add a test with pg_collation_current_version(0)?
Done.
> + pg_strncasecmp("POSIX.", collcollate, 6) != 0)
>
> I didn't know that "POSIX." was possible.
Yeah, that isn't valid on my (quite current) GNU or FreeBSD systems,
and doesn't show up in their "locale -a" output, but I wondered about
that theoretical possibility and googled it, and that showed that it
does exist out there, though I don't know where/which versions,
possibly only a long time ago. You know what, let's just forget that
bit, it's not necessary. Removed.
> While on it, I guess that you could add tab completion support for
> CREATE COLLATION foo FROM.
Good point. Added.
> And shouldn't CREATE COLLATION complete
> with the list of existing collation?
Rght, fixed.
From 1a3092e6ca386bac3292aa60c20e3b2a2ce08fff Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Wed, 17 Feb 2021 14:10:40 +1300
Subject: [PATCH v3 1/4] Hide internal error for
pg_collation_actual_version().
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
Reviewed-by: Michael Paquier
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 +-
src/test/regress/expected/collate.icu.utf8.out | 14 ++
src/test/regress/sql/collate.icu.utf8.sql | 5 +
7 files changed, 33 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
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
On Thu, Feb 18, 2021 at 10:45:53AM +1300, Thomas Munro wrote:
> 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!
Could you just add a test with pg_collation_current_version(0)?
+ pg_strncasecmp("POSIX.", collcollate, 6) != 0)
I didn't know that "POSIX." was possible.
While on it, I guess that you could add tab completion support for
CREATE COLLATION foo FROM. And shouldn't CREATE COLLATION complete
with the list of existing collation?
--
Michael
signature.asc
Description: PGP signature
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
On Wed, Feb 17, 2021 at 8:04 PM Michael Paquier 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
Date: Wed, 17 Feb 2021 14:10:40 +1300
Subject: [PATCH v2 1/4] Hide internal error for
pg_collation_actual_version().
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
Reviewed-by: Michael Paquier
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").
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
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.
--
Michael
signature.asc
Description: PGP signature
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
On Mon, Jan 18, 2021 at 11:22 AM Thomas Munro 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
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
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, );
+
+ 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(Oi
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
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. I'm also wondering if it would be better to name that thing with "current" rather than "actual".
pg_collation_actual_version() ERROR: cache lookup failed for collation 123
As of 257836a75, this returns: |postgres=# SELECT pg_collation_actual_version(123); |ERROR: cache lookup failed for collation 123 |postgres=# \errverbose |ERROR: XX000: cache lookup failed for collation 123 |LOCATION: get_collation_version_for_oid, pg_locale.c:1754 I'm of the impression that's considered to be a bad behavior for SQL accessible functions. In v13, it did the same thing but with different language: |ts=# SELECT pg_collation_actual_version(123); |ERROR: collation with OID 123 does not exist |ts=# \errverbose |ERROR: 42704: collation with OID 123 does not exist |LOCATION: pg_collation_actual_version, collationcmds.c:367 -- Justin
