Thread
Commits
-
Tab-complete CREATE COLLATION.
- 5bc09a74719d 14.0 landed
-
Refactor get_collation_current_version().
- beb4480c853a 14.0 landed
-
pg_collation_actual_version() -> pg_collation_current_version().
- 9cf184cc0599 14.0 landed
-
Hide internal error for pg_collation_actual_version(<bad OID>).
- 0fb0a0503bfc 14.0 landed
-
pg_collation_actual_version() ERROR: cache lookup failed for collation 123
Justin Pryzby <pryzby@telsasoft.com> — 2021-01-17T21:59:40Z
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
-
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
Thomas Munro <thomas.munro@gmail.com> — 2021-01-17T22:22:13Z
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".
-
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
Thomas Munro <thomas.munro@gmail.com> — 2021-02-17T02:08:36Z
On Mon, Jan 18, 2021 at 11:22 AM Thomas Munro <thomas.munro@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.
-
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
Michael Paquier <michael@paquier.xyz> — 2021-02-17T07:04:01Z
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 -
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
Thomas Munro <thomas.munro@gmail.com> — 2021-02-17T21:45:53Z
On Wed, Feb 17, 2021 at 8:04 PM Michael Paquier <michael@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! -
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
Michael Paquier <michael@paquier.xyz> — 2021-02-18T07:15:44Z
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 -
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
Thomas Munro <thomas.munro@gmail.com> — 2021-02-22T05:34:22Z
On Thu, Feb 18, 2021 at 8:15 PM Michael Paquier <michael@paquier.xyz> 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. -
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
Michael Paquier <michael@paquier.xyz> — 2021-02-22T07:27:17Z
On Mon, Feb 22, 2021 at 06:34:22PM +1300, Thomas Munro wrote: > On Thu, Feb 18, 2021 at 8:15 PM Michael Paquier <michael@paquier.xyz> 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 -
Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123
Thomas Munro <thomas.munro@gmail.com> — 2021-02-22T11:30:18Z
On Mon, Feb 22, 2021 at 8:27 PM Michael Paquier <michael@paquier.xyz> wrote: > Looks good to me, thanks! Pushed, with one further small change: I realised that tab completion should use a "schema" query.