Thread: amvalidate(): cache lookup failed for operator class 123
Per sqlsmith. postgres=# select amvalidate(123); ERROR: cache lookup failed for operator class 123 postgres=# \errverbose ERROR: XX000: cache lookup failed for operator class 123 LOCATION: amvalidate, amapi.c:125 The usual expectation is that sql callable functions should return null rather than hitting elog(). -- Justin
Justin Pryzby <pryzby@telsasoft.com> writes:
> Per sqlsmith.
> postgres=# select amvalidate(123);
> ERROR: cache lookup failed for operator class 123
> postgres=# \errverbose
> ERROR: XX000: cache lookup failed for operator class 123
> LOCATION: amvalidate, amapi.c:125
> The usual expectation is that sql callable functions should return null rather
> than hitting elog().
Meh. I'm not convinced that that position ought to apply to amvalidate.
Under what circumstances would you be calling that on an opclass that
might be about to be dropped?
regards, tom lane
On Thu, May 13, 2021 at 02:22:16PM -0400, Tom Lane wrote: > Justin Pryzby <pryzby@telsasoft.com> writes: > > Per sqlsmith. > > postgres=# select amvalidate(123); > > ERROR: cache lookup failed for operator class 123 > > postgres=# \errverbose > > ERROR: XX000: cache lookup failed for operator class 123 > > LOCATION: amvalidate, amapi.c:125 > > > The usual expectation is that sql callable functions should return null rather > > than hitting elog(). > > Meh. I'm not convinced that that position ought to apply to amvalidate. > Under what circumstances would you be calling that on an opclass that > might be about to be dropped? Sure, no problem. I'm just passing on the message :) -- Justin
On Thu, May 13, 2021 at 2:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Meh. I'm not convinced that that position ought to apply to amvalidate. I am still of the opinion that we ought to apply it across the board, for consistency. It makes it easier for humans to know which problems are known to be reachable and which are thought to be can't-happen and thus bugs. If we fix cases like this to return a real error code, then anything that comes up as XX000 is likely to be a real bug, whereas if we don't, the things that we're not concerned about have to be filtered out by some other method, probably involving a human being. If the filter that human being has to apply further involves reading Tom Lane's mind and knowing what he will think about a particular report, or alternatively asking him, it just makes complicated something that we could have made simple. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, May 13, 2021 at 2:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Meh. I'm not convinced that that position ought to apply to amvalidate.
> I am still of the opinion that we ought to apply it across the board,
> for consistency.
The main reason I'm concerned about applying that rule to amvalidate
is that then how do you know what's actually an error case?
As a hardly-irrelevant counterexample, we have a whole bunch of
regression tests that do something like
SELECT ...
WHERE NOT amvalidate(oid);
Every one of those is silently and dangerously wrong if amvalidate
might sometimes return null.
regards, tom lane
On Thu, May 13, 2021 at 4:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > The main reason I'm concerned about applying that rule to amvalidate > is that then how do you know what's actually an error case? > > As a hardly-irrelevant counterexample, we have a whole bunch of > regression tests that do something like > > SELECT ... > WHERE NOT amvalidate(oid); > > Every one of those is silently and dangerously wrong if amvalidate > might sometimes return null. Oh, I didn't notice previously that Justin's proposal was to make the functions return NULL. He's correct that this is consistent with other cases, and if we go that way, then these queries need to be updated. I had just been imaging using ereport(ERROR, ...) which wouldn't have that problem. I think either approach would be an improvement over the status quo. -- Robert Haas EDB: http://www.enterprisedb.com
On Sat, May 15, 2021 at 12:00:37PM -0400, Robert Haas wrote: > Oh, I didn't notice previously that Justin's proposal was to make the > functions return NULL. He's correct that this is consistent with other > cases, and if we go that way, then these queries need to be updated. I > had just been imaging using ereport(ERROR, ...) which wouldn't have > that problem. I think either approach would be an improvement over the > status quo. FWIW, I am not convinced with what we could gain by sending NULL as result rather than bump on an ERROR with this function. Don't take me wrong, I like when system functions return a gentle NULL result if something does not exist, if the function is something we document and if it can be used with large catalog scans a-la-pg_class. I don't think that there is any need to apply that to amvalidate() though, and it could mean potential issues with out-of-core modules, rum for one, no? -- Michael
Attachment
On Thu, May 13, 2021 at 12:01:22PM -0500, Justin Pryzby wrote:
> postgres=# select amvalidate(123);
> ERROR: cache lookup failed for operator class 123
> The usual expectation is that sql callable functions should return null rather
> than hitting elog().
On Thu, May 13, 2021 at 2:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Meh. I'm not convinced that that position ought to apply to amvalidate.
On Thu, May 13, 2021 at 04:12:10PM -0400, Robert Haas wrote:
> I am still of the opinion that we ought to apply it across the board,
> for consistency. It makes it easier for humans to know which problems
> are known to be reachable and which are thought to be can't-happen and
> thus bugs. If we fix cases like this to return a real error code, then
> anything that comes up as XX000 is likely to be a real bug, whereas if
> we don't, the things that we're not concerned about have to be
> filtered out by some other method, probably involving a human being.
> If the filter that human being has to apply further involves reading
> Tom Lane's mind and knowing what he will think about a particular
> report, or alternatively asking him, it just makes complicated
> something that we could have made simple.
FWIW, here are some other cases from sqlsmith which hit elog()/XX000:
postgres=# select unknownin('');
ERROR: failed to find conversion function from unknown to text
postgres=# \errverbose
ERROR: XX000: failed to find conversion function from unknown to text
LOCATION: coerce_type, parse_coerce.c:542
postgres=# SELECT pg_catalog.interval( '12 seconds'::interval ,3);
ERROR: unrecognized interval typmod: 3
postgres=# SELECT pg_describe_object(1,0,1);
ERROR: invalid non-zero objectSubId for object class 1
postgres=# SELECT pg_read_file( repeat('a',333));
ERROR: could not open file
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
forreading: File name too long
--
Justin
more elogs hit by sqlsmith (Re: amvalidate(): cache lookup failed for operator class 123)
From
Justin Pryzby
Date:
On Mon, Feb 13, 2023 at 07:50:53AM -0600, Justin Pryzby wrote:
> On Thu, May 13, 2021 at 12:01:22PM -0500, Justin Pryzby wrote:
> > postgres=# select amvalidate(123);
> > ERROR: cache lookup failed for operator class 123
> > The usual expectation is that sql callable functions should return null rather
> > than hitting elog().
>
> On Thu, May 13, 2021 at 2:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Meh. I'm not convinced that that position ought to apply to amvalidate.
>
> On Thu, May 13, 2021 at 04:12:10PM -0400, Robert Haas wrote:
> > I am still of the opinion that we ought to apply it across the board,
> > for consistency. It makes it easier for humans to know which problems
> > are known to be reachable and which are thought to be can't-happen and
> > thus bugs. If we fix cases like this to return a real error code, then
> > anything that comes up as XX000 is likely to be a real bug, whereas if
> > we don't, the things that we're not concerned about have to be
> > filtered out by some other method, probably involving a human being.
> > If the filter that human being has to apply further involves reading
> > Tom Lane's mind and knowing what he will think about a particular
> > report, or alternatively asking him, it just makes complicated
> > something that we could have made simple.
>
> FWIW, here are some other cases from sqlsmith which hit elog()/XX000:
>
> postgres=# select unknownin('');
> ERROR: failed to find conversion function from unknown to text
> postgres=# \errverbose
> ERROR: XX000: failed to find conversion function from unknown to text
> LOCATION: coerce_type, parse_coerce.c:542
>
> postgres=# SELECT pg_catalog.interval( '12 seconds'::interval ,3);
> ERROR: unrecognized interval typmod: 3
>
> postgres=# SELECT pg_describe_object(1,0,1);
> ERROR: invalid non-zero objectSubId for object class 1
>
> postgres=# SELECT pg_read_file( repeat('a',333));
> ERROR: could not open file
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
forreading: File name too long
postgres=# SELECT acldefault('a',0);
ERROR: unrecognized object type abbreviation: a
postgres=# SELECT setweight('a','1');
ERROR: unrecognized weight: 49
regression=# select float8_regr_intercept(ARRAY[1]) ;
ERROR: float8_regr_intercept: expected 6-element float8 array
None of these is new in v16.