Error-safe user functions

Sorry, I didn't not tried building using meson.
One line was fixed in the new test module's meson.build.

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

Nikita Glukhov <n.gluhov@postgrespro.ru> writes:

On 30.09.2022, Tom Lane wrote:

I strongly recommend against having a new pg_proc column at all.

I think the only way to avoid catalog modification (adding new columns
to pg_proc or pg_type, introducing new function signatures etc.) and
to avoid adding some additional code to the entry of error-safe
functions is to bump version of our calling convention. I simply added
flag Pg_finfo_record.errorsafe which is set to true when the new
PG_FUNCTION_INFO_V2_ERRORSAFE() macro is used.

I don't think you got my point at all.

I do not think we need a new pg_proc column (btw, touching pg_proc.dat
is morally equivalent to a pg_proc column), and I do not think we need
a new call-convention version either, because I think that this sort
of thing:

+        /* check whether input function supports returning errors */
+        if (cstate->opts.null_on_error_flags[attnum - 1] &&
+            !func_is_error_safe(in_func_oid))
+            ereport(ERROR,
+                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                     errmsg("input function for datatype \"%s\" does not support error handling",
+                            format_type_be(att->atttypid))));

is useless. It does not benefit anybody to pre-emptively throw an error
because you are afraid that some other code might throw an error later.
That just converts "might fail" to "guaranteed to fail" --- how is that
better?

I think what we want is to document options like NULL_ON_ERROR along the
lines of

If the input data in one of the specified columns is invalid,
set the column's value to NULL instead of reporting an error.
This feature will only work if the column datatype's input
function has been upgraded to support it; otherwise, an invalid
input value will result in an error anyway.

and just leave it on the heads of extension authors to get their
code moved forward. (If we fail to get all the core types converted
by the time v16 reaches feature freeze, we'd have to add some docs
about which ones support this; but maybe we will get all that done
and not need documentation effort.)

Some other recommendations:

* The primary work-product from an initial patch of this sort is an
API specification. Therefore, your 0001 patch ought to be introducing
some prose documentation somewhere (and I don't mean comments in elog.h,
rather a README file or even the SGML docs --- utils/fmgr/README might
be a good spot). Getting that text right so that people understand
what to do is more important than any single code detail. You are not
winning any fans by not bothering with code comments such as per-function
header comments, either.

* Submitting 16-part patch series is a good way to discourage people
from reviewing your work. I'd toss most of the datatype conversions
overboard for the moment, planning to address them later once the core
patch is committed. The initial patchset only needs to have one or two
data types done as proof-of-concept.

* I'd toss the test module overboard too. Once you've got COPY using
the feature, that's a perfectly good testbed. The effort spent on
the test module would have been better spent on making the COPY support
more complete (ie, get rid of the silly restriction to CSV).

* The 0015 and 0016 patches don't seem to belong here either. It's
impossible to review these when the code is neither commented nor
connected to any use-case.

* I think that the ereturn macro is the right idea, but I don't understand
the rationale for also inventing PG_RETURN_ERROR. Also, ereturn's
implementation isn't great --- I don't like duplicating the __VA_ARGS__
text, because that will lead to substantial code bloat. It'd likely
work better to make ereturn very much like ereport, except with a
different finishing function that contains the throw-or-not logic.
As a small nitpick, I think I'd make ereturn's argument order be return
value then edata then ...; it just seems more sensible that way.

* execnodes.h seems like an *extremely* random place to put struct
FuncCallError; that will force inclusion of execnodes.h in many places
that did not need it before. Possibly fmgr.h is the right place for it?
In general you need to think about avoiding major inclusion bloat
(and I wonder whether the patchset passes cpluspluscheck). It might
help to treat ereturn's edata argument as just "void *" and confine
references to the FuncCallError struct to the errfinish-replacement
subroutine, ie drop the tests in PG_GET_ERROR_PTR and do that check
inside elog.c.

