Re: Error-safe user functions

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Dec 16, 2022 at 1:31 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The reg* functions probably need a unified plan as to how far
>> down we want to push non-error behavior.

> I would be in favor of an aggressive approach.

Here's a proposed patch for converting regprocin and friends
to soft error reporting. I'll say at the outset that it's an
engineering compromise, and it may be worth going further in
future. But I doubt it's worth doing more than this for v16,
because the next steps would be pretty invasive.

I've converted all the errors thrown directly within regproc.c,
and also converted parseTypeString, typeStringToTypeName, and
stringToQualifiedNameList to report their own errors softly.
This affected some outside callers, but not so many of them
that I think it's worth inventing compatibility wrappers.

I dealt with lookup failures by just changing the input functions
to call the respective lookup functions with missing_ok = true,
and then throw their own error softly on failure.

Also, I've changed to_regproc() and friends to return NULL
in exactly the same cases that are now soft errors for the
input functions. Previously they were a bit inconsistent
about what triggered hard errors vs. returning NULL.
(Perhaps we should go further than this, and convert all these
functions to just be DirectInputFunctionCallSafe wrappers
around the corresponding input functions? That would save
some duplicative code, but I've not done it here.)

What's not fixed here:

1. As previously discussed, parse errors in type names are
thrown by the main grammar, so getting those to not be
hard errors seems like too big a lift for today.

2. Errors about invalid type modifiers (reported by
typenameTypeMod or type-specific typmodin routines) are not
trapped either. Fixing this would require extending the
soft-error conventions to typmodin routines, which maybe will
be worth doing someday but it seems pretty far down the
priority list. Specifying a typmod is surely not main-line
usage for regtypein.

3. Throwing our own error has the demerit that it might be
different from what the underlying lookup function would have
reported. This is visible in some changes in existing
regression test cases, such as

-ERROR: schema "ng_catalog" does not exist
+ERROR: relation "ng_catalog.pg_class" does not exist

This isn't wrong, exactly, but the loss of specificity is
a bit annoying.

4. This still fails to trap errors about "too many dotted names"
and "cross-database references are not implemented", which are
thrown in DeconstructQualifiedName, LookupTypeName,
RangeVarGetRelid, and maybe some other places.

5. We also don't trap errors about "the schema exists but
you don't have USAGE permission to do a lookup in it",
because LookupExplicitNamespace still throws that even
when passed missing_ok = true.

The obvious way to fix #3,#4,#5 is to change pretty much all
of the catalog lookup infrastructure to deal in escontext
arguments instead of "missing_ok" booleans. That might be
worth doing --- it'd have benefits beyond the immediate
problem, I think --- but I feel it's a bigger lift than we
want to undertake for v16. It'd be better to spend the time
we have left for v16 on building features that use soft error
reporting than on refining corner cases in the reg* functions.

So I think we should stop more or less here, possibly after
changing the to_regfoo functions to be simple wrappers
around the soft input functions.

regards, tom lane