* I wonder if there's a way to avoid the CopyErrorData and FreeErrorData
steps in use-cases like this --- that's pure overhead, really, for
COPY's purposes, and it's probably not the only use-case that will
think so. Maybe we could complicate FuncCallError a little and pass
a flag that indicates that we only want to know whether an error
occurred, not what it was exactly. On the other hand, if you assume
that errors should be rare, maybe that's useless micro-optimization.

Basically, this patch set should be a lot smaller and not have ambitions
beyond "get the API right" and "make one or two datatypes support COPY
NULL_ON_ERROR". Add more code once that core functionality gets reviewed
and committed.

regards, tom lane

The idea is simple -- introduce new "error-safe" calling mode of user
functions by passing special node through FunctCallInfo.context, in
which function should write error info and return instead of throwing
it. Also such functions should manually free resources before
returning an error. This gives ability to avoid PG_TRY/PG_CATCH and
subtransactions.

I tried something similar when trying to implement TRY_CAST (

https://learn.microsoft.com/en-us/sql/t-sql/functions/try-cast-transact-sql?view=sql-server-ver16)
late last year. I also considered having a default datum rather than just
returning NULL.

I had not considered a new node type. I had considered having every
function have a "safe" version, which would be a big duplication of logic
requiring a lot of regression tests and possibly fuzzing tests.

Instead, I extended every core input function to have an extra boolean
parameter to indicate if failures were allowed, and then an extra Datum
parameter for the default value. The Input function wouldn't need to check
the value of the new parameters until it was already in a situation where
it found invalid data, but the extra overhead still remained, and it meant
that basically every third party type extension would need to be changed.

Then I considered whether the cast failure should be completely silent, or
if the previous error message should instead be omitted as a LOG/INFO/WARN,
and if we'd want that to be configurable, so then the boolean parameter
became an integer enum:

* regular fail (0)
* use default silently (1)
* use default emit LOG/NOTICE/WARNING (2,3,4)

At the time, all of this seemed like too big of a change for a function
that isn't even in the SQL Standard, but maybe SQL/JSON changes that.

If so, it would allow for a can-cast-to test that users would find very
useful. Something like:

SELECT CASE WHEN 'abc' CAN BE integer THEN 'Integer' ELSE 'Nope' END

There's obviously no standard syntax to support that, but the data
cleansing possibilities would be great.

On 2022-10-07 Fr 13:37, Tom Lane wrote:

[ lots of detailed review ]

Basically, this patch set should be a lot smaller and not have ambitions
beyond "get the API right" and "make one or two datatypes support COPY
NULL_ON_ERROR". Add more code once that core functionality gets reviewed
and committed.

Nikita,

just checking in, are you making progress on this? I think we really
need to get this reviewed and committed ASAP if we are to have a chance
to get the SQL/JSON stuff reworked to use it in time for release 16.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Tue, Nov 15, 2022 at 11:36 AM Andrew Dunstan <andrew@dunslane.net> wrote:

On 2022-10-07 Fr 13:37, Tom Lane wrote:

[ lots of detailed review ]

Basically, this patch set should be a lot smaller and not have ambitions
beyond "get the API right" and "make one or two datatypes support COPY
NULL_ON_ERROR". Add more code once that core functionality gets reviewed
and committed.

Nikita,

just checking in, are you making progress on this? I think we really
need to get this reviewed and committed ASAP if we are to have a chance
to get the SQL/JSON stuff reworked to use it in time for release 16.

I'm making an attempt at this or something very similar to it. I don't yet
have a patch ready.

Corey Huinker <corey.huinker@gmail.com> writes:

On Tue, Nov 15, 2022 at 11:36 AM Andrew Dunstan <andrew@dunslane.net> wrote:

Nikita,
just checking in, are you making progress on this? I think we really
need to get this reviewed and committed ASAP if we are to have a chance
to get the SQL/JSON stuff reworked to use it in time for release 16.

I'm making an attempt at this or something very similar to it. I don't yet
have a patch ready.

Cool. We can't delay too much longer on this if we want to have
a credible feature in v16. Although I want a minimal initial
patch, there will still be a ton of incremental work to do after
the core capability is reviewed and committed, so there's no
time to lose.

regards, tom lane

On Mon, Nov 21, 2022 at 12:26:45AM -0500, Tom Lane wrote:

Corey Huinker <corey.huinker@gmail.com> writes:

I'm making an attempt at this or something very similar to it. I don't yet
have a patch ready.

Nice to hear that. If a WIP or a proof of concept takes more than a
few hours, how about beginning a new thread with the ideas you have in
mind so as we could agree about how to shape this error-to-default
conversion facility on data input?

Cool. We can't delay too much longer on this if we want to have
a credible feature in v16. Although I want a minimal initial
patch, there will still be a ton of incremental work to do after
the core capability is reviewed and committed, so there's no
time to lose.

I was glancing at the patch of upthread and the introduction of a v2
scared me, so I've stopped at this point.
--
Michael

On 2022-11-21 Mo 00:26, Tom Lane wrote:

Corey Huinker <corey.huinker@gmail.com> writes:

On Tue, Nov 15, 2022 at 11:36 AM Andrew Dunstan <andrew@dunslane.net> wrote:

Nikita,
just checking in, are you making progress on this? I think we really
need to get this reviewed and committed ASAP if we are to have a chance
to get the SQL/JSON stuff reworked to use it in time for release 16.

I'm making an attempt at this or something very similar to it. I don't yet
have a patch ready.

Cool. We can't delay too much longer on this if we want to have
a credible feature in v16. Although I want a minimal initial
patch, there will still be a ton of incremental work to do after
the core capability is reviewed and committed, so there's no
time to lose.

OK, here's a set of minimal patches based on Nikita's earlier work and
also some work by my colleague Amul Sul. It tries to follow Tom's
original outline at [1]/messages/by-id/13351.1661965592@sss.pgh.pa.us, and do as little else as possible.

Patch 1 introduces the IOCallContext node. The caller should set the
no_error_throw flag and clear the error_found flag, which will be set by
a conforming IO function if an error is found. It also includes a string
field for an error message. I haven't used that, it's more there to
stimulate discussion. Robert suggested to me that maybe it should be an
ErrorData, but I'm not sure how we would use it.

Patch 2 introduces InputFunctionCallContext(), which is similar to
InputFunctionCall() but with an extra context parameter. Note that it's
ok for an input function to return a NULL to this function if an error
is found.

Patches 3 and 4 modify the bool_in() and int4in() functions respectively
to handle an IOContextCall appropriately if provided one in their
fcinfo.context.

Patch 5 introduces COPY FROM ... NULL_ON_ERROR which, in addition to
being useful in itself, provides a test of the previous patches.

I hope we can get a fairly quick agreement so that work can being on
adjusting at least those things needed for the SQL/JSON patches ASAP.
Our goal should be to adjust all the core input functions, but that's
not quite so urgent, and can be completed in parallel with the SQL/JSON
work.

cheers

andrew

[1]: /messages/by-id/13351.1661965592@sss.pgh.pa.us

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

OK, here's a set of minimal patches based on Nikita's earlier work and
also some work by my colleague Amul Sul. It tries to follow Tom's
original outline at [1], and do as little else as possible.

This is not really close at all to what I had in mind.

The main objection is that we shouldn't be passing back a "char *"
error string (though I observe that your 0003 and 0004 patches aren't
even bothering to do that much). I think we want to pass back a
fully populated ErrorData struct so that we can report everything
the actual error would have done (notably, the SQLSTATE). That means
that elog.h/.c has to be intimately involved in this. I liked Nikita's
overall idea of introducing an "ereturn" macro, with the idea that
where we have, say,

ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("value \"%s\" is out of range for type %s",
s, "integer")));

we would write

ereturn(context, ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("value \"%s\" is out of range for type %s",
s, "integer")));
return NULL; // or whatever is appropriate

and the involvement with the contents of the context node would
all be confined to some new code in elog.c. That would help
prevent the #include-footprint-bloat that is otherwise going to
ensue.

(Maybe we could assume that ereturn's elevel must be ERROR, and
save a little notation. I'm not very wedded to "ereturn" as the
new macro name either, though it's not awful.)

Also, as I said before, the absolute first priority has to be
documentation explaining what function authors are supposed to
do differently than before.

I'd be willing to have a go at this myself, except that Corey
said he was working on it, so I don't want to step on his toes.

regards, tom lane

On Thu, Dec 1, 2022 at 1:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

The main objection is that we shouldn't be passing back a "char *"
error string (though I observe that your 0003 and 0004 patches aren't
even bothering to do that much). I think we want to pass back a
fully populated ErrorData struct so that we can report everything
the actual error would have done (notably, the SQLSTATE).

+1.

That means that elog.h/.c has to be intimately involved in this.
I liked Nikita's
overall idea of introducing an "ereturn" macro, with the idea that
where we have, say,

ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("value \"%s\" is out of range for type %s",
s, "integer")));

we would write

ereturn(context, ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("value \"%s\" is out of range for type %s",
s, "integer")));
return NULL; // or whatever is appropriate

It sounds like you're imagining that ereturn doesn't return, which
seems confusing. But I don't know that I'd like it better if it did.
Magic return statements hidden inside macros seem not too fun. What
I'd like to see is a macro that takes a pointer to an ErrorData and
the rest of the arguments like ereport() and stuffs everything in
there. And then you can pass that to ThrowErrorData() later if you
like. That way it's visible when you're using the macro where you're
putting the error. I think that would make the code more readable.

Also, as I said before, the absolute first priority has to be
documentation explaining what function authors are supposed to
do differently than before.

+1.

I'd be willing to have a go at this myself, except that Corey
said he was working on it, so I don't want to step on his toes.

Time is short, and I do not think Corey will be too sad if you decide
to have a go at it. The chances of person A being able to write the
code person B is imagining as well as person B could write it are not
great, regardless of who A and B are. And I think the general
consensus around here is that you're a better coder than most.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

It sounds like you're imagining that ereturn doesn't return, which
seems confusing. But I don't know that I'd like it better if it did.

The spec I had in mind was that it would behave as ereport(ERROR)
unless a suitable FuncErrorContext node is passed, in which case
it'd store the error data into that node and return. This leaves
the invoker with only the job of passing control back afterwards,
if it gets control back. I'd be the first to agree that "ereturn"
doesn't capture that detail very well, but I don't have a better name.
(And I do like the fact that this name is the same length as "ereport",
so that we won't end up with lots of reindentation to do.)

Magic return statements hidden inside macros seem not too fun. What
I'd like to see is a macro that takes a pointer to an ErrorData and
the rest of the arguments like ereport() and stuffs everything in
there. And then you can pass that to ThrowErrorData() later if you
like. That way it's visible when you're using the macro where you're
putting the error. I think that would make the code more readable.

I think that'd just complicate the places that are having to report
such errors --- of which there are likely to be hundreds by the time
we are done. I will not accept a solution that requires more than
the absolute minimum of additions to the error-reporting spots.

regards, tom lane

On Thu, Dec 1, 2022 at 2:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

It sounds like you're imagining that ereturn doesn't return, which
seems confusing. But I don't know that I'd like it better if it did.

The spec I had in mind was that it would behave as ereport(ERROR)
unless a suitable FuncErrorContext node is passed, in which case
it'd store the error data into that node and return. This leaves
the invoker with only the job of passing control back afterwards,
if it gets control back. I'd be the first to agree that "ereturn"
doesn't capture that detail very well, but I don't have a better name.
(And I do like the fact that this name is the same length as "ereport",
so that we won't end up with lots of reindentation to do.)

I don't think it's sensible to make decisions about important syntax
on the basis of byte-length. Reindenting is a one-time effort; code
clarity will be with us forever.

Magic return statements hidden inside macros seem not too fun. What
I'd like to see is a macro that takes a pointer to an ErrorData and
the rest of the arguments like ereport() and stuffs everything in
there. And then you can pass that to ThrowErrorData() later if you
like. That way it's visible when you're using the macro where you're
putting the error. I think that would make the code more readable.

I think that'd just complicate the places that are having to report
such errors --- of which there are likely to be hundreds by the time
we are done.

OK, that's a fair point.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Dec 1, 2022 at 2:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'd be the first to agree that "ereturn"
doesn't capture that detail very well, but I don't have a better name.
(And I do like the fact that this name is the same length as "ereport",
so that we won't end up with lots of reindentation to do.)

I don't think it's sensible to make decisions about important syntax
on the basis of byte-length. Reindenting is a one-time effort; code
clarity will be with us forever.

Sure, but without a proposal for a better name, that's irrelevant.

regards, tom lane

On Thu, Dec 1, 2022 at 3:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I don't think it's sensible to make decisions about important syntax
on the basis of byte-length. Reindenting is a one-time effort; code
clarity will be with us forever.

Sure, but without a proposal for a better name, that's irrelevant.

Sure, but you're far too clever not to be able to come up with
something good without any help from me. io_error_return_or_throw()?
store_or_report_io_error()? Or just store_io_error()?

It sounds to me like we're crafting something that is specific to and
can only be used with type input and output functions, so the name
probably should reflect that rather than being something totally
generic like ereturn() or error_stash() or whatever. If we were making
this into a general-purpose way of sticking an error someplace, then a
name like that would make sense and this would be an extension of the
elog.c interface. But what you're proposing is a great deal more
specialized than that.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

It sounds to me like we're crafting something that is specific to and
can only be used with type input and output functions, so the name
probably should reflect that rather than being something totally
generic like ereturn() or error_stash() or whatever.

My opinion is exactly the opposite. Don't we already have a need
for error-safe type conversions, too, in the JSON stuff? Even if
I couldn't point to a need-it-now requirement, I think we will
eventually find a use for this with some other classes of functions.

If we were making
this into a general-purpose way of sticking an error someplace, then a
name like that would make sense and this would be an extension of the
elog.c interface. But what you're proposing is a great deal more
specialized than that.

I'm proposing *exactly* an extension of the elog.c interface;
so were you, a couple messages back. It's only specialized to I/O
in the sense that our current need is for that.

regards, tom lane

On Thu, Dec 1, 2022 at 5:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

It sounds to me like we're crafting something that is specific to and
can only be used with type input and output functions, so the name
probably should reflect that rather than being something totally
generic like ereturn() or error_stash() or whatever.

My opinion is exactly the opposite. Don't we already have a need
for error-safe type conversions, too, in the JSON stuff? Even if
I couldn't point to a need-it-now requirement, I think we will
eventually find a use for this with some other classes of functions.

<sputters>

But you yourself proposed a new node called IOCallContext. It can't be
right to have the names be specific to I/O functions in one part of
the patch and totally generic in another part.

Hmm, but yesterday I see that you were now calling it FuncCallContext.

I think the design is evolving in your head as you think about this
more, which is totally understandable and actually very good. However,
this is also why I think that you should produce the patch you
actually want instead of letting other people repeatedly submit
patches and then complain that they weren't what you had in mind.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

I think the design is evolving in your head as you think about this
more, which is totally understandable and actually very good. However,
this is also why I think that you should produce the patch you
actually want instead of letting other people repeatedly submit
patches and then complain that they weren't what you had in mind.

OK, Corey hasn't said anything, so I will have a look at this over
the weekend.

regards, tom lane

On 2022-12-02 Fr 09:12, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I think the design is evolving in your head as you think about this
more, which is totally understandable and actually very good. However,
this is also why I think that you should produce the patch you
actually want instead of letting other people repeatedly submit
patches and then complain that they weren't what you had in mind.

OK, Corey hasn't said anything, so I will have a look at this over
the weekend.

Great. Let's hope we can get this settled early next week and then we
can get to work on the next tranche of functions, those that will let
the SQL/JSON work restart.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Fri, Dec 2, 2022 at 9:12 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I think the design is evolving in your head as you think about this
more, which is totally understandable and actually very good. However,
this is also why I think that you should produce the patch you
actually want instead of letting other people repeatedly submit
patches and then complain that they weren't what you had in mind.

OK, Corey hasn't said anything, so I will have a look at this over
the weekend.

regards, tom lane

Sorry, had several life issues intervene. Glancing over what was discussed
because it seems pretty similar to what I had in mind. Will respond back in
detail shortly.