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.

On Fri, Dec 2, 2022 at 9:34 AM Andrew Dunstan <andrew@dunslane.net> wrote:

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

I'm still working on organizing my patch, but it grew out of a desire to do
this:

CAST(value AS TypeName DEFAULT expr)

This is a thing that exists in other forms in other databases and while it
may look unrelated, it is essentially the SQL/JSON casts within a nested
data structure issue, just a lot simpler.

My original plan had been two new params to all _in() functions: a boolean
error_mode and a default expression Datum.

After consulting with Jeff Davis and Michael Paquier, the notion of
modifying fcinfo itself two booleans:
allow_error (this call is allowed to return if there was an error with
INPUT) and
has_error (this function has the concept of a purely-input-based error,
and found one)

The nice part about this is that unaware functions can ignore these values,
and custom data types that did not check these values would continue to
work as before. It wouldn't respect the CAST default, but that's up to the
extension writer to fix, and that's a pretty acceptable failure mode.

Where this gets tricky is arrays and complex types: the default expression
applies only to the object explicitly casted, so if somebody tried CAST
('{"123","abc"}'::text[] AS integer[] DEFAULT '{0}') the inner casts need
to know that they _can_ allow input errors, but have no default to offer,
they need merely report their failure upstream...and that's where the
issues align with the SQL/JSON issue.

In pursuing this, I see that my method was simultaneously too little and
too much. Too much in the sense that it alters the structure for all fmgr
functions, though in a very minor and back-compatible way, and too little
in the sense that we could actually return the ereport info in a structure
and leave it to the node to decide whether to raise it or not. Though I
should add that there many situations where we don't care about the
specifics of the error, we just want to know that one existed and move on,
so time spent forming that return structure would be time wasted.

The one gap I see so far in the patch presented is that it returns a null
value on bad input, which might be ok if the node has the default, but that
then presents the node with having to understand whether it was a null
because of bad input vs a null that was expected.

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

I'm still working on organizing my patch, but it grew out of a desire to do
this:
CAST(value AS TypeName DEFAULT expr)
This is a thing that exists in other forms in other databases and while it
may look unrelated, it is essentially the SQL/JSON casts within a nested
data structure issue, just a lot simpler.

Okay, maybe that's why I was thinking we had a requirement for
failure-free casts. Sure, you can transform it to the other thing
by always implementing this as a cast-via-IO, but you could run into
semantics issues that way. (If memory serves, we already have cases
where casting X to Y gives a different result from casting X to text
to Y.)

My original plan had been two new params to all _in() functions: a boolean
error_mode and a default expression Datum.
After consulting with Jeff Davis and Michael Paquier, the notion of
modifying fcinfo itself two booleans:
allow_error (this call is allowed to return if there was an error with
INPUT) and
has_error (this function has the concept of a purely-input-based error,
and found one)

Hmm ... my main complaint about that is the lack of any way to report
the details of the error. I realize that a plain boolean failure flag
might be enough for our immediate use-cases, but I don't want to expend
the amount of effort this is going to involve and then later find we
have a use-case where we need the error details.

The sketch that's in my head at the moment is to make use of the existing
"context" field of FunctionCallInfo: if that contains a node of some
to-be-named type, then we are requesting that errors not be thrown
but instead be reported by passing back an ErrorData using a field of
that node. The issue about not constructing an ErrorData if the outer
caller doesn't need it could perhaps be addressed by adding some boolean
flag fields in that node, but the details of that need not be known to
the functions reporting errors this way; it'd be a side channel from the
outer caller to elog.c.

The main objection I can see to this approach is that we only support
one context value per call, so you could not easily combine this
functionality with existing use-cases for the context field. A quick
census of InitFunctionCallInfoData calls finds aggregates, window
functions, triggers, and procedures, none of which seem like plausible
candidates for wanting no-error behavior, so I'm not too concerned
about that. (Maybe the error-reporting node could be made a sub-node
of the context node in any future cases where we do need it?)

The one gap I see so far in the patch presented is that it returns a null
value on bad input, which might be ok if the node has the default, but that
then presents the node with having to understand whether it was a null
because of bad input vs a null that was expected.

Yeah. That's something we could probably get away with for the case of
input functions only, but I think explicit out-of-band signaling that
there was an error is a more future-proof solution.

regards, tom lane

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

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

I'm still working on organizing my patch, but it grew out of a desire to

do

this:
CAST(value AS TypeName DEFAULT expr)
This is a thing that exists in other forms in other databases and while

it

may look unrelated, it is essentially the SQL/JSON casts within a nested
data structure issue, just a lot simpler.

Okay, maybe that's why I was thinking we had a requirement for
failure-free casts. Sure, you can transform it to the other thing
by always implementing this as a cast-via-IO, but you could run into
semantics issues that way. (If memory serves, we already have cases
where casting X to Y gives a different result from casting X to text
to Y.)

Yes, I was setting aside the issue of direct cast functions for my v0.1

My original plan had been two new params to all _in() functions: a

boolean

error_mode and a default expression Datum.
After consulting with Jeff Davis and Michael Paquier, the notion of
modifying fcinfo itself two booleans:
allow_error (this call is allowed to return if there was an error with
INPUT) and
has_error (this function has the concept of a purely-input-based error,
and found one)

Hmm ... my main complaint about that is the lack of any way to report
the details of the error. I realize that a plain boolean failure flag
might be enough for our immediate use-cases, but I don't want to expend
the amount of effort this is going to involve and then later find we
have a use-case where we need the error details.

I agree, but then we're past a boolean for allow_error, and we probably get
into a list of modes like this:

CAST_ERROR_ERROR /* default ereport(), what we do now */
CAST_ERROR_REPORT_FULL /* report that the cast failed, everything that you
would have put in the ereport() instead put in a struct that gets returned
to caller */
CAST_ERROR_REPORT_SILENT /* report that the cast failed, but nobody cares
why so don't even form the ereport strings, good for bulk operations */
CAST_ERROR_WARNING /* report that the cast failed, but emit ereport() as a
warning */
CAST_ERROR_[NOTICE,LOG,DEBUG1,..DEBUG5] /* same, but some other loglevel */

The sketch that's in my head at the moment is to make use of the existing
"context" field of FunctionCallInfo: if that contains a node of some
to-be-named type, then we are requesting that errors not be thrown
but instead be reported by passing back an ErrorData using a field of
that node. The issue about not constructing an ErrorData if the outer
caller doesn't need it could perhaps be addressed by adding some boolean
flag fields in that node, but the details of that need not be known to
the functions reporting errors this way; it'd be a side channel from the
outer caller to elog.c.

That should be a good place for it, assuming it's not already used like
fn_extra is. It would also squash those cases above into just three: ERROR,
REPORT_FULL, and REPORT_SILENT, leaving it up to the node what type of
erroring/logging is appropriate.

The main objection I can see to this approach is that we only support
one context value per call, so you could not easily combine this
functionality with existing use-cases for the context field. A quick
census of InitFunctionCallInfoData calls finds aggregates, window
functions, triggers, and procedures, none of which seem like plausible
candidates for wanting no-error behavior, so I'm not too concerned
about that. (Maybe the error-reporting node could be made a sub-node
of the context node in any future cases where we do need it?)

A subnode had occurred to me when fiddling about with fn_extra, so so that
applies here, but if we're doing a sub-node, then maybe it's worth it's own
parameter. I struggled with that because of how few of these functions will
use it vs how often they're executed.

The one gap I see so far in the patch presented is that it returns a null
value on bad input, which might be ok if the node has the default, but

that

then presents the node with having to understand whether it was a null
because of bad input vs a null that was expected.

Yeah. That's something we could probably get away with for the case of
input functions only, but I think explicit out-of-band signaling that
there was an error is a more future-proof solution.

I think we'll run into it fairly soon, because if I recall correctly,
SQL/JSON has a formatting spec that essentially means that we're not
calling input functions, we're calling TO_CHAR() and TO_DATE(), but they're
very similiar to input functions.

One positive side effect of all this is we can get a isa(value, typname)
construct like this "for free", we just try the cast and return the value.

I'm still working on my patch even though it will probably be sidelined in
the hopes that it informs us of any subsequent issues.

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

The main objection I can see to this approach is that we only support
one context value per call, so you could not easily combine this
functionality with existing use-cases for the context field. A quick
census of InitFunctionCallInfoData calls finds aggregates, window
functions, triggers, and procedures, none of which seem like plausible
candidates for wanting no-error behavior, so I'm not too concerned
about that. (Maybe the error-reporting node could be made a sub-node
of the context node in any future cases where we do need it?)

I kind of wonder why we don't just add another member to FmgrInfo.
It's 48 bytes right now and this would increase the size to 56 bytes,
so it's not as if we're increasing the number of cache lines or even
using up all of the remaining byte space. It's an API break, but
people have to recompile for new major versions anyway, so I guess I
don't really see the downside.

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

Robert Haas <robertmhaas@gmail.com> writes:

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

The main objection I can see to this approach is that we only support
one context value per call, so you could not easily combine this
functionality with existing use-cases for the context field.

I kind of wonder why we don't just add another member to FmgrInfo.
It's 48 bytes right now and this would increase the size to 56 bytes,

This'd be FunctionCallInfoData not FmgrInfo.

I'm not terribly concerned about the size of FunctionCallInfoData,
but I am concerned about the number of cycles spent to initialize it,
because we do that pretty durn often. So I don't really want to add
fields to it without compelling use-cases, and I don't see one here.

regards, tom lane

Andrew Dunstan <andrew@dunslane.net> writes:

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.

OK, here's a draft proposal. I should start out by acknowledging that
this steals a great deal from Nikita's original patch as well as yours,
though I editorialized heavily.

0001 is the core infrastructure and documentation for the feature.
(I didn't bother breaking it down further than that.)

0002 fixes boolin and int4in. That is the work that we're going to
have to replicate in an awful lot of places, and I am pleased by how
short-and-sweet it is. Of course, stuff like the datetime functions
might be more complex to adapt.

Then 0003 is a quick-hack version of COPY that is able to exercise
all this. I did not bother with the per-column flags as you had
them, because I'm not sure if they're worth the trouble compared
to a simple boolean; in any case we can add that refinement later.
What I did add was a WARN_ON_ERROR option that exercises the ability
to extract the error message after a soft error. I'm not proposing
that as a shippable feature, it's just something for testing.

I think there are just a couple of loose ends here:

1. Bikeshedding on my name choices is welcome. I know Robert is
dissatisfied with "ereturn", but I'm content with that so I didn't
change it here.

2. Everybody has struggled with just where to put the declaration
of the error context structure. The most natural home for it
probably would be elog.h, but that's out because it cannot depend
on nodes.h, and the struct has to be a Node type to conform to
the fmgr safety guidelines. What I've done here is to drop it
in nodes.h, as we've done with a couple of other hard-to-classify
node types; but I can't say I'm satisfied with that.

Other plausible answers seem to be:

* Drop it in fmgr.h. The only real problem is that historically
we've not wanted fmgr.h to depend on nodes.h either. But I'm not
sure how strong the argument for that really is/was. If we did
do it like that we could clean up a few kluges, both in this patch
and pre-existing (fmNodePtr at least could go away).

* Invent a whole new header just for this struct. But then we're
back to the question of what to call it. Maybe something along the
lines of utils/elog_extras.h ?

regards, tom lane

I think there are just a couple of loose ends here:

1. Bikeshedding on my name choices is welcome. I know Robert is
dissatisfied with "ereturn", but I'm content with that so I didn't
change it here.

1. details_please => include_error_data

as this hints the reader directly to the struct to be filled out

2. ereturn_* => errfeedback / error_feedback / efeedback

It is returned, but it's not taking control and the caller could ignore it.
I arrived at his after checking https://www.thesaurus.com/browse/report and
https://www.thesaurus.com/browse/hint.

2. Everybody has struggled with just where to put the declaration
of the error context structure. The most natural home for it
probably would be elog.h, but that's out because it cannot depend
on nodes.h, and the struct has to be a Node type to conform to
the fmgr safety guidelines. What I've done here is to drop it
in nodes.h, as we've done with a couple of other hard-to-classify
node types; but I can't say I'm satisfied with that.

Other plausible answers seem to be:

* Drop it in fmgr.h. The only real problem is that historically
we've not wanted fmgr.h to depend on nodes.h either. But I'm not
sure how strong the argument for that really is/was. If we did
do it like that we could clean up a few kluges, both in this patch
and pre-existing (fmNodePtr at least could go away).

* Invent a whole new header just for this struct. But then we're
back to the question of what to call it. Maybe something along the
lines of utils/elog_extras.h ?

Would moving ErrorReturnContext and the ErrorData struct to their own
util/errordata.h allow us to avoid the void pointer for ercontext? If so,
that'd be a win because typed pointers give the reader some idea of what is
expected there, as well as aiding doxygen-like tools.

Overall this looks like a good foundation.

My own effort was getting bogged down in the number of changes I needed to
make in code paths that would never want a failover case for their
typecasts, so I'm going to refactor my work on top of this and see where I
get stuck.

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

My own effort was getting bogged down in the number of changes I needed to
make in code paths that would never want a failover case for their
typecasts, so I'm going to refactor my work on top of this and see where I
get stuck.

+1, that would be a good way to see if I missed anything.

regards, tom lane

On 2022-12-03 Sa 16:46, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

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.

OK, here's a draft proposal. I should start out by acknowledging that
this steals a great deal from Nikita's original patch as well as yours,
though I editorialized heavily.

0001 is the core infrastructure and documentation for the feature.
(I didn't bother breaking it down further than that.)

0002 fixes boolin and int4in. That is the work that we're going to
have to replicate in an awful lot of places, and I am pleased by how
short-and-sweet it is. Of course, stuff like the datetime functions
might be more complex to adapt.

Then 0003 is a quick-hack version of COPY that is able to exercise
all this. I did not bother with the per-column flags as you had
them, because I'm not sure if they're worth the trouble compared
to a simple boolean; in any case we can add that refinement later.
What I did add was a WARN_ON_ERROR option that exercises the ability
to extract the error message after a soft error. I'm not proposing
that as a shippable feature, it's just something for testing.

Overall I think this is pretty good, and I hope we can settle on it quickly.

I think there are just a couple of loose ends here:

1. Bikeshedding on my name choices is welcome. I know Robert is
dissatisfied with "ereturn", but I'm content with that so I didn't
change it here.

I haven't got anything better than ereturn.

details_please seems more informal than our usual style. details_wanted
maybe?

2. Everybody has struggled with just where to put the declaration
of the error context structure. The most natural home for it
probably would be elog.h, but that's out because it cannot depend
on nodes.h, and the struct has to be a Node type to conform to
the fmgr safety guidelines. What I've done here is to drop it
in nodes.h, as we've done with a couple of other hard-to-classify
node types; but I can't say I'm satisfied with that.

Other plausible answers seem to be:

* Drop it in fmgr.h. The only real problem is that historically
we've not wanted fmgr.h to depend on nodes.h either. But I'm not
sure how strong the argument for that really is/was. If we did
do it like that we could clean up a few kluges, both in this patch
and pre-existing (fmNodePtr at least could go away).

* Invent a whole new header just for this struct. But then we're
back to the question of what to call it. Maybe something along the
lines of utils/elog_extras.h ?

Maybe a new header misc_nodes.h?

Soon after we get this done I think we'll find we need to extend this to
non-input functions. But that can wait a short while.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-03 Sa 16:46, Tom Lane wrote:

1. Bikeshedding on my name choices is welcome. I know Robert is
dissatisfied with "ereturn", but I'm content with that so I didn't
change it here.

details_please seems more informal than our usual style. details_wanted
maybe?

Yeah, Corey didn't like that either. "details_wanted" works for me.

Soon after we get this done I think we'll find we need to extend this to
non-input functions. But that can wait a short while.

I'm curious to know exactly which other use-cases you foresee.
It wouldn't be a bad idea to write some draft code to verify
that this mechanism will work conveniently for them.

regards, tom lane

On 2022-12-04 Su 10:25, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-03 Sa 16:46, Tom Lane wrote:

1. Bikeshedding on my name choices is welcome. I know Robert is
dissatisfied with "ereturn", but I'm content with that so I didn't
change it here.

details_please seems more informal than our usual style. details_wanted
maybe?

Yeah, Corey didn't like that either. "details_wanted" works for me.

Soon after we get this done I think we'll find we need to extend this to
non-input functions. But that can wait a short while.

I'm curious to know exactly which other use-cases you foresee.
It wouldn't be a bad idea to write some draft code to verify
that this mechanism will work conveniently for them.

The SQL/JSON patches at [1]/messages/by-id/f54ebd2b-0e67-d1c6-4ff7-5d732492d1a0@postgrespro.ru included fixes for some numeric and datetime
conversion functions as well as various input functions, so that's a
fairly immediate need. More generally, I can see uses for error free
casts, something like, say CAST(foo AS bar ON ERROR blurfl)

cheers

andrew

[1]: /messages/by-id/f54ebd2b-0e67-d1c6-4ff7-5d732492d1a0@postgrespro.ru
/messages/by-id/f54ebd2b-0e67-d1c6-4ff7-5d732492d1a0@postgrespro.ru

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

On 12/4/22 17:21, Andrew Dunstan wrote:

More generally, I can see uses for error free
casts, something like, say CAST(foo AS bar ON ERROR blurfl)

What I am proposing for inclusion in the standard is basically the same
as what JSON does:

<cast specification> ::=
CAST <left paren>
<cast operand> AS <cast target>
[ FORMAT <cast template> ]
[ <cast error behavior> ON ERROR ]
<right paren>

<cast error behavior> ::=
ERROR
| NULL
| DEFAULT <value expression>

Once/If I get that in, I will be pushing to get that syntax in postgres
as well.
--
Vik Fearing

On Sun, Dec 04, 2022 at 06:01:33PM +0100, Vik Fearing wrote:

Once/If I get that in, I will be pushing to get that syntax in postgres as
well.

If I may ask, how long would it take to know if this grammar would be
integrated in the standard or not?
--
Michael

On Sat, Dec 3, 2022 at 10:57 PM Corey Huinker <corey.huinker@gmail.com> wrote:

2. ereturn_* => errfeedback / error_feedback / feedback

Oh, I like that, especially errfeedback.

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

Robert Haas <robertmhaas@gmail.com> writes:

On Sat, Dec 3, 2022 at 10:57 PM Corey Huinker <corey.huinker@gmail.com> wrote:

2. ereturn_* => errfeedback / error_feedback / feedback

Oh, I like that, especially errfeedback.

efeedback? But TBH I do not think any of these are better than ereturn.

Whether or not you agree with my position that it'd be best if the new
macro name is the same length as "ereport", I hope we can all agree
that it had better be short. ereport call nests already tend to contain
quite long lines. We don't need to add another couple tab-stops worth
of indentation there.

As for it being the same length: if you take a close look at my 0002
patch, you will realize that replacing ereport with a different-length
name will double or triple the number of lines that need to be touched
in many input functions. Yeah, we could sweep that under the rug to
some extent by submitting non-pgindent'd patches and running a separate
pgindent commit later, but that is not without pain. I don't want to
go there for the sake of a name that isn't really compellingly The
Right Word, and "feedback" just isn't very compelling IMO.

regards, tom lane

On Mon, Dec 5, 2022 at 11:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sat, Dec 3, 2022 at 10:57 PM Corey Huinker <corey.huinker@gmail.com> wrote:

2. ereturn_* => errfeedback / error_feedback / feedback

Oh, I like that, especially errfeedback.

efeedback? But TBH I do not think any of these are better than ereturn.

I do. Having a macro name that is "return" plus one character is going
to make people think that it returns. I predict that if you insist on
using that name people are still going to be making mistakes based on
that confusion 10 years from now.

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

On 2022-12-05 Mo 11:20, Robert Haas wrote:

On Mon, Dec 5, 2022 at 11:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sat, Dec 3, 2022 at 10:57 PM Corey Huinker <corey.huinker@gmail.com> wrote:

2. ereturn_* => errfeedback / error_feedback / feedback

Oh, I like that, especially errfeedback.

efeedback? But TBH I do not think any of these are better than ereturn.

I do. Having a macro name that is "return" plus one character is going
to make people think that it returns. I predict that if you insist on
using that name people are still going to be making mistakes based on
that confusion 10 years from now.

OK, I take both this point and Tom's about trying to keep it the same
length. So we need something that's  7 letters, doesn't say 'return' and
preferably begins with 'e'. I modestly suggest 'eseterr', or if we like
the 'feedback' idea 'efeedbk'.

cheers

andrew

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

On 12/5/22 11:36, Andrew Dunstan wrote:

On 2022-12-05 Mo 11:20, Robert Haas wrote:

On Mon, Dec 5, 2022 at 11:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sat, Dec 3, 2022 at 10:57 PM Corey Huinker <corey.huinker@gmail.com> wrote:

2. ereturn_* => errfeedback / error_feedback / feedback

Oh, I like that, especially errfeedback.

efeedback? But TBH I do not think any of these are better than ereturn.

I do. Having a macro name that is "return" plus one character is going
to make people think that it returns. I predict that if you insist on
using that name people are still going to be making mistakes based on
that confusion 10 years from now.

OK, I take both this point and Tom's about trying to keep it the same
length. So we need something that's  7 letters, doesn't say 'return' and
preferably begins with 'e'. I modestly suggest 'eseterr', or if we like
the 'feedback' idea 'efeedbk'.

Maybe eretort?

--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Dec 5, 2022 at 11:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

efeedback? But TBH I do not think any of these are better than ereturn.

I do. Having a macro name that is "return" plus one character is going
to make people think that it returns.

But it does return, or at least you need to code on the assumption
that it will. (The cases where it doesn't aren't much different
from any situation where a called subroutine unexpectedly throws
an error. Callers typically don't have to consider that.)

regards, tom lane

Joe Conway <mail@joeconway.com> writes:

On 12/5/22 11:36, Andrew Dunstan wrote:

OK, I take both this point and Tom's about trying to keep it the same
length. So we need something that's 7 letters, doesn't say 'return' and
preferably begins with 'e'. I modestly suggest 'eseterr', or if we like
the 'feedback' idea 'efeedbk'.

Maybe eretort?

Nah, it's so close to ereport that it looks like a typo. eseterr isn't
awful, perhaps. Or maybe errXXXX, but I've not thought of suitable XXXX.

regards, tom lane

On Mon, Dec 5, 2022 at 12:09 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

But it does return, or at least you need to code on the assumption
that it will. (The cases where it doesn't aren't much different
from any situation where a called subroutine unexpectedly throws
an error. Callers typically don't have to consider that.)

Are you just trolling me here?

AIUI, the macro never returns in the sense of using the return
statement, unlike PG_RETURN_WHATEVER(), which do. It possibly
transfers control by throwing an error. But that is also true of just
about everything you do in PostgreSQL code, because errors can get
thrown from almost anywhere. So clearly the possibility of a non-local
transfer of control is not the issue here. The issue is the
possibility that there will be NO transfer of control. That is, you
are compelled to write ereturn() and then afterwards you still need a
return statement.

I do not understand how it is possible to sensibly argue that someone
won't see a macro called ereturn() and perhaps come to the false
conclusion that it will always return.

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

I wrote:

Nah, it's so close to ereport that it looks like a typo. eseterr isn't
awful, perhaps. Or maybe errXXXX, but I've not thought of suitable XXXX.

... "errsave", maybe?

regards, tom lane

On Mon, Dec 5, 2022 at 12:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

Nah, it's so close to ereport that it looks like a typo. eseterr isn't
awful, perhaps. Or maybe errXXXX, but I've not thought of suitable XXXX.

... "errsave", maybe?

eseterr or errsave seem totally fine to me, FWIW.

I would probably choose a more verbose name if I were doing it, but I
do get the point that keeping line lengths reasonable is important,
and if someone were to accuse me of excessive prolixity, I would be
unable to mount much of a defense.

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

On 12/5/22 12:35, Robert Haas wrote:

On Mon, Dec 5, 2022 at 12:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

Nah, it's so close to ereport that it looks like a typo. eseterr isn't
awful, perhaps. Or maybe errXXXX, but I've not thought of suitable XXXX.

... "errsave", maybe?

eseterr or errsave seem totally fine to me, FWIW.

+1

I would probably choose a more verbose name if I were doing it, but I
do get the point that keeping line lengths reasonable is important,
and if someone were to accuse me of excessive prolixity, I would be
unable to mount much of a defense.

prolixity -- nice word! I won't comment on its applicability to you in
particular ;-P

--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On 2022-Dec-05, Tom Lane wrote:

I wrote:

Nah, it's so close to ereport that it looks like a typo. eseterr isn't
awful, perhaps. Or maybe errXXXX, but I've not thought of suitable XXXX.

... "errsave", maybe?

IMO eseterr is quite awful while errsave is not, so here goes my vote
for the latter.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Tom: There seems to be something broken here.
Teodor: I'm in sackcloth and ashes... Fixed.
http://archives.postgresql.org/message-id/482D1632.8010507@sigaev.ru

Robert Haas <robertmhaas@gmail.com> writes:

AIUI, the macro never returns in the sense of using the return
statement, unlike PG_RETURN_WHATEVER(), which do.

Oh! Now I see what you don't like about it. I thought you
meant "return to the call site", not "return to the call site's
caller". Agreed that that could be confusing.

regards, tom lane

On Mon, Dec 5, 2022 at 12:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

AIUI, the macro never returns in the sense of using the return
statement, unlike PG_RETURN_WHATEVER(), which do.

Oh! Now I see what you don't like about it. I thought you
meant "return to the call site", not "return to the call site's
caller". Agreed that that could be confusing.

OK, good. I couldn't figure out what in the world we were arguing
about... apparently I wasn't being as clear as I thought I was.

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

On 2022-12-05 Mo 12:42, Alvaro Herrera wrote:

On 2022-Dec-05, Tom Lane wrote:

I wrote:

Nah, it's so close to ereport that it looks like a typo. eseterr isn't
awful, perhaps. Or maybe errXXXX, but I've not thought of suitable XXXX.

... "errsave", maybe?

IMO eseterr is quite awful while errsave is not, so here goes my vote
for the latter.

Wait a minute!  Oh, no, sorry, as you were, 'errsave' is fine.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

Wait a minute! Oh, no, sorry, as you were, 'errsave' is fine.

Seems like everybody's okay with errsave. I'll make a v2 in a
little bit. I'd like to try updating array_in and/or record_in
just to verify that indirection cases work okay, before we consider
the design to be set.

regards, tom lane

On Mon, Dec 5, 2022 at 11:36 AM Andrew Dunstan <andrew@dunslane.net> wrote:

On 2022-12-05 Mo 11:20, Robert Haas wrote:

On Mon, Dec 5, 2022 at 11:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sat, Dec 3, 2022 at 10:57 PM Corey Huinker <corey.huinker@gmail.com>

wrote:

2. ereturn_* => errfeedback / error_feedback / feedback

Oh, I like that, especially errfeedback.

efeedback? But TBH I do not think any of these are better than ereturn.

I do. Having a macro name that is "return" plus one character is going
to make people think that it returns. I predict that if you insist on
using that name people are still going to be making mistakes based on
that confusion 10 years from now.

OK, I take both this point and Tom's about trying to keep it the same
length. So we need something that's 7 letters, doesn't say 'return' and
preferably begins with 'e'. I modestly suggest 'eseterr', or if we like
the 'feedback' idea 'efeedbk'.

Consulting https://www.thesaurus.com/browse/feedback again:
ereply clocks in at 7 characters.

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

Andrew Dunstan <andrew@dunslane.net> writes:

Wait a minute! Oh, no, sorry, as you were, 'errsave' is fine.

Seems like everybody's okay with errsave. I'll make a v2 in a
little bit. I'd like to try updating array_in and/or record_in
just to verify that indirection cases work okay, before we consider
the design to be set.

+1 to errsave.

On 2022-12-05 Mo 14:22, Corey Huinker wrote:

On Mon, Dec 5, 2022 at 11:36 AM Andrew Dunstan <andrew@dunslane.net>
wrote:

On 2022-12-05 Mo 11:20, Robert Haas wrote:

On Mon, Dec 5, 2022 at 11:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sat, Dec 3, 2022 at 10:57 PM Corey Huinker

<corey.huinker@gmail.com> wrote:

2. ereturn_* => errfeedback / error_feedback / feedback

Oh, I like that, especially errfeedback.

efeedback?  But TBH I do not think any of these are better than

ereturn.

I do. Having a macro name that is "return" plus one character is

going

to make people think that it returns. I predict that if you

insist on

using that name people are still going to be making mistakes

based on

that confusion 10 years from now.

OK, I take both this point and Tom's about trying to keep it the same
length. So we need something that's  7 letters, doesn't say
'return' and
preferably begins with 'e'. I modestly suggest 'eseterr', or if we
like
the 'feedback' idea 'efeedbk'.

Consulting https://www.thesaurus.com/browse/feedback again:
ereply clocks in at 7 characters.

It does?

cheers

andrew

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

I wrote:

Seems like everybody's okay with errsave. I'll make a v2 in a
little bit. I'd like to try updating array_in and/or record_in
just to verify that indirection cases work okay, before we consider
the design to be set.

v2 as promised, incorporating the discussed renamings as well as some
follow-on ones (ErrorReturnContext -> ErrorSaveContext, notably).

I also tried moving the struct into a new header file, miscnodes.h
after Andrew's suggestion upthread. That seems at least marginally
cleaner than putting it in nodes.h, although I'm not wedded to this
choice.

I was really glad that I took the trouble to update some less-trivial
input functions, because I learned two things:

* It's better if InputFunctionCallSafe will tolerate the case of not
being passed an ErrorSaveContext. In the COPY hack it felt worthwhile
to have completely separate code paths calling InputFunctionCallSafe
or InputFunctionCall, but that's less appetizing elsewhere.

* There's a crying need for a macro that wraps up errsave() with an
immediate return. Hence, ereturn() is reborn from the ashes. I hope
Robert won't object to that name if it *does* do a return.

I feel pretty good about this version; it seems committable if there
are not objections. Not sure if we should commit 0003 like this,
though.

regards, tom lane

Hi,

On 2022-12-05 16:40:06 -0500, Tom Lane wrote:

+/*
+ * errsave_start --- begin a "safe" error-reporting cycle
+ *
+ * If "context" isn't an ErrorSaveContext node, this behaves as
+ * errstart(ERROR, domain), and the errsave() macro ends up acting
+ * exactly like ereport(ERROR, ...).
+ *
+ * If "context" is an ErrorSaveContext node, but the node creator only wants
+ * notification of the fact of a safe error without any details, just set
+ * the error_occurred flag in the ErrorSaveContext node and return false,
+ * which will cause us to skip the remaining error processing steps.
+ *
+ * Otherwise, create and initialize error stack entry and return true.
+ * Subsequently, errmsg() and perhaps other routines will be called to further
+ * populate the stack entry.  Finally, errsave_finish() will be called to
+ * tidy up.
+ */
+bool
+errsave_start(void *context, const char *domain)

Why is context a void *?

+{
+	ErrorSaveContext *escontext;
+	ErrorData  *edata;
+
+	/*
+	 * Do we have a context for safe error reporting?  If not, just punt to
+	 * errstart().
+	 */
+	if (context == NULL || !IsA(context, ErrorSaveContext))
+		return errstart(ERROR, domain);

I don't think we should "accept" !IsA(context, ErrorSaveContext) - that
seems likely to hide things like use-after-free.

+	if (++errordata_stack_depth >= ERRORDATA_STACK_SIZE)
+	{
+		/*
+		 * Wups, stack not big enough.  We treat this as a PANIC condition
+		 * because it suggests an infinite loop of errors during error
+		 * recovery.
+		 */
+		errordata_stack_depth = -1; /* make room on stack */
+		ereport(PANIC, (errmsg_internal("ERRORDATA_STACK_SIZE exceeded")));
+	}

This is the fourth copy of this code...

+/*
+ * errsave_finish --- end a "safe" error-reporting cycle
+ *
+ * If errsave_start() decided this was a regular error, behave as
+ * errfinish().  Otherwise, package up the error details and save
+ * them in the ErrorSaveContext node.
+ */
+void
+errsave_finish(void *context, const char *filename, int lineno,
+			   const char *funcname)
+{
+	ErrorSaveContext *escontext = (ErrorSaveContext *) context;
+	ErrorData  *edata = &errordata[errordata_stack_depth];
+
+	/* verify stack depth before accessing *edata */
+	CHECK_STACK_DEPTH();
+
+	/*
+	 * If errsave_start punted to errstart, then elevel will be ERROR or
+	 * perhaps even PANIC.  Punt likewise to errfinish.
+	 */
+	if (edata->elevel >= ERROR)
+		errfinish(filename, lineno, funcname);

I'd put a pg_unreachable() or such after the errfinish() call.

+	/*
+	 * Else, we should package up the stack entry contents and deliver them to
+	 * the caller.
+	 */
+	recursion_depth++;
+
+	/* Save the last few bits of error state into the stack entry */
+	if (filename)
+	{
+		const char *slash;
+
+		/* keep only base name, useful especially for vpath builds */
+		slash = strrchr(filename, '/');
+		if (slash)
+			filename = slash + 1;
+		/* Some Windows compilers use backslashes in __FILE__ strings */
+		slash = strrchr(filename, '\\');
+		if (slash)
+			filename = slash + 1;
+	}
+
+	edata->filename = filename;
+	edata->lineno = lineno;
+	edata->funcname = funcname;
+	edata->elevel = ERROR;		/* hide the LOG value used above */
+
+	/*
+	 * We skip calling backtrace and context functions, which are more likely
+	 * to cause trouble than provide useful context; they might act on the
+	 * assumption that a transaction abort is about to occur.
+	 */

This seems like a fair bit of duplicated code.

+ * This is the same as InputFunctionCall, but the caller may also pass a
+ * previously-initialized ErrorSaveContext node.  (We declare that as
+ * "void *" to avoid including miscnodes.h in fmgr.h.)

It seems way cleaner to forward declare ErrorSaveContext instead of
using void *.

If escontext points
+ * to an ErrorSaveContext, any "safe" errors detected by the input function
+ * will be reported by filling the escontext struct.  The caller must
+ * check escontext->error_occurred before assuming that the function result
+ * is meaningful.

I wonder if we shouldn't instead make InputFunctionCallSafe() return a
boolean and return the Datum via a pointer. As callers are otherwise
going to need to do SAFE_ERROR_OCCURRED(escontext) themselves, I think
it should also lead to more concise (and slightly more efficient) code.

+Datum
+InputFunctionCallSafe(FmgrInfo *flinfo, char *str,
+					  Oid typioparam, int32 typmod,
+					  void *escontext)

Is there a reason not to provide this infrastructure for
ReceiveFunctionCall() as well?

Not that I have a suggestion for a better name, but I don't particularly
like "Safe" denoting non-erroring input function calls. There's too many
interpretations of safe - e.g. safe against privilege escalation issues
or such.

@@ -252,10 +254,13 @@ record_in(PG_FUNCTION_ARGS)
column_info->column_type = column_type;
}

-		values[i] = InputFunctionCall(&column_info->proc,
-									  column_data,
-									  column_info->typioparam,
-									  att->atttypmod);
+		values[i] = InputFunctionCallSafe(&column_info->proc,
+										  column_data,
+										  column_info->typioparam,
+										  att->atttypmod,
+										  escontext);
+		if (SAFE_ERROR_OCCURRED(escontext))
+			PG_RETURN_NULL();

It doesn't *quite* seem right to set ->isnull in case of an error. Not
that it has an obvious harm.

Wonder if it's perhaps worth to add VALGRIND_MAKE_MEM_UNDEFINED() calls
to InputFunctionCallSafe() to more easily detect cases where a caller
ignores that an error occured.

+			if (safe_mode)
+			{
+				ErrorSaveContext *es_context = cstate->es_context;
+
+				/* Must reset the error_occurred flag each time */
+				es_context->error_occurred = false;

I'd put that into the if (es_context->error_occurred) path. Likely the
window for store-forwarding issues is smaller than
InputFunctionCallSafe(), but it's trivial to write it differently...

diff --git a/src/test/regress/sql/copy.sql b/src/test/regress/sql/copy.sql
index 285022e07c..ff77d27cfc 100644
--- a/src/test/regress/sql/copy.sql
+++ b/src/test/regress/sql/copy.sql
@@ -268,3 +268,23 @@ a	c	b

SELECT * FROM header_copytest ORDER BY a;
drop table header_copytest;
+
+-- "safe" error handling
+create table on_error_copytest(i int, b bool, ai int[]);
+
+copy on_error_copytest from stdin with (null_on_error);
+1	a	{1,}
+err	1	{x}
+2	f	{3,4}
+bad	x	{,
+\.
+
+copy on_error_copytest from stdin with (warn_on_error);
+3	0	[3:4]={3,4}
+4	b	[0:1000]={3,4}
+err	t	{}
+bad	z	{"zed"}
+\.
+
+select * from on_error_copytest;
+drop table on_error_copytest;

Think it'd be good to have a test for a composite type where one of the
columns safely errors out and the other doesn't.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

Why is context a void *?

elog.h can't depend on nodes.h, at least not without some rather
fundamental rethinking of our #include relationships. We could
possibly use the same kind of hack that fmgr.h does:

typedef struct Node *fmNodePtr;

but I'm not sure that's much of an improvement. Note that it'd
*not* be correct to declare it as anything more specific than Node*,
since the fmgr context pointer is Node* and we're not expecting
callers to do their own IsA checks to see what they were passed.

I don't think we should "accept" !IsA(context, ErrorSaveContext) - that
seems likely to hide things like use-after-free.

No, see above. Moving the IsA checks out to the callers would
not improve the garbage-pointer risk one bit, it would just
add code bloat.

I'd put a pg_unreachable() or such after the errfinish() call.

[ shrug... ] Kinda pointless IMO, but OK.

This seems like a fair bit of duplicated code.

I don't think refactoring to remove the duplication would improve it.

+ * This is the same as InputFunctionCall, but the caller may also pass a
+ * previously-initialized ErrorSaveContext node.  (We declare that as
+ * "void *" to avoid including miscnodes.h in fmgr.h.)

It seems way cleaner to forward declare ErrorSaveContext instead of
using void *.

Again, it cannot be any more specific than Node*. But you're right
that we could use fmNodePtr here, and that would be at least a little
nicer.

I wonder if we shouldn't instead make InputFunctionCallSafe() return a
boolean and return the Datum via a pointer. As callers are otherwise
going to need to do SAFE_ERROR_OCCURRED(escontext) themselves, I think
it should also lead to more concise (and slightly more efficient) code.

Hmm, maybe. It would be a bigger change from existing code, but
I don't think very many call sites would be impacted. (But by
the same token, we'd not save much code this way.) Personally
I put more value on keeping similar APIs between InputFunctionCall
and InputFunctionCallSafe, but I won't argue hard if you're insistent.

Is there a reason not to provide this infrastructure for
ReceiveFunctionCall() as well?

There's a comment in 0003 about that: I doubt that it makes sense
to have no-error semantics for binary input. That would require
far more trust in the receive functions' ability to detect garbage
input than I think they have in reality. Perhaps more to the
point, even if we ultimately do that I don't want to do it now.
Including the receive functions in the first-pass conversion would
roughly double the amount of work needed per datatype, and we are
already going to be hard put to it to finish what needs to be done
for v16.

Not that I have a suggestion for a better name, but I don't particularly
like "Safe" denoting non-erroring input function calls. There's too many
interpretations of safe - e.g. safe against privilege escalation issues
or such.

Yeah, I'm not that thrilled with it either --- but it's a reasonably
on-point modifier, and short.

It doesn't *quite* seem right to set ->isnull in case of an error. Not
that it has an obvious harm.

Doesn't matter: if the caller pays attention to either the Datum
value or the isnull flag, it's broken.

Wonder if it's perhaps worth to add VALGRIND_MAKE_MEM_UNDEFINED() calls
to InputFunctionCallSafe() to more easily detect cases where a caller
ignores that an error occured.

I do not think there are going to be enough callers of
InputFunctionCallSafe that we need such tactics to validate them.

I'd put that into the if (es_context->error_occurred) path. Likely the
window for store-forwarding issues is smaller than
InputFunctionCallSafe(), but it's trivial to write it differently...

Does not seem better to me, and your argument for it seems like the
worst sort of premature micro-optimization.

Think it'd be good to have a test for a composite type where one of the
columns safely errors out and the other doesn't.

I wasn't trying all that hard on the error tests, because I think
0003 is just throwaway code at this point. If we want to seriously
check the input functions' behavior then we need to factorize the
tests so it can be done per-datatype, not in one central place in
the COPY tests. For the core types it could make sense to provide
some function in pg_regress.c that allows access to the non-exception
code path independently of COPY; but I'm not sure how contrib
datatypes could use that.

In any case, I'm unconvinced that testing each error exit both ways is
likely to be a profitable use of test cycles. The far more likely source
of problems with this patch series is going to be that we miss converting
some ereport call that is reachable with bad input. No amount of
testing is going to prove that that didn't happen.

regards, tom lane

Hi,

On 2022-12-05 19:18:11 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

Why is context a void *?

elog.h can't depend on nodes.h, at least not without some rather
fundamental rethinking of our #include relationships. We could
possibly use the same kind of hack that fmgr.h does:

typedef struct Node *fmNodePtr;

but I'm not sure that's much of an improvement. Note that it'd
*not* be correct to declare it as anything more specific than Node*,
since the fmgr context pointer is Node* and we're not expecting
callers to do their own IsA checks to see what they were passed.

Ah - I hadn't actually grokked that that's the reason for the
void*. Unless I missed a comment to that regard, entirely possible, it
seems worth explaining that above errsave_start().

This seems like a fair bit of duplicated code.

I don't think refactoring to remove the duplication would improve it.

Why? I think a populate_edata() or such seems to make sense. And the
required argument to skip ->backtrace and error_context_stack processing
seem like things that'd be good to document anyway.

I wonder if we shouldn't instead make InputFunctionCallSafe() return a
boolean and return the Datum via a pointer. As callers are otherwise
going to need to do SAFE_ERROR_OCCURRED(escontext) themselves, I think
it should also lead to more concise (and slightly more efficient) code.

Hmm, maybe. It would be a bigger change from existing code, but
I don't think very many call sites would be impacted. (But by
the same token, we'd not save much code this way.) Personally
I put more value on keeping similar APIs between InputFunctionCall
and InputFunctionCallSafe, but I won't argue hard if you're insistent.

I think it's good to diverge from the existing code, because imo the
behaviour is quite different and omitting the SAFE_ERROR_OCCURRED()
check will lead to brokenness.

Is there a reason not to provide this infrastructure for
ReceiveFunctionCall() as well?

There's a comment in 0003 about that: I doubt that it makes sense
to have no-error semantics for binary input. That would require
far more trust in the receive functions' ability to detect garbage
input than I think they have in reality. Perhaps more to the
point, even if we ultimately do that I don't want to do it now.
Including the receive functions in the first-pass conversion would
roughly double the amount of work needed per datatype, and we are
already going to be hard put to it to finish what needs to be done
for v16.

Fair enough.

Wonder if it's perhaps worth to add VALGRIND_MAKE_MEM_UNDEFINED() calls
to InputFunctionCallSafe() to more easily detect cases where a caller
ignores that an error occured.

I do not think there are going to be enough callers of
InputFunctionCallSafe that we need such tactics to validate them.

I predict that we'll have quite a few bugs due to converting some parts
of the system, but not other parts. But we can add them later, so I'll
not insist on it.

I'd put that into the if (es_context->error_occurred) path. Likely the
window for store-forwarding issues is smaller than
InputFunctionCallSafe(), but it's trivial to write it differently...

Does not seem better to me, and your argument for it seems like the
worst sort of premature micro-optimization.

Shrug. The copy code is quite slow today, but not by a single source,
but by death by a thousand cuts.

Think it'd be good to have a test for a composite type where one of the
columns safely errors out and the other doesn't.

I wasn't trying all that hard on the error tests, because I think
0003 is just throwaway code at this point.

I am mainly interested in having *something* test erroring out hard when
using the "Safe" mechanism, which afaict we don't have with the patches
as they stand. You're right that it'd be better to do that without COPY
in the way, but it doesn't seem all that crucial.

If we want to seriously check the input functions' behavior then we
need to factorize the tests so it can be done per-datatype, not in one
central place in the COPY tests. For the core types it could make
sense to provide some function in pg_regress.c that allows access to
the non-exception code path independently of COPY; but I'm not sure
how contrib datatypes could use that.

It might be worth adding a function for testing safe input functions
into core PG - it's not like we don't have other such functions.

But perhaps it's even worth having such a function properly exposed:
It's not at all rare to parse text data during ETL and quite often
erroring out fatally is undesirable. As savepoints are undesirable
overhead-wise, there's a lot of SQL out there that tries to do a
pre-check about whether some text could be cast to some other data
type. A function that'd try to cast input to a certain type without
erroring out hard would be quite useful for that.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2022-12-05 19:18:11 -0500, Tom Lane wrote:

but I'm not sure that's much of an improvement. Note that it'd
*not* be correct to declare it as anything more specific than Node*,
since the fmgr context pointer is Node* and we're not expecting
callers to do their own IsA checks to see what they were passed.

Ah - I hadn't actually grokked that that's the reason for the
void*. Unless I missed a comment to that regard, entirely possible, it
seems worth explaining that above errsave_start().

There's a comment about that in elog.h IIRC, but no harm in saying
it in elog.c as well.

Having said that, I am warming a little bit to making these pointers
be Node* or an alias spelling of that rather than void*.

I don't think refactoring to remove the duplication would improve it.

Why? I think a populate_edata() or such seems to make sense. And the
required argument to skip ->backtrace and error_context_stack processing
seem like things that'd be good to document anyway.

Meh. Well, I'll have a look, but it seems kind of orthogonal to the
main point of the patch.

Hmm, maybe. It would be a bigger change from existing code, but
I don't think very many call sites would be impacted. (But by
the same token, we'd not save much code this way.) Personally
I put more value on keeping similar APIs between InputFunctionCall
and InputFunctionCallSafe, but I won't argue hard if you're insistent.

I think it's good to diverge from the existing code, because imo the
behaviour is quite different and omitting the SAFE_ERROR_OCCURRED()
check will lead to brokenness.

True, but it only helps for the immediate caller of InputFunctionCallSafe,
not for call levels further out. Still, I'll give that a look.

I wasn't trying all that hard on the error tests, because I think
0003 is just throwaway code at this point.

I am mainly interested in having *something* test erroring out hard when
using the "Safe" mechanism, which afaict we don't have with the patches
as they stand. You're right that it'd be better to do that without COPY
in the way, but it doesn't seem all that crucial.

Hmm, either I'm confused or you're stating that backwards --- aren't
the hard-error code paths already tested by our existing tests?

But perhaps it's even worth having such a function properly exposed:
It's not at all rare to parse text data during ETL and quite often
erroring out fatally is undesirable. As savepoints are undesirable
overhead-wise, there's a lot of SQL out there that tries to do a
pre-check about whether some text could be cast to some other data
type. A function that'd try to cast input to a certain type without
erroring out hard would be quite useful for that.

Corey and Vik are already talking about a non-error CAST variant.
Maybe we should leave this in abeyance until something shows up
for that? Otherwise we'll be making a nonstandard API for what
will probably ultimately be SQL-spec functionality. I don't mind
that as regression-test infrastructure, but I'm a bit less excited
about exposing it as a user feature.

regards, tom lane

Hi,

On 2022-12-05 20:06:55 -0500, Tom Lane wrote:

I wasn't trying all that hard on the error tests, because I think
0003 is just throwaway code at this point.

I am mainly interested in having *something* test erroring out hard when
using the "Safe" mechanism, which afaict we don't have with the patches
as they stand. You're right that it'd be better to do that without COPY
in the way, but it doesn't seem all that crucial.

Hmm, either I'm confused or you're stating that backwards --- aren't
the hard-error code paths already tested by our existing tests?

What I'd like to test is a hard error, either due to an input function
that wasn't converted or because it's a type of error that can't be
handled "softly", but when using the "safe" interface.

But perhaps it's even worth having such a function properly exposed:
It's not at all rare to parse text data during ETL and quite often
erroring out fatally is undesirable. As savepoints are undesirable
overhead-wise, there's a lot of SQL out there that tries to do a
pre-check about whether some text could be cast to some other data
type. A function that'd try to cast input to a certain type without
erroring out hard would be quite useful for that.

Corey and Vik are already talking about a non-error CAST variant.
Maybe we should leave this in abeyance until something shows up
for that? Otherwise we'll be making a nonstandard API for what
will probably ultimately be SQL-spec functionality. I don't mind
that as regression-test infrastructure, but I'm a bit less excited
about exposing it as a user feature.

Yea, I'm fine with that. I was just thinking out loud on this aspect.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2022-12-05 20:06:55 -0500, Tom Lane wrote:

Hmm, either I'm confused or you're stating that backwards --- aren't
the hard-error code paths already tested by our existing tests?

What I'd like to test is a hard error, either due to an input function
that wasn't converted or because it's a type of error that can't be
handled "softly", but when using the "safe" interface.

Oh, I see. That seems like kind of a problematic requirement,
unless we leave some datatype around that's intentionally not
ever going to be converted. For datatypes that we do convert,
there shouldn't be any easy way to get to a hard error.

I don't really quite understand why you're worried about that
though. The hard-error code paths are well tested already.

regards, tom lane

Hi,

On 2022-12-05 20:19:26 -0500, Tom Lane wrote:

That seems like kind of a problematic requirement, unless we leave some
datatype around that's intentionally not ever going to be converted. For
datatypes that we do convert, there shouldn't be any easy way to get to a
hard error.

I suspect there are going to be types we can't convert. But even if not - that
actually makes a *stronger* case for ensuring the path is tested, because
certainly some out of core types aren't going to be converted.

This made me look at fmgr/README again:

+Considering datatype input functions as examples, typical "safe" error
+conditions include input syntax errors and out-of-range values.  An input
+function typically detects such cases with simple if-tests and can easily
+change the following ereport call to errsave.  Error conditions that
+should NOT be handled this way include out-of-memory, internal errors, and
+anything where there is any question about our ability to continue normal
+processing of the transaction.  Those should still be thrown with ereport.

I wonder if we should provide more guidance around what kind of catalogs
access are acceptable before avoiding throwing an error.

This in turn make me look at record_in() in 0002 - I think we might be leaking
a tupledesc refcount in case of errors. Yup:

DROP TABLE IF EXISTS tbl_as_record, tbl_with_record;

CREATE TABLE tbl_as_record(a int, b int);
CREATE TABLE tbl_with_record(composite_col tbl_as_record, non_composite_col int);

COPY tbl_with_record FROM stdin WITH (warn_on_error);
kdjkdf 212
\.

WARNING: 22P02: invalid input for column composite_col: malformed record literal: "kdjkdf"
WARNING: 01000: TupleDesc reference leak: TupleDesc 0x7fb1c5fd0c58 (159584,-1) still referenced

I don't really quite understand why you're worried about that
though. The hard-error code paths are well tested already.

Afaict they're not tested when going through InputFunctionCallSafe() / with an
ErrorSaveContext. To me that does seem worth testing.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

This in turn make me look at record_in() in 0002 - I think we might be leaking
a tupledesc refcount in case of errors. Yup:

Doh :-( ... I did that function a little too hastily, obviously.
Thanks for catching that.

regards, tom lane

On 2022-12-05 Mo 20:06, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

But perhaps it's even worth having such a function properly exposed:
It's not at all rare to parse text data during ETL and quite often
erroring out fatally is undesirable. As savepoints are undesirable
overhead-wise, there's a lot of SQL out there that tries to do a
pre-check about whether some text could be cast to some other data
type. A function that'd try to cast input to a certain type without
erroring out hard would be quite useful for that.

Corey and Vik are already talking about a non-error CAST variant.

/metoo! :-)

Maybe we should leave this in abeyance until something shows up
for that? Otherwise we'll be making a nonstandard API for what
will probably ultimately be SQL-spec functionality. I don't mind
that as regression-test infrastructure, but I'm a bit less excited
about exposing it as a user feature.

I think a functional mechanism could be very useful. Who knows when the
standard might specify something in this area?

cheers

andrew

--

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

[ continuing the naming quagmire... ]

I wrote:

Andres Freund <andres@anarazel.de> writes:

Not that I have a suggestion for a better name, but I don't particularly
like "Safe" denoting non-erroring input function calls. There's too many
interpretations of safe - e.g. safe against privilege escalation issues
or such.

Yeah, I'm not that thrilled with it either --- but it's a reasonably
on-point modifier, and short.

It occurs to me that another spelling could be NoError (or _noerror
where not using camel case). There's some precedent for that already;
and where we have it, it has the same implication of reporting rather
than throwing certain errors, without making a guarantee about all
errors. For instance lookup_rowtype_tupdesc_noerror won't prevent
throwing errors if catalog corruption is detected inside the catcaches.

I'm not sure this is any *better* than Safe ... it's longer, less
mellifluous, and still subject to misinterpretation. But it's
a possible alternative.

regards, tom lane

On 2022-12-06 Tu 09:42, Tom Lane wrote:

[ continuing the naming quagmire... ]

I wrote:

Andres Freund <andres@anarazel.de> writes:

Not that I have a suggestion for a better name, but I don't particularly
like "Safe" denoting non-erroring input function calls. There's too many
interpretations of safe - e.g. safe against privilege escalation issues
or such.

Yeah, I'm not that thrilled with it either --- but it's a reasonably
on-point modifier, and short.

It occurs to me that another spelling could be NoError (or _noerror
where not using camel case). There's some precedent for that already;
and where we have it, it has the same implication of reporting rather
than throwing certain errors, without making a guarantee about all
errors. For instance lookup_rowtype_tupdesc_noerror won't prevent
throwing errors if catalog corruption is detected inside the catcaches.

I'm not sure this is any *better* than Safe ... it's longer, less
mellifluous, and still subject to misinterpretation. But it's
a possible alternative.

Yeah, I don't think there's terribly much to choose between 'safe' and
'noerror' in terms of meaning.

I originally chose InputFunctionCallContext as a more neutral name in
case we wanted to be able to pass some other sort of node for the
context in future.

Maybe that was a little too forward looking.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-06 Tu 09:42, Tom Lane wrote:

I'm not sure this is any *better* than Safe ... it's longer, less
mellifluous, and still subject to misinterpretation. But it's
a possible alternative.

Yeah, I don't think there's terribly much to choose between 'safe' and
'noerror' in terms of meaning.

Yeah, I just wanted to throw it out there and see if anyone thought
it was a better idea.

I originally chose InputFunctionCallContext as a more neutral name in
case we wanted to be able to pass some other sort of node for the
context in future.
Maybe that was a little too forward looking.

I didn't like that because it seemed to convey nothing at all about
the expected behavior.

regards, tom lane

On Tue, Dec 6, 2022 at 11:07 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I originally chose InputFunctionCallContext as a more neutral name in
case we wanted to be able to pass some other sort of node for the
context in future.
Maybe that was a little too forward looking.

I didn't like that because it seemed to convey nothing at all about
the expected behavior.

I feel like this can go either way. If we pick a name that conveys a
specific intended behavior now, and then later we want to pass some
other sort of node for some purpose other than ignoring errors, it's
unpleasant to have a name that sounds like it can only ignore errors.
But if we never use it for anything other than ignoring errors, a
specific name is clearer.

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

On Tue, Dec 6, 2022 at 6:46 AM Andrew Dunstan <andrew@dunslane.net> wrote:

On 2022-12-05 Mo 20:06, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

But perhaps it's even worth having such a function properly exposed:
It's not at all rare to parse text data during ETL and quite often
erroring out fatally is undesirable. As savepoints are undesirable
overhead-wise, there's a lot of SQL out there that tries to do a
pre-check about whether some text could be cast to some other data
type. A function that'd try to cast input to a certain type without
erroring out hard would be quite useful for that.

Corey and Vik are already talking about a non-error CAST variant.

/metoo! :-)

Maybe we should leave this in abeyance until something shows up
for that? Otherwise we'll be making a nonstandard API for what
will probably ultimately be SQL-spec functionality. I don't mind
that as regression-test infrastructure, but I'm a bit less excited
about exposing it as a user feature.

I think a functional mechanism could be very useful. Who knows when the
standard might specify something in this area?

Vik's working on the standard (he put the spec in earlier in this thread).
I'm working on implementing it on top of Tom's work, but I'm one patchset
behind at the moment.

Once completed, it should be leverage-able in several places, COPY being
the most obvious.

What started all this was me noticing that if I implemented TRY_CAST in
pl/pgsql with an exception block, then I wasn't able to use parallel query.

OK, here's a v3 responding to the comments from Andres.

0000 is preliminary refactoring of elog.c, with (I trust) no
functional effect. It gets rid of some pre-existing code duplication
as well as setting up to let 0001's additions be less duplicative.

0001 adopts use of Node pointers in place of "void *". To do this
I needed an alias type in elog.h equivalent to fmgr.h's fmNodePtr.
I decided that having two different aliases would be too confusing,
so what I did here was to converge both elog.h and fmgr.h on using
the same alias "typedef struct Node *NodePtr". That has to be in
elog.h since it's included first, from postgres.h. (I thought of
defining NodePtr in postgres.h, but postgres.h includes elog.h
immediately so that wouldn't have looked very nice.)

I also adopted Andres' recommendation that InputFunctionCallSafe
return boolean. I'm still not totally sold on that ... but it does
end with array_in and record_in never using SAFE_ERROR_OCCURRED at
all, so maybe the idea's OK.

0002 adjusts the I/O functions for these API changes, and fixes
my silly oversight about error cleanup in record_in.

Given the discussion about testing requirements, I threw away the
COPY hack entirely. This 0003 provides a couple of SQL-callable
functions that can be used to invoke a specific datatype's input
function. I haven't documented them, pending bikeshedding on
names etc. I also arranged to test array_in and record_in with
a datatype that still throws errors, reserving the existing test
type "widget" for that purpose.

(I'm not intending to foreclose development of new COPY features
in this area, just abandoning the idea that that's our initial
test mechanism.)

Thoughts?

regards, tom lane

Robert Haas <robertmhaas@gmail.com> writes:

I feel like this can go either way. If we pick a name that conveys a
specific intended behavior now, and then later we want to pass some
other sort of node for some purpose other than ignoring errors, it's
unpleasant to have a name that sounds like it can only ignore errors.
But if we never use it for anything other than ignoring errors, a
specific name is clearer.

With Andres' proposal to make the function return boolean succeed/fail,
I think it's pretty clear that the only useful case is to pass an
ErrorSaveContext. There may well be future APIs that pass some other
kind of context object to input functions, but they'll presumably
have different goals and want a different sort of wrapper function.

regards, tom lane

On 2022-12-06 Tu 15:21, Tom Lane wrote:

OK, here's a v3 responding to the comments from Andres.

Looks pretty good to me.

0000 is preliminary refactoring of elog.c, with (I trust) no
functional effect. It gets rid of some pre-existing code duplication
as well as setting up to let 0001's additions be less duplicative.

0001 adopts use of Node pointers in place of "void *". To do this
I needed an alias type in elog.h equivalent to fmgr.h's fmNodePtr.
I decided that having two different aliases would be too confusing,
so what I did here was to converge both elog.h and fmgr.h on using
the same alias "typedef struct Node *NodePtr". That has to be in
elog.h since it's included first, from postgres.h. (I thought of
defining NodePtr in postgres.h, but postgres.h includes elog.h
immediately so that wouldn't have looked very nice.)

I also adopted Andres' recommendation that InputFunctionCallSafe
return boolean. I'm still not totally sold on that ... but it does
end with array_in and record_in never using SAFE_ERROR_OCCURRED at
all, so maybe the idea's OK.

Originally I wanted to make the new function look as much like the
original as possible, but I'm not wedded to that either. I can live with
it like this.

0002 adjusts the I/O functions for these API changes, and fixes
my silly oversight about error cleanup in record_in.

Given the discussion about testing requirements, I threw away the
COPY hack entirely. This 0003 provides a couple of SQL-callable
functions that can be used to invoke a specific datatype's input
function. I haven't documented them, pending bikeshedding on
names etc. I also arranged to test array_in and record_in with
a datatype that still throws errors, reserving the existing test
type "widget" for that purpose.

(I'm not intending to foreclose development of new COPY features
in this area, just abandoning the idea that that's our initial
test mechanism.)

The new functions on their own are likely to make plenty of people quite
happy once we've adjusted all the input functions.

Perhaps we should add a type in the regress library that will never have
a safe input function, so we can test that the mechanism works as
expected in that case even after we adjust all the core data types'
input functions.

Otherwise I think we're good to go.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

Perhaps we should add a type in the regress library that will never have
a safe input function, so we can test that the mechanism works as
expected in that case even after we adjust all the core data types'
input functions.

I was intending that the existing "widget" type be that. 0003 already
adds a comment to widget_in saying not to "fix" its one ereport call.

Returning to the naming quagmire -- it occurred to me just now that
it might be helpful to call this style of error reporting "soft"
errors rather than "safe" errors, which'd provide a nice contrast
with "hard" errors thrown by longjmp'ing. That would lead to naming
all the variant functions XXXSoft not XXXSafe. There would still
be commentary to the effect that "soft errors must be safe, in the
sense that there's no question whether it's safe to continue
processing the transaction". Anybody think that'd be an
improvement?

regards, tom lane

On Wed, Dec 7, 2022 at 7:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Returning to the naming quagmire -- it occurred to me just now that
it might be helpful to call this style of error reporting "soft"
errors rather than "safe" errors, which'd provide a nice contrast
with "hard" errors thrown by longjmp'ing. That would lead to naming
all the variant functions XXXSoft not XXXSafe. There would still
be commentary to the effect that "soft errors must be safe, in the
sense that there's no question whether it's safe to continue
processing the transaction". Anybody think that'd be an
improvement?

+1

David J.

On 2022-12-07 We 09:20, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Perhaps we should add a type in the regress library that will never have
a safe input function, so we can test that the mechanism works as
expected in that case even after we adjust all the core data types'
input functions.

I was intending that the existing "widget" type be that. 0003 already
adds a comment to widget_in saying not to "fix" its one ereport call.

Yeah, I see that, I must have been insufficiently caffeinated.

Returning to the naming quagmire -- it occurred to me just now that
it might be helpful to call this style of error reporting "soft"
errors rather than "safe" errors, which'd provide a nice contrast
with "hard" errors thrown by longjmp'ing. That would lead to naming
all the variant functions XXXSoft not XXXSafe. There would still
be commentary to the effect that "soft errors must be safe, in the
sense that there's no question whether it's safe to continue
processing the transaction". Anybody think that'd be an
improvement?

I'm not sure InputFunctionCallSoft would be an improvement. Maybe
InputFunctionCallSoftError would be clearer, but I don't know that it's
much of an improvement either. The same goes for the other visible changes.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-07 We 09:20, Tom Lane wrote:

Returning to the naming quagmire -- it occurred to me just now that
it might be helpful to call this style of error reporting "soft"
errors rather than "safe" errors, which'd provide a nice contrast
with "hard" errors thrown by longjmp'ing. That would lead to naming
all the variant functions XXXSoft not XXXSafe.

I'm not sure InputFunctionCallSoft would be an improvement.

Yeah, after reflecting on it a bit more I'm not that impressed with
that as a function name either.

(I think that "soft error" could be useful as informal terminology.
AFAIR we don't use "hard error" in any formal way either, but there
are certainly comments using that phrase.)

More questions:

* Anyone want to bikeshed about the new SQL-level function names?
I'm reasonably satisfied with "pg_input_is_valid" for the bool-returning
variant, but not so much with "pg_input_invalid_message" for the
error-message-returning variant. Thinking about "pg_input_error_message"
instead, but that's not stellar either.

* Where in the world shall we document these, if we document them?
The only section of chapter 9 that seems even a little bit appropriate
is "9.26. System Information Functions and Operators", and even there,
they would need their own new table because they don't fit well in any
existing table.

BTW, does anyone else agree that 9.26 is desperately in need of some
<sect2> subdivisions? It seems to have gotten a lot longer since
I looked at it last.

regards, tom lane

On Wed, Dec 7, 2022 at 8:04 AM Andrew Dunstan <andrew@dunslane.net> wrote:

On 2022-12-07 We 09:20, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Perhaps we should add a type in the regress library that will never have
a safe input function, so we can test that the mechanism works as
expected in that case even after we adjust all the core data types'
input functions.

I was intending that the existing "widget" type be that. 0003 already
adds a comment to widget_in saying not to "fix" its one ereport call.

Yeah, I see that, I must have been insufficiently caffeinated.

Returning to the naming quagmire -- it occurred to me just now that
it might be helpful to call this style of error reporting "soft"
errors rather than "safe" errors, which'd provide a nice contrast
with "hard" errors thrown by longjmp'ing. That would lead to naming
all the variant functions XXXSoft not XXXSafe. There would still
be commentary to the effect that "soft errors must be safe, in the
sense that there's no question whether it's safe to continue
processing the transaction". Anybody think that'd be an
improvement?

I'm not sure InputFunctionCallSoft would be an improvement. Maybe
InputFunctionCallSoftError would be clearer, but I don't know that it's
much of an improvement either. The same goes for the other visible changes.

InputFunctionCallSafe -> TryInputFunctionCall

I think in create type saying "input functions to handle errors softly" is
an improvement over "input functions to return safe errors".

start->save->finish describes a soft error handling procedure quite well.
safe has baggage, all code should be "safe".

fmgr/README: "Handling Non-Exception Errors" -> "Soft Error Handling"

"typical safe error conditions include" -> "error conditions that can be
handled softly include"

(pg_input_is_valid) "input function has been updated to return "safe'
errors" -> "input function has been updated to soft error handling"

Unrelated observation: "Although the error stack is not large, we don't
expect to run out of space." -> "Because the error stack is not large,
assume that we will not run out of space and panic if we are wrong."?

David J.

On Wed, Dec 7, 2022 at 8:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-07 We 09:20, Tom Lane wrote:

Returning to the naming quagmire -- it occurred to me just now that
it might be helpful to call this style of error reporting "soft"
errors rather than "safe" errors, which'd provide a nice contrast
with "hard" errors thrown by longjmp'ing. That would lead to naming
all the variant functions XXXSoft not XXXSafe.

I'm not sure InputFunctionCallSoft would be an improvement.

Yeah, after reflecting on it a bit more I'm not that impressed with
that as a function name either.

(I think that "soft error" could be useful as informal terminology.
AFAIR we don't use "hard error" in any formal way either, but there
are certainly comments using that phrase.)

More questions:

* Anyone want to bikeshed about the new SQL-level function names?
I'm reasonably satisfied with "pg_input_is_valid" for the bool-returning
variant, but not so much with "pg_input_invalid_message" for the
error-message-returning variant. Thinking about "pg_input_error_message"
instead, but that's not stellar either.

Why not do away with two separate functions and define a composite type
(boolean, text) for is_valid to return?

* Where in the world shall we document these, if we document them?
The only section of chapter 9 that seems even a little bit appropriate
is "9.26. System Information Functions and Operators", and even there,
they would need their own new table because they don't fit well in any
existing table.

I would indeed just add a table there.

BTW, does anyone else agree that 9.26 is desperately in need of some
<sect2> subdivisions? It seems to have gotten a lot longer since
I looked at it last.

I'd be inclined to do something like what we are attempting for Chapter 28
Monitoring Database Activity; introduce pagination through refentry and
build our own table of contents into it.

David J.

"David G. Johnston" <david.g.johnston@gmail.com> writes:

Why not do away with two separate functions and define a composite type
(boolean, text) for is_valid to return?

I don't see any advantage to that. It would be harder to use in both
use-cases.

BTW, does anyone else agree that 9.26 is desperately in need of some
<sect2> subdivisions? It seems to have gotten a lot longer since
I looked at it last.

I'd be inclined to do something like what we are attempting for Chapter 28
Monitoring Database Activity; introduce pagination through refentry and
build our own table of contents into it.

I'd prefer to follow the model that already exists in 9.27,
ie break it up with <sect2>'s, which provide a handy
sub-table-of-contents.

regards, tom lane

On Wed, Dec 7, 2022 at 9:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

"David G. Johnston" <david.g.johnston@gmail.com> writes:

Why not do away with two separate functions and define a composite type
(boolean, text) for is_valid to return?

I don't see any advantage to that. It would be harder to use in both
use-cases.

I don't really see a use case for either of them individually. If all you
are doing is printing them out in a test and checking the result in what
situation wouldn't you want to check that both the true/false and message
are as expected? Plus, you don't have to figure out a name for the second
function.

BTW, does anyone else agree that 9.26 is desperately in need of some
<sect2> subdivisions? It seems to have gotten a lot longer since
I looked at it last.

I'd be inclined to do something like what we are attempting for Chapter

28

Monitoring Database Activity; introduce pagination through refentry and
build our own table of contents into it.

I'd prefer to follow the model that already exists in 9.27,
ie break it up with <sect2>'s, which provide a handy
sub-table-of-contents.

I have a bigger issue with the non-pagination myself; the extra bit of
effort to manually create a tabular ToC (where we can add descriptions)
seems like a worthy price to pay.

Are you suggesting we should not go down the path that v8-0003 does in the
monitoring section cleanup thread? I find the usability of Chapter 54
System Views to be superior to these two run-on chapters and would rather
we emulate it in both these places - for what is in the end very little
additional effort, all mechanical in nature.

David J.

"David G. Johnston" <david.g.johnston@gmail.com> writes:

On Wed, Dec 7, 2022 at 9:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

"David G. Johnston" <david.g.johnston@gmail.com> writes:

Why not do away with two separate functions and define a composite type
(boolean, text) for is_valid to return?

I don't see any advantage to that. It would be harder to use in both
use-cases.

I don't really see a use case for either of them individually.

Uh, several people opined that pg_input_is_valid would be of field
interest. If I thought these were only for testing purposes I wouldn't
be especially concerned about documenting them at all.

Are you suggesting we should not go down the path that v8-0003 does in the
monitoring section cleanup thread? I find the usability of Chapter 54
System Views to be superior to these two run-on chapters and would rather
we emulate it in both these places - for what is in the end very little
additional effort, all mechanical in nature.

I have not been following that thread, and am not really excited about
putting in a huge amount of documentation work here. I'd just like 9.26
to have a mini-TOC at the page head, which <sect2>'s would be enough for.

regards, tom lane

On Wed, Dec 7, 2022 at 9:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Perhaps we should add a type in the regress library that will never have
a safe input function, so we can test that the mechanism works as
expected in that case even after we adjust all the core data types'
input functions.

I was intending that the existing "widget" type be that. 0003 already
adds a comment to widget_in saying not to "fix" its one ereport call.

Returning to the naming quagmire -- it occurred to me just now that
it might be helpful to call this style of error reporting "soft"
errors rather than "safe" errors, which'd provide a nice contrast
with "hard" errors thrown by longjmp'ing. That would lead to naming
all the variant functions XXXSoft not XXXSafe. There would still
be commentary to the effect that "soft errors must be safe, in the
sense that there's no question whether it's safe to continue
processing the transaction". Anybody think that'd be an
improvement?

In my attempt to implement CAST...DEFAULT, I noticed that I immediately
needed an
OidInputFunctionCallSafe, which was trivial but maybe something we want to
add to the infra patch, but the comments around that function also somewhat
indicate that we might want to just do the work in-place and call
InputFunctionCallSafe directly. Open to both ideas.

Looking forward cascades up into coerce_type and its brethren, and
reimplementing those from a Node returner to a boolean returner with a Node
parameter seems a bit of a stretch, so I have to pick a point where the
code pivots from passing down a safe-mode indicator and passing back a
found_error indicator (which may be combine-able, as safe is always true
when the found_error pointer is not null, and always false when it isn't),
but for the most part things look do-able.

On Wed, Dec 7, 2022 at 9:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

"David G. Johnston" <david.g.johnston@gmail.com> writes:

Are you suggesting we should not go down the path that v8-0003 does in

the

monitoring section cleanup thread? I find the usability of Chapter 54
System Views to be superior to these two run-on chapters and would rather
we emulate it in both these places - for what is in the end very little
additional effort, all mechanical in nature.

I have not been following that thread, and am not really excited about
putting in a huge amount of documentation work here. I'd just like 9.26
to have a mini-TOC at the page head, which <sect2>'s would be enough for.

So long as you aren't opposed to the idea if someone else does the work,
adding sect2 is better than nothing even if it is just a stop-gap measure.

David J.

On 2022-12-07 09:20:33 -0500, Tom Lane wrote:

Returning to the naming quagmire -- it occurred to me just now that
it might be helpful to call this style of error reporting "soft"
errors rather than "safe" errors, which'd provide a nice contrast
with "hard" errors thrown by longjmp'ing.

+1

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

In my attempt to implement CAST...DEFAULT, I noticed that I immediately
needed an
OidInputFunctionCallSafe, which was trivial but maybe something we want to
add to the infra patch, but the comments around that function also somewhat
indicate that we might want to just do the work in-place and call
InputFunctionCallSafe directly. Open to both ideas.

I'm a bit skeptical of that. IMO using OidInputFunctionCall is only
appropriate in places that will be executed just once per query.
Otherwise, unless you have zero concern for performance, you should
be caching the function lookup. (The test functions in my 0003 patch
illustrate the standard way to do that within SQL-callable functions.
If you're implementing CAST as a new kind of executable expression,
the lookup would likely happen in expression compilation.)

I don't say that OidInputFunctionCallSafe won't ever be useful, but
I doubt it's what we want in CAST.

regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:

So long as you aren't opposed to the idea if someone else does the work,
adding sect2 is better than nothing even if it is just a stop-gap measure.

OK, we can agree on that.

As for the other point --- not sure why I didn't remember this right off,
but the point of two test functions is that one exercises the code path
with details_wanted = true while the other exercises details_wanted =
false. A combined function would only test the first case.

regards, tom lane

Hi,

On 2022-12-06 15:21:09 -0500, Tom Lane wrote:

+{ oid => '8050', descr => 'test whether string is valid input for data type',
+  proname => 'pg_input_is_valid', provolatile => 's', prorettype => 'bool',
+  proargtypes => 'text regtype', prosrc => 'pg_input_is_valid' },
+{ oid => '8051', descr => 'test whether string is valid input for data type',
+  proname => 'pg_input_is_valid', provolatile => 's', prorettype => 'bool',
+  proargtypes => 'text regtype int4', prosrc => 'pg_input_is_valid_mod' },
+{ oid => '8052',
+  descr => 'get error message if string is not valid input for data type',
+  proname => 'pg_input_invalid_message', provolatile => 's',
+  prorettype => 'text', proargtypes => 'text regtype',
+  prosrc => 'pg_input_invalid_message' },
+{ oid => '8053',
+  descr => 'get error message if string is not valid input for data type',
+  proname => 'pg_input_invalid_message', provolatile => 's',
+  prorettype => 'text', proargtypes => 'text regtype int4',
+  prosrc => 'pg_input_invalid_message_mod' },
+

Is there a guarantee that input functions are stable or immutable? We don't
have any volatile input functions in core PG:

SELECT provolatile, count(*) FROM pg_proc WHERE oid IN (SELECT typinput FROM pg_type) GROUP BY provolatile;

Greetings,

Andres Freund

On Wed, Dec 7, 2022 at 10:34 AM Andres Freund <andres@anarazel.de> wrote:

+{ oid => '8053',
+  descr => 'get error message if string is not valid input for data

type',

+  proname => 'pg_input_invalid_message', provolatile => 's',
+  prorettype => 'text', proargtypes => 'text regtype int4',
+  prosrc => 'pg_input_invalid_message_mod' },
+

Is there a guarantee that input functions are stable or immutable? We don't
have any volatile input functions in core PG:

SELECT provolatile, count(*) FROM pg_proc WHERE oid IN (SELECT typinput
FROM pg_type) GROUP BY provolatile;

Effectively yes, though I'm not sure if it is formally documented or
otherwise enforced by the system.

The fact we allow stable is a bit of a sore spot, volatile would be a
terrible property for an I/O function.

David J.

Andres Freund <andres@anarazel.de> writes:

Is there a guarantee that input functions are stable or immutable?

There's a project policy that that should be true. That justifies
marking things like record_in as stable --- if the per-column input
functions could be volatile, record_in would need to be as well.
There are other dependencies on it; see e.g. aab353a60, 3db6524fe.

We don't
have any volatile input functions in core PG:

Indeed, because type_sanity.sql checks that.

regards, tom lane

I wrote:

Andres Freund <andres@anarazel.de> writes:

Is there a guarantee that input functions are stable or immutable?

There's a project policy that that should be true. That justifies
marking things like record_in as stable --- if the per-column input
functions could be volatile, record_in would need to be as well.
There are other dependencies on it; see e.g. aab353a60, 3db6524fe.

I dug in the archives and found the thread leading up to aab353a60:

/messages/by-id/AANLkTik8v7O9QR9jjHNVh62h-COC1B0FDUNmEYMdtKjR@mail.gmail.com

regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:

On Wed, Dec 7, 2022 at 8:04 AM Andrew Dunstan <andrew@dunslane.net> wrote:

I'm not sure InputFunctionCallSoft would be an improvement. Maybe
InputFunctionCallSoftError would be clearer, but I don't know that it's
much of an improvement either. The same goes for the other visible changes.

InputFunctionCallSafe -> TryInputFunctionCall

I think we are already using "TryXXX" for code that involves catching
ereport errors. Since the whole point here is that we are NOT doing
that, I think this naming would be more confusing than helpful.

Unrelated observation: "Although the error stack is not large, we don't
expect to run out of space." -> "Because the error stack is not large,
assume that we will not run out of space and panic if we are wrong."?

That doesn't seem to make the point I wanted to make.

I've adopted your other suggestions in the v4 I'm preparing now.

regards, tom lane

OK, here's a v4 that I think is possibly committable.

I've changed all the comments and docs to use the "soft error"
terminology, but since using "soft" in the actual function names
didn't seem that appealing, they still use "safe".

I already pushed the 0000 elog-refactoring patch, since that seemed
uncontroversial. 0001 attached covers the same territory as before,
but I regrouped the rest so that 0002 installs the new test support
functions, then 0003 adds both the per-datatype changes and
corresponding test cases for bool, int4, arrays, and records.
The idea here is that 0003 can be pointed to as a sample of what
has to be done to datatype input functions, while the preceding
patches can be cited as relevant documentation. (I've not decided
whether to squash 0001 and 0002 together or commit them separately.
Does it make sense to break 0003 into 4 separate commits, or is
that overkill?)

Thoughts?

regards, tom lane

On 2022-12-07 We 17:32, Tom Lane wrote:

OK, here's a v4 that I think is possibly committable.

I've changed all the comments and docs to use the "soft error"
terminology, but since using "soft" in the actual function names
didn't seem that appealing, they still use "safe".

I already pushed the 0000 elog-refactoring patch, since that seemed
uncontroversial. 0001 attached covers the same territory as before,
but I regrouped the rest so that 0002 installs the new test support
functions, then 0003 adds both the per-datatype changes and
corresponding test cases for bool, int4, arrays, and records.
The idea here is that 0003 can be pointed to as a sample of what
has to be done to datatype input functions, while the preceding
patches can be cited as relevant documentation. (I've not decided
whether to squash 0001 and 0002 together or commit them separately.
Does it make sense to break 0003 into 4 separate commits, or is
that overkill?)

No strong opinion about 0001 and 0002. I'm happy enough with them as
they are, but if you want to squash them that's ok. I wouldn't break up
0003. I think we're going to end up committing the remaining work in
batches, although they would probably be a bit more thematically linked
than these.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-07 We 17:32, Tom Lane wrote:

Does it make sense to break 0003 into 4 separate commits, or is
that overkill?)

No strong opinion about 0001 and 0002. I'm happy enough with them as
they are, but if you want to squash them that's ok. I wouldn't break up
0003. I think we're going to end up committing the remaining work in
batches, although they would probably be a bit more thematically linked
than these.

Yeah, we certainly aren't likely to do this work as
one-commit-per-datatype going forward. I'm just wondering
how to do these initial commits so that they provide
good reference material.

regards, tom lane

Hi,

On 2022-12-07 17:32:21 -0500, Tom Lane wrote:

I already pushed the 0000 elog-refactoring patch, since that seemed
uncontroversial. 0001 attached covers the same territory as before,
but I regrouped the rest so that 0002 installs the new test support
functions, then 0003 adds both the per-datatype changes and
corresponding test cases for bool, int4, arrays, and records.
The idea here is that 0003 can be pointed to as a sample of what
has to be done to datatype input functions, while the preceding
patches can be cited as relevant documentation. (I've not decided
whether to squash 0001 and 0002 together or commit them separately.

I think they make sense as is.

Does it make sense to break 0003 into 4 separate commits, or is
that overkill?)

I think it'd be fine either way.

+ * If "context" is an ErrorSaveContext node, but the node creator only wants
+ * notification of the fact of a soft error without any details, just set
+ * the error_occurred flag in the ErrorSaveContext node and return false,
+ * which will cause us to skip the remaining error processing steps.
+ *
+ * Otherwise, create and initialize error stack entry and return true.
+ * Subsequently, errmsg() and perhaps other routines will be called to further
+ * populate the stack entry.  Finally, errsave_finish() will be called to
+ * tidy up.
+ */
+bool
+errsave_start(NodePtr context, const char *domain)

I wonder if there are potential use-cases for levels other than ERROR. I can
potentially see us wanting to defer some FATALs, e.g. when they occur in
process exit hooks.

+{
+	ErrorSaveContext *escontext;
+	ErrorData  *edata;
+
+	/*
+	 * Do we have a context for soft error reporting?  If not, just punt to
+	 * errstart().
+	 */
+	if (context == NULL || !IsA(context, ErrorSaveContext))
+		return errstart(ERROR, domain);
+
+	/* Report that a soft error was detected */
+	escontext = (ErrorSaveContext *) context;
+	escontext->error_occurred = true;
+
+	/* Nothing else to do if caller wants no further details */
+	if (!escontext->details_wanted)
+		return false;
+
+	/*
+	 * Okay, crank up a stack entry to store the info in.
+	 */
+
+	recursion_depth++;
+
+	/* Initialize data for this error frame */
+	edata = get_error_stack_entry();

For a moment I was worried that it could lead to odd behaviour that we don't
do get_error_stack_entry() when !details_wanted, due to not raising an error
we'd otherwise raise. But that's a should-never-be-reached case, so ...

+/*
+ * errsave_finish --- end a "soft" error-reporting cycle
+ *
+ * If errsave_start() decided this was a regular error, behave as
+ * errfinish().  Otherwise, package up the error details and save
+ * them in the ErrorSaveContext node.
+ */
+void
+errsave_finish(NodePtr context, const char *filename, int lineno,
+			   const char *funcname)
+{
+	ErrorSaveContext *escontext = (ErrorSaveContext *) context;
+	ErrorData  *edata = &errordata[errordata_stack_depth];
+
+	/* verify stack depth before accessing *edata */
+	CHECK_STACK_DEPTH();
+
+	/*
+	 * If errsave_start punted to errstart, then elevel will be ERROR or
+	 * perhaps even PANIC.  Punt likewise to errfinish.
+	 */
+	if (edata->elevel >= ERROR)
+	{
+		errfinish(filename, lineno, funcname);
+		pg_unreachable();
+	}

It seems somewhat ugly transport this knowledge via edata->elevel, but it's
not too bad.

+/*
+ * We cannot include nodes.h yet, so make a stub reference.  (This is also
+ * used by fmgr.h, which doesn't want to depend on nodes.h either.)
+ */
+typedef struct Node *NodePtr;

Seems like it'd be easier to just forward declare the struct, and use the
non-typedef'ed name in the header than to have to deal with these
interdependencies and the differing typenames.

+/*----------
+ * Support for reporting "soft" errors that don't require a full transaction
+ * abort to clean up.  This is to be used in this way:
+ *		errsave(context,
+ *				errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ *				errmsg("invalid input syntax for type %s: \"%s\"",
+ *					   "boolean", in_str),
+ *				... other errxxx() fields as needed ...);
+ *
+ * "context" is a node pointer or NULL, and the remaining auxiliary calls
+ * provide the same error details as in ereport().  If context is not a
+ * pointer to an ErrorSaveContext node, then errsave(context, ...)
+ * behaves identically to ereport(ERROR, ...).  If context is a pointer
+ * to an ErrorSaveContext node, then the information provided by the
+ * auxiliary calls is stored in the context node and control returns
+ * normally.  The caller of errsave() must then do any required cleanup
+ * and return control back to its caller.  That caller must check the
+ * ErrorSaveContext node to see whether an error occurred before
+ * it can trust the function's result to be meaningful.
+ *
+ * errsave_domain() allows a message domain to be specified; it is
+ * precisely analogous to ereport_domain().
+ *----------
+ */
+#define errsave_domain(context, domain, ...)	\
+	do { \
+		NodePtr context_ = (context); \
+		pg_prevent_errno_in_scope(); \
+		if (errsave_start(context_, domain)) \
+			__VA_ARGS__, errsave_finish(context_, __FILE__, __LINE__, __func__); \
+	} while(0)

Perhaps worth noting here that the reason why the errsave_start/errsave_finish
split exist differs a bit from the reason in ereport_domain()? "Over there"
it's just about not wanting to incur overhead when the message isn't logged,
but here we'll always have >= ERROR, but ->details_wanted can still lead to
not wanting to incur the overhead.

/*
diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c
index db843a0fbf..bdafcff02d 100644
--- a/src/backend/utils/adt/rowtypes.c
+++ b/src/backend/utils/adt/rowtypes.c
@@ -77,6 +77,7 @@ record_in(PG_FUNCTION_ARGS)
char	   *string = PG_GETARG_CSTRING(0);
Oid			tupType = PG_GETARG_OID(1);
int32		tupTypmod = PG_GETARG_INT32(2);
+	Node	   *escontext = fcinfo->context;
HeapTupleHeader result;
TupleDesc	tupdesc;
HeapTuple	tuple;
@@ -100,7 +101,7 @@ record_in(PG_FUNCTION_ARGS)
* supply a valid typmod, and then we can do something useful for RECORD.
*/
if (tupType == RECORDOID && tupTypmod < 0)
-		ereport(ERROR,
+		ereturn(escontext, (Datum) 0,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("input of anonymous composite types is not implemented")));

Is it ok that we throw an error in lookup_rowtype_tupdesc()? Normally those
should not be reachable by users, I think? The new testing functions might
reach it, but that seems fine, they're test functions.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

I wonder if there are potential use-cases for levels other than ERROR. I can
potentially see us wanting to defer some FATALs, e.g. when they occur in
process exit hooks.

I thought about that early on, and concluded not. The whole thing is
moot for levels less than ERROR, of course, and I'm having a hard
time seeing how it could be useful for FATAL or PANIC. Maybe I just
lack imagination, but if a call is specifying FATAL rather than just
ERROR then it seems to me it's already a special snowflake rather
than something we could fold into a generic non-error behavior.

For a moment I was worried that it could lead to odd behaviour that we don't
do get_error_stack_entry() when !details_wanted, due to not raising an error
we'd otherwise raise. But that's a should-never-be-reached case, so ...

I don't see how. Returning false out of errsave_start causes the
errsave macro to immediately give control back to the caller, which
will go on about its business.

It seems somewhat ugly transport this knowledge via edata->elevel, but it's
not too bad.

The LOG-vs-ERROR business, you mean? Yeah. I considered adding another
bool flag to ErrorData, but couldn't convince myself it was worth the
trouble. If we find a problem we can do that sometime in future.

+/*
+ * We cannot include nodes.h yet, so make a stub reference.  (This is also
+ * used by fmgr.h, which doesn't want to depend on nodes.h either.)
+ */
+typedef struct Node *NodePtr;

Seems like it'd be easier to just forward declare the struct, and use the
non-typedef'ed name in the header than to have to deal with these
interdependencies and the differing typenames.

Meh. I'm a little allergic to writing "struct foo *" in function argument
lists, because I so often see gcc pointing out that if struct foo isn't
yet known then that can silently mean something different than you
intended. With the typedef, it either works or is an error, no halfway
about it. And the struct way isn't really much better in terms of
having two different notations to use rather than only one.

Perhaps worth noting here that the reason why the errsave_start/errsave_finish
split exist differs a bit from the reason in ereport_domain()? "Over there"
it's just about not wanting to incur overhead when the message isn't logged,
but here we'll always have >= ERROR, but ->details_wanted can still lead to
not wanting to incur the overhead.

Hmmm ... it seems like the same reason to me, we don't want to incur the
overhead if the "start" function says not to.

Is it ok that we throw an error in lookup_rowtype_tupdesc()?

Yeah, that should fall in the category of internal errors I think.
I don't see how you'd reach that from a bad input string.

(Or to be more precise, the point of pg_input_is_valid is to tell
you whether the input string is valid, not to tell you whether the
type name is valid; if you're worried about the latter you need
a separate and earlier test.)

regards, tom lane

On Wed, Dec 7, 2022 at 12:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

In my attempt to implement CAST...DEFAULT, I noticed that I immediately
needed an
OidInputFunctionCallSafe, which was trivial but maybe something we want

to

add to the infra patch, but the comments around that function also

somewhat

indicate that we might want to just do the work in-place and call
InputFunctionCallSafe directly. Open to both ideas.

I'm a bit skeptical of that. IMO using OidInputFunctionCall is only
appropriate in places that will be executed just once per query.

That is what's happening when the expr of the existing CAST ( expr AS
typename ) is a constant and we want to just resolve the constant at parse
time.

Show quoted text

On 2022-Dec-07, David G. Johnston wrote:

Are you suggesting we should not go down the path that v8-0003 does in the
monitoring section cleanup thread? I find the usability of Chapter 54
System Views to be superior to these two run-on chapters and would rather
we emulate it in both these places - for what is in the end very little
additional effort, all mechanical in nature.

I think the new 9.26 is much better now than what we had there two days
ago. Maybe it would be even better with your proposed changes, but
let's see what you come up with.

As for Chapter 54, while it's a lot better than what we had previously,
I have a complaint about the new presentation: the overview table
appears (at least in the HTML presentation) in a separate page from the
initial page of the chapter. So to get the intended table of contents I
have to move forward from the unintended table of contents (i.e. from
https://www.postgresql.org/docs/devel/views.html forward to
https://www.postgresql.org/docs/devel/views-overview.html ). This seems
pointless. I think it would be better if we just removed the line
<sect1 id="overview">, which would put that table in the "front page".

I also have an issue with Chapter 28, more precisely 28.2.2, where we
have a similar TOC-style tables (Tables 28.1 and 28.2), but these ones
seem inferior to the new table in Chapter 54 in that the outgoing links
are in random positions in the text of the table. It would be better to
put those in a column of their own, so that they are all vertically
aligned and easier to spot/click. Not sure if you've been here already.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"En las profundidades de nuestro inconsciente hay una obsesiva necesidad
de un universo lógico y coherente. Pero el universo real se halla siempre
un paso más allá de la lógica" (Irulan)

Andres Freund <andres@anarazel.de> writes:

On 2022-12-07 17:32:21 -0500, Tom Lane wrote:

+typedef struct Node *NodePtr;

Seems like it'd be easier to just forward declare the struct, and use the
non-typedef'ed name in the header than to have to deal with these
interdependencies and the differing typenames.

I've been having second thoughts about how to handle this issue.
As we convert more and more datatypes, references to "Node *" are
going to be needed in assorted headers that don't currently have
any reason to #include nodes.h. Rather than bloating their include
footprints, we'll want to use the alternate spelling, whichever
it is. (I already had to do this in array.h.) Some of these headers
might be things that are also read by frontend compiles, in which
case they won't have access to elog.h either, so that NodePtr in
this formulation won't work for them. (I ran into a variant of that
with an early draft of this patch series.)

If we stick with NodePtr we'll probably end by putting that typedef
into c.h so that it's accessible in frontend as well as backend.
I don't have a huge problem with that, but I concede it's a little ugly.

If we go with "struct Node *" then we can solve such problems by
just repeating "struct Node;" forward-declarations in as many
headers as we have to. This is a bit ugly too, but maybe less so,
and it's a method we use elsewhere. The main downside I can see
to it is that we will probably not find out all the places where
we need such declarations until we get field complaints that
"header X doesn't compile for me". elog.h will have a struct Node
declaration, and that will be visible in every backend compilation
we do as well as every cpluspluscheck/headerscheck test.

Another notational point I'm wondering about is whether we want
to create hundreds of direct references to fcinfo->context.
Is it time to invent

#define PG_GET_CONTEXT() (fcinfo->context)

and write that instead in all these input functions?

Thoughts?

regards, tom lane

On Thu, Dec 8, 2022 at 11:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

If we go with "struct Node *" then we can solve such problems by
just repeating "struct Node;" forward-declarations in as many
headers as we have to.

Yes, I think just putting "struct Node;" in as many places as
necessary is the way to go. Or even:

struct Node;
typedef struct Node Node;

....which I think then allows for Node * to be used later.

A small problem with typedef struct Something *SomethingElse is that
it can get hard to keep track of whether some identifier is a pointer
to a struct or just a struct. This doesn't bother me as much as it
does some other hackers, from what I gather anyway, but I think we
should be pretty judicious in using typedef that way. "SomethingPtr"
really has no advantage over "Something *". It is neither shorter nor
clearer.

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

Hi,

On 2022-12-08 16:00:10 -0500, Robert Haas wrote:

On Thu, Dec 8, 2022 at 11:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

If we go with "struct Node *" then we can solve such problems by
just repeating "struct Node;" forward-declarations in as many
headers as we have to.

Yes, I think just putting "struct Node;" in as many places as
necessary is the way to go. Or even:

+1

struct Node;
typedef struct Node Node;

That doesn't work well, because C99 doesn't allow typedefs to be redeclared in
the same scope. IIRC C11 added suppport for it, and a lot of compilers already
supported it before.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2022-12-08 16:00:10 -0500, Robert Haas wrote:

Yes, I think just putting "struct Node;" in as many places as
necessary is the way to go. Or even:

+1

OK, here's a v5 that does it like that.

I've spent a little time pushing ahead on other input functions,
and realized that my original plan to require a pre-encoded typmod
for these test functions was not very user-friendly. So in v5
you can write something like

pg_input_is_valid('1234.567', 'numeric(7,4)')

0004 attached finishes up the remaining core numeric datatypes
(int*, float*, numeric). I ripped out float8in_internal_opt_error
in favor of a function that uses the new APIs.

0005 converts contrib/cube, which I chose to tackle partly because
I'd already touched it in 0004, partly because it seemed like a
good idea to verify that extension modules wouldn't have any
problems with this apprach, and partly because I wondered whether
an input function that uses a Bison/Flex parser would have big
problems getting converted. This one didn't, anyway.

Given that this additional experimentation didn't find any holes
in the API design, I think this is pretty much ready to go.

regards, tom lane

On 2022-12-08 Th 17:57, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2022-12-08 16:00:10 -0500, Robert Haas wrote:

Yes, I think just putting "struct Node;" in as many places as
necessary is the way to go. Or even:

+1

OK, here's a v5 that does it like that.

I've spent a little time pushing ahead on other input functions,
and realized that my original plan to require a pre-encoded typmod
for these test functions was not very user-friendly. So in v5
you can write something like

pg_input_is_valid('1234.567', 'numeric(7,4)')

0004 attached finishes up the remaining core numeric datatypes
(int*, float*, numeric). I ripped out float8in_internal_opt_error
in favor of a function that uses the new APIs.

Great, that takes care of some of the relatively urgent work.

0005 converts contrib/cube, which I chose to tackle partly because
I'd already touched it in 0004, partly because it seemed like a
good idea to verify that extension modules wouldn't have any
problems with this apprach, and partly because I wondered whether
an input function that uses a Bison/Flex parser would have big
problems getting converted. This one didn't, anyway.

Cool

Given that this additional experimentation didn't find any holes
in the API design, I think this is pretty much ready to go.

I will look in more detail tomorrow, but it LGTM on a quick look.

cheers

andrew

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

Hi,

On 2022-12-08 17:57:09 -0500, Tom Lane wrote:

Given that this additional experimentation didn't find any holes
in the API design, I think this is pretty much ready to go.

One interesting area is timestamp / datetime related code. There's been some
past efforts in the area, mostly in 5bc450629b3. See the RETURN_ERROR macro in
formatting.c.

This is not directly about type input functions, but it looks to me that the
functionality in the patchset should work.

I certainly have the hope that it'll make the code look a bit less ugly...

It looks like a fair bit of work to convert this code, so I don't think we
should tie converting formatting.c to the patchset. But it might be a good
idea for Tom to skim the code to see whether there's any things impacting the
design.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2022-12-08 17:57:09 -0500, Tom Lane wrote:

Given that this additional experimentation didn't find any holes
in the API design, I think this is pretty much ready to go.

One interesting area is timestamp / datetime related code. There's been some
past efforts in the area, mostly in 5bc450629b3. See the RETURN_ERROR macro in
formatting.c.
This is not directly about type input functions, but it looks to me that the
functionality in the patchset should work.

Yeah, I was planning to take a look at that before walking away from
this stuff. (I'm sure not volunteering to convert ALL the input
functions, but I'll do the datetime code.)

You're right that formatting.c is doing stuff that's not exactly
an input function, but I don't see why we can't apply the same
API concepts to it.

regards, tom lane

On 2022-12-08 Th 21:59, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2022-12-08 17:57:09 -0500, Tom Lane wrote:

Given that this additional experimentation didn't find any holes
in the API design, I think this is pretty much ready to go.

One interesting area is timestamp / datetime related code. There's been some
past efforts in the area, mostly in 5bc450629b3. See the RETURN_ERROR macro in
formatting.c.
This is not directly about type input functions, but it looks to me that the
functionality in the patchset should work.

Yeah, I was planning to take a look at that before walking away from
this stuff. (I'm sure not volunteering to convert ALL the input
functions, but I'll do the datetime code.)

Awesome. Perhaps if there are no more comments you can commit what you
currently have so people can start work on other input functions.

Thanks for your work on this.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-08 Th 21:59, Tom Lane wrote:

Yeah, I was planning to take a look at that before walking away from
this stuff. (I'm sure not volunteering to convert ALL the input
functions, but I'll do the datetime code.)

Awesome. Perhaps if there are no more comments you can commit what you
currently have so people can start work on other input functions.

Pushed. As I said, I'll take a look at the datetime area. Do we
have any volunteers for other input functions?

regards, tom lane

On 2022-12-09 Fr 10:16, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-08 Th 21:59, Tom Lane wrote:

Yeah, I was planning to take a look at that before walking away from
this stuff. (I'm sure not volunteering to convert ALL the input
functions, but I'll do the datetime code.)

Awesome. Perhaps if there are no more comments you can commit what you
currently have so people can start work on other input functions.

Pushed.

Great!

As I said, I'll take a look at the datetime area. Do we
have any volunteers for other input functions?

I am currently looking at the json types. I think that will be enough to
let us rework the sql/json patches as discussed a couple of months ago.

cheers

andrew

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

On Fri, Dec 9, 2022 at 9:08 PM Andrew Dunstan <andrew@dunslane.net> wrote:

On 2022-12-09 Fr 10:16, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-08 Th 21:59, Tom Lane wrote:

Yeah, I was planning to take a look at that before walking away from
this stuff. (I'm sure not volunteering to convert ALL the input
functions, but I'll do the datetime code.)

Awesome. Perhaps if there are no more comments you can commit what you
currently have so people can start work on other input functions.

Pushed.

Great!

As I said, I'll take a look at the datetime area. Do we
have any volunteers for other input functions?

I am currently looking at the json types. I think that will be enough to
let us rework the sql/json patches as discussed a couple of months ago.

I will pick a few other input functions, thanks.

Regards,
Amul

On Fri, Dec 9, 2022 at 11:17 AM Amul Sul <sulamul@gmail.com> wrote:

On Fri, Dec 9, 2022 at 9:08 PM Andrew Dunstan <andrew@dunslane.net> wrote:

On 2022-12-09 Fr 10:16, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-08 Th 21:59, Tom Lane wrote:

Yeah, I was planning to take a look at that before walking away from
this stuff. (I'm sure not volunteering to convert ALL the input
functions, but I'll do the datetime code.)

Awesome. Perhaps if there are no more comments you can commit what you
currently have so people can start work on other input functions.

Pushed.

Great!

As I said, I'll take a look at the datetime area. Do we
have any volunteers for other input functions?

I am currently looking at the json types. I think that will be enough to
let us rework the sql/json patches as discussed a couple of months ago.

I will pick a few other input functions, thanks.

Regards,
Amul

I can do a few as well, as I need them done for the CAST With Default
effort.

Amul, please let me know which ones you pick so we don't duplicate work.

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-09 Fr 10:16, Tom Lane wrote:

As I said, I'll take a look at the datetime area. Do we
have any volunteers for other input functions?

I am currently looking at the json types. I think that will be enough to
let us rework the sql/json patches as discussed a couple of months ago.

Cool. I've finished up what I wanted to do with the datetime code.

It occurred to me that we're going to have a bit of a problem
with domain_in. We can certainly make it pass back any soft
errors from the underlying type's input function, and we can
make it return a soft error if a domain constraint evaluates
to false. However, what happens if some function in a check
constraint throws an error? Our only hope of trapping that,
given that it's a general user-defined expression, would be
a subtransaction. Which is exactly what we don't want here.

I think though that it might be okay to just define this as
Not Our Problem. Although we don't seem to try to enforce it,
non-immutable domain check constraints are strongly deprecated
(the CREATE DOMAIN man page says that we assume immutability).
And not throwing errors is something that we usually consider
should ride along with immutability. So I think it might be
okay to say "if you want soft error treatment for a domain,
make sure its check constraints don't throw errors".

Thoughts?

regards, tom lane

On 2022-Dec-09, Tom Lane wrote:

I think though that it might be okay to just define this as
Not Our Problem. Although we don't seem to try to enforce it,
non-immutable domain check constraints are strongly deprecated
(the CREATE DOMAIN man page says that we assume immutability).
And not throwing errors is something that we usually consider
should ride along with immutability. So I think it might be
okay to say "if you want soft error treatment for a domain,
make sure its check constraints don't throw errors".

I think that's fine. If the user does, say "CHECK (value > 0)" and that
results in a soft error, that seems to me enough support for now. If
they want to do something more elaborate, they can write C functions.
Maybe eventually we'll want to offer some other mechanism that doesn't
require C, but let's figure out what the requirements are. I don't
think we know that, at this point.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Estoy de acuerdo contigo en que la verdad absoluta no existe...
El problema es que la mentira sí existe y tu estás mintiendo" (G. Lama)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2022-Dec-09, Tom Lane wrote:

... So I think it might be
okay to say "if you want soft error treatment for a domain,
make sure its check constraints don't throw errors".

I think that's fine. If the user does, say "CHECK (value > 0)" and that
results in a soft error, that seems to me enough support for now. If
they want to do something more elaborate, they can write C functions.
Maybe eventually we'll want to offer some other mechanism that doesn't
require C, but let's figure out what the requirements are. I don't
think we know that, at this point.

A fallback we can offer to anyone with such a problem is "write a
plpgsql function and wrap the potentially-failing bit in an exception
block". Then they get to pay the cost of the subtransaction, while
we're not imposing one on everybody else.

regards, tom lane

On 2022-12-09 Fr 10:37, Andrew Dunstan wrote:

I am currently looking at the json types. I think that will be enough to
let us rework the sql/json patches as discussed a couple of months ago.

OK, json is a fairly easy case, see attached. But jsonb is a different
kettle of fish. Both the semantic routines called by the parser and the
subsequent call to JsonbValueToJsonb() can raise errors. These are
pretty much all about breaking various limits (for strings, objects,
arrays). There's also a call to numeric_in, but I assume that a string
that's already parsed as a valid json numeric literal won't upset
numeric_in. Many of these occur several calls down the stack, so
adjusting everything to deal with them would be fairly invasive. Perhaps
we could instead document that this class of input error won't be
trapped, at least for jsonb. We could still test for well-formed jsonb
input, just as I propose for json. That means that we would not be able
to trap one of these errors in the ON ERROR clause of JSON_TABLE. I
think we can probably live with that.

Thoughts?

cheers

andrew

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

so 10. 12. 2022 v 15:35 odesílatel Andrew Dunstan <andrew@dunslane.net>
napsal:

On 2022-12-09 Fr 10:37, Andrew Dunstan wrote:

I am currently looking at the json types. I think that will be enough to
let us rework the sql/json patches as discussed a couple of months ago.

OK, json is a fairly easy case, see attached. But jsonb is a different
kettle of fish. Both the semantic routines called by the parser and the
subsequent call to JsonbValueToJsonb() can raise errors. These are
pretty much all about breaking various limits (for strings, objects,
arrays). There's also a call to numeric_in, but I assume that a string
that's already parsed as a valid json numeric literal won't upset
numeric_in. Many of these occur several calls down the stack, so
adjusting everything to deal with them would be fairly invasive. Perhaps
we could instead document that this class of input error won't be
trapped, at least for jsonb. We could still test for well-formed jsonb
input, just as I propose for json. That means that we would not be able
to trap one of these errors in the ON ERROR clause of JSON_TABLE. I
think we can probably live with that.

Thoughts?

+1

Pavel

Show quoted text

cheers

andrew

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

On Sat, Dec 10, 2022 at 9:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2022-Dec-09, Tom Lane wrote:

... So I think it might be
okay to say "if you want soft error treatment for a domain,
make sure its check constraints don't throw errors".

I think that's fine. If the user does, say "CHECK (value > 0)" and that
results in a soft error, that seems to me enough support for now. If
they want to do something more elaborate, they can write C functions.
Maybe eventually we'll want to offer some other mechanism that doesn't
require C, but let's figure out what the requirements are. I don't
think we know that, at this point.

A fallback we can offer to anyone with such a problem is "write a
plpgsql function and wrap the potentially-failing bit in an exception
block". Then they get to pay the cost of the subtransaction, while
we're not imposing one on everybody else.

regards, tom lane

That exception block will prevent parallel plans. I'm not saying it isn't
the best way forward for us, but wanted to make that side effect clear.

Andrew Dunstan <andrew@dunslane.net> writes:

OK, json is a fairly easy case, see attached. But jsonb is a different
kettle of fish. Both the semantic routines called by the parser and the
subsequent call to JsonbValueToJsonb() can raise errors. These are
pretty much all about breaking various limits (for strings, objects,
arrays). There's also a call to numeric_in, but I assume that a string
that's already parsed as a valid json numeric literal won't upset
numeric_in.

Um, nope ...

regression=# select '1e1000000'::jsonb;
ERROR: value overflows numeric format
LINE 1: select '1e1000000'::jsonb;
^

Many of these occur several calls down the stack, so
adjusting everything to deal with them would be fairly invasive. Perhaps
we could instead document that this class of input error won't be
trapped, at least for jsonb.

Seeing that SQL/JSON is one of the major drivers of this whole project,
it seemed a little sad to me that jsonb couldn't manage to implement
what is required. So I spent a bit of time poking at it. Attached
is an extended version of your patch that also covers jsonb.

The main thing I soon realized is that the JsonSemAction API is based
on the assumption that semantic actions will report errors by throwing
them. This is a bit schizophrenic considering the parser itself carefully
hands back error codes instead of throwing anything (excluding palloc
failures of course). What I propose in the attached is that we change
that API so that action functions return JsonParseErrorType, and add
an enum value denoting "I already logged a suitable error, so you don't
have to". It was a little tedious to modify all the existing functions
that way, but not hard. Only the ones used by jsonb_in need to do
anything except "return JSON_SUCCESS", at least for now.

(I wonder if pg_verifybackup's parse_manifest.c could use a second
look at how it's handling errors, given this API. I didn't study it
closely.)

I have not done anything here about errors within JsonbValueToJsonb.
There would need to be another round of API-extension in that area
if we want to be able to trap its errors. As you say, those are mostly
about exceeding implementation size limits, so I suppose one could argue
that they are not so different from palloc failure. It's still annoying.
If people are good with the changes attached, I might take a look at
that.

regards, tom lane

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

On Sat, Dec 10, 2022 at 9:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

A fallback we can offer to anyone with such a problem is "write a
plpgsql function and wrap the potentially-failing bit in an exception
block". Then they get to pay the cost of the subtransaction, while
we're not imposing one on everybody else.

That exception block will prevent parallel plans. I'm not saying it isn't
the best way forward for us, but wanted to make that side effect clear.

Hmm. Apropos of that, I notice that domain_in is marked PARALLEL SAFE,
which seems like a bad idea if it could invoke not-so-parallel-safe
expressions. Do we need to mark it less safe, and if so how much less?

Anyway, assuming that people are okay with the Not Our Problem approach,
the patch is pretty trivial, as attached. I started to write an addition
to the CREATE DOMAIN man page recommending that domain CHECK constraints
not throw errors, but couldn't get past the bare recommendation. Normally
I'd want to explain such a thing along the lines of "For example, X won't
work" ... but we don't yet have any committed features that depend on
this. I'm inclined to leave it like that for now. If we don't remember
to fix it once we do have some features, I'm sure somebody will ask a
question about it eventually.

regards, tom lane

On 2022-12-10 Sa 14:38, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

OK, json is a fairly easy case, see attached. But jsonb is a different
kettle of fish. Both the semantic routines called by the parser and the
subsequent call to JsonbValueToJsonb() can raise errors. These are
pretty much all about breaking various limits (for strings, objects,
arrays). There's also a call to numeric_in, but I assume that a string
that's already parsed as a valid json numeric literal won't upset
numeric_in.

Um, nope ...

regression=# select '1e1000000'::jsonb;
ERROR: value overflows numeric format
LINE 1: select '1e1000000'::jsonb;
^

Oops, yeah.

Many of these occur several calls down the stack, so
adjusting everything to deal with them would be fairly invasive. Perhaps
we could instead document that this class of input error won't be
trapped, at least for jsonb.

Seeing that SQL/JSON is one of the major drivers of this whole project,
it seemed a little sad to me that jsonb couldn't manage to implement
what is required. So I spent a bit of time poking at it. Attached
is an extended version of your patch that also covers jsonb.

The main thing I soon realized is that the JsonSemAction API is based
on the assumption that semantic actions will report errors by throwing
them. This is a bit schizophrenic considering the parser itself carefully
hands back error codes instead of throwing anything (excluding palloc
failures of course). What I propose in the attached is that we change
that API so that action functions return JsonParseErrorType, and add
an enum value denoting "I already logged a suitable error, so you don't
have to". It was a little tedious to modify all the existing functions
that way, but not hard. Only the ones used by jsonb_in need to do
anything except "return JSON_SUCCESS", at least for now.

Many thanks for doing this, it looks good.

I have not done anything here about errors within JsonbValueToJsonb.
There would need to be another round of API-extension in that area
if we want to be able to trap its errors. As you say, those are mostly
about exceeding implementation size limits, so I suppose one could argue
that they are not so different from palloc failure. It's still annoying.
If people are good with the changes attached, I might take a look at
that.

Awesome.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-10 Sa 14:38, Tom Lane wrote:

Seeing that SQL/JSON is one of the major drivers of this whole project,
it seemed a little sad to me that jsonb couldn't manage to implement
what is required. So I spent a bit of time poking at it. Attached
is an extended version of your patch that also covers jsonb.

Many thanks for doing this, it looks good.

Cool, thanks. Looking at my notes, there's one other loose end
I forgot to mention:

* Note: pg_unicode_to_server() will throw an error for a
* conversion failure, rather than returning a failure
* indication. That seems OK.

We ought to do something about that, but I'm not sure how hard we
ought to work at it. Perhaps it's sufficient to make a variant of
pg_unicode_to_server that just returns true/false instead of failing,
and add a JsonParseErrorType for "untranslatable character" to let
json_errdetail return a reasonably on-point message. We could imagine
extending the ErrorSaveContext infrastructure into the encoding
conversion modules, and maybe at some point that'll be worth doing,
but in this particular context it doesn't seem like we'd be getting
a very much better error message. The main thing that we would get
from such an extension is a chance to capture the report from
report_untranslatable_char. But what that adds is the ability to
identify exactly which character couldn't be translated --- and in
this use-case there's always just one.

regards, tom lane

On 2022-12-10 Sa 19:00, Tom Lane wrote:

Looking at my notes, there's one other loose end
I forgot to mention:

* Note: pg_unicode_to_server() will throw an error for a
* conversion failure, rather than returning a failure
* indication. That seems OK.

We ought to do something about that, but I'm not sure how hard we
ought to work at it. Perhaps it's sufficient to make a variant of
pg_unicode_to_server that just returns true/false instead of failing,
and add a JsonParseErrorType for "untranslatable character" to let
json_errdetail return a reasonably on-point message.

Seems reasonable.

We could imagine
extending the ErrorSaveContext infrastructure into the encoding
conversion modules, and maybe at some point that'll be worth doing,
but in this particular context it doesn't seem like we'd be getting
a very much better error message. The main thing that we would get
from such an extension is a chance to capture the report from
report_untranslatable_char. But what that adds is the ability to
identify exactly which character couldn't be translated --- and in
this use-case there's always just one.

Yeah, probably overkill for now.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-10 Sa 14:38, Tom Lane wrote:

I have not done anything here about errors within JsonbValueToJsonb.
There would need to be another round of API-extension in that area
if we want to be able to trap its errors. As you say, those are mostly
about exceeding implementation size limits, so I suppose one could argue
that they are not so different from palloc failure. It's still annoying.
If people are good with the changes attached, I might take a look at
that.

Awesome.

I spent some time looking at this, and was discouraged to conclude
that the notational mess would probably be substantially out of
proportion to the value. The main problem is that we'd have to change
the API of pushJsonbValue, which has more than 150 call sites, most
of which would need to grow a new test for failure return. Maybe
somebody will feel like tackling that at some point, but not me today.

regards, tom lane

On 2022-12-11 Su 12:24, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-10 Sa 14:38, Tom Lane wrote:

I have not done anything here about errors within JsonbValueToJsonb.
There would need to be another round of API-extension in that area
if we want to be able to trap its errors. As you say, those are mostly
about exceeding implementation size limits, so I suppose one could argue
that they are not so different from palloc failure. It's still annoying.
If people are good with the changes attached, I might take a look at
that.

Awesome.

I spent some time looking at this, and was discouraged to conclude
that the notational mess would probably be substantially out of
proportion to the value. The main problem is that we'd have to change
the API of pushJsonbValue, which has more than 150 call sites, most
of which would need to grow a new test for failure return. Maybe
somebody will feel like tackling that at some point, but not me today.

Yes, I had similar feelings when I looked at it. I don't think this
needs to hold up proceeding with the SQL/JSON rework, which I think can
reasonably restart now.

Maybe as we work through the remaining input functions (there are about
60 core candidates left on my list) we should mark them with a comment
if no adjustment is needed.

I'm going to look at jsonpath and the text types next, I somewhat tied
up this week but might get to relook at pushJsonbValue later in the month.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

Maybe as we work through the remaining input functions (there are about
60 core candidates left on my list) we should mark them with a comment
if no adjustment is needed.

I did a quick pass through them last night. Assuming that we don't
need to touch the unimplemented input functions (eg for pseudotypes),
I count these core functions as still needing work:

aclitemin
bit_in
box_in
bpcharin
byteain
cash_in
cidin
cidr_in
circle_in
inet_in
int2vectorin
jsonpath_in
line_in
lseg_in
macaddr8_in
macaddr_in
multirange_in
namein
oidin
oidvectorin
path_in
pg_lsn_in
pg_snapshot_in
point_in
poly_in
range_in
regclassin
regcollationin
regconfigin
regdictionaryin
regnamespacein
regoperatorin
regoperin
regprocedurein
regprocin
regrolein
regtypein
tidin
tsqueryin
tsvectorin
uuid_in
varbit_in
varcharin
xid8in
xidin
xml_in

and these contrib functions:

hstore:
hstore_in
intarray:
bqarr_in
isn:
ean13_in
isbn_in
ismn_in
issn_in
upc_in
ltree:
ltree_in
lquery_in
ltxtq_in
seg:
seg_in

Maybe we should have a conversation about which of these are
highest priority to get to a credible feature. We clearly need
to fix the remaining SQL-spec types (varchar and bpchar, mainly).
At the other extreme, likely nobody would weep if we never fixed
int2vectorin, for instance.

I'm a little concerned about the cost-benefit of fixing the reg* types.
The ones that accept type names actually use the core grammar to parse
those. Now, we probably could fix the grammar to be non-throwing, but
it'd be very invasive and I'm not sure about the performance impact.
It might be best to content ourselves with soft reporting of lookup
failures, as opposed to syntax problems.

regards, tom lane

Hi,

On 2022-12-11 12:24:11 -0500, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-10 Sa 14:38, Tom Lane wrote:

I have not done anything here about errors within JsonbValueToJsonb.
There would need to be another round of API-extension in that area
if we want to be able to trap its errors. As you say, those are mostly
about exceeding implementation size limits, so I suppose one could argue
that they are not so different from palloc failure. It's still annoying.
If people are good with the changes attached, I might take a look at
that.

Awesome.

I spent some time looking at this, and was discouraged to conclude
that the notational mess would probably be substantially out of
proportion to the value. The main problem is that we'd have to change
the API of pushJsonbValue, which has more than 150 call sites, most
of which would need to grow a new test for failure return. Maybe
somebody will feel like tackling that at some point, but not me today.

Could we address this more minimally by putting the error state into the
JsonbParseState and add a check for that error state to convertToJsonb() or
such (by passing in the JsonbParseState)? We'd need to return immediately in
pushJsonbValue() if there's already an error, but that that's not too bad.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2022-12-11 12:24:11 -0500, Tom Lane wrote:

I spent some time looking at this, and was discouraged to conclude
that the notational mess would probably be substantially out of
proportion to the value. The main problem is that we'd have to change
the API of pushJsonbValue, which has more than 150 call sites, most
of which would need to grow a new test for failure return. Maybe
somebody will feel like tackling that at some point, but not me today.

Could we address this more minimally by putting the error state into the
JsonbParseState and add a check for that error state to convertToJsonb() or
such (by passing in the JsonbParseState)? We'd need to return immediately in
pushJsonbValue() if there's already an error, but that that's not too bad.

We could shoehorn error state into the JsonbParseState, although the
fact that that stack normally starts out empty is a bit of a problem.
I think you'd have to push a dummy entry if you want soft errors,
store the error state pointer into that, and have pushState() copy
down the parent's error pointer. Kind of ugly, but do-able. Whether
it's better than replacing that argument with a pointer-to-struct-
that-includes-the-stack-and-the-error-pointer wasn't real clear to me.

What seemed like a mess was getting the calling code to quit early.
I'm not convinced that just putting an immediate exit into pushJsonbValue
would be enough, because the callers tend to assume a series of calls
will behave as they expect. Probably some of the call sites could
ignore the issue, but you'd still end with a lot of messy changes
I fear.

regards, tom lane

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

Andrew Dunstan <andrew@dunslane.net> writes:

Maybe as we work through the remaining input functions (there are about
60 core candidates left on my list) we should mark them with a comment
if no adjustment is needed.

I did a quick pass through them last night. Assuming that we don't
need to touch the unimplemented input functions (eg for pseudotypes),
I count these core functions as still needing work:

aclitemin
bit_in
box_in
bpcharin
byteain
cash_in
cidin
cidr_in
circle_in
inet_in
int2vectorin
jsonpath_in
line_in
lseg_in
macaddr8_in
macaddr_in

Attaching patches changing these functions except bpcharin,
byteain, jsonpath_in, and cidin. I am continuing work on the next
items below:

multirange_in
namein
oidin
oidvectorin
path_in
pg_lsn_in
pg_snapshot_in
point_in
poly_in
range_in
regclassin
regcollationin
regconfigin
regdictionaryin
regnamespacein
regoperatorin
regoperin
regprocedurein
regprocin
regrolein
regtypein
tidin
tsqueryin
tsvectorin
uuid_in
varbit_in
varcharin
xid8in
xidin
xml_in

and these contrib functions:

hstore:
hstore_in
intarray:
bqarr_in
isn:
ean13_in
isbn_in
ismn_in
issn_in
upc_in
ltree:
ltree_in
lquery_in
ltxtq_in
seg:
seg_in

Maybe we should have a conversation about which of these are
highest priority to get to a credible feature. We clearly need
to fix the remaining SQL-spec types (varchar and bpchar, mainly).
At the other extreme, likely nobody would weep if we never fixed
int2vectorin, for instance.

I'm a little concerned about the cost-benefit of fixing the reg* types.
The ones that accept type names actually use the core grammar to parse
those. Now, we probably could fix the grammar to be non-throwing, but
it'd be very invasive and I'm not sure about the performance impact.
It might be best to content ourselves with soft reporting of lookup
failures, as opposed to syntax problems.

Regards,
Amul

On Tue, Dec 13, 2022 at 6:03 PM Amul Sul <sulamul@gmail.com> wrote:

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

Andrew Dunstan <andrew@dunslane.net> writes:

Maybe as we work through the remaining input functions (there are about
60 core candidates left on my list) we should mark them with a comment
if no adjustment is needed.

I did a quick pass through them last night. Assuming that we don't
need to touch the unimplemented input functions (eg for pseudotypes),
I count these core functions as still needing work:

aclitemin
bit_in
box_in
bpcharin
byteain
cash_in
cidin
cidr_in
circle_in
inet_in
int2vectorin
jsonpath_in
line_in
lseg_in
macaddr8_in
macaddr_in

Attaching patches changing these functions except bpcharin,
byteain, jsonpath_in, and cidin. I am continuing work on the next
items below:

multirange_in
namein
oidin
oidvectorin
path_in
pg_lsn_in
pg_snapshot_in
point_in
poly_in
range_in
regclassin
regcollationin
regconfigin
regdictionaryin
regnamespacein
regoperatorin
regoperin
regprocedurein
regprocin
regrolein
regtypein
tidin
tsqueryin
tsvectorin
uuid_in
varbit_in
varcharin
xid8in
xidin

Attaching a complete set of the patches changing function till this
except bpcharin, byteain jsonpath_in that Andrew is planning to look
in. I have skipped reg* functions.
multirange_in and range_in changes are a bit complicated and big --
planning to resume work on that and the rest of the items in the list
in the last week of this month, thanks.

xml_in

and these contrib functions:

hstore:
hstore_in
intarray:
bqarr_in
isn:
ean13_in
isbn_in
ismn_in
issn_in
upc_in
ltree:
ltree_in
lquery_in
ltxtq_in
seg:
seg_in

Maybe we should have a conversation about which of these are
highest priority to get to a credible feature. We clearly need
to fix the remaining SQL-spec types (varchar and bpchar, mainly).
At the other extreme, likely nobody would weep if we never fixed
int2vectorin, for instance.

I'm a little concerned about the cost-benefit of fixing the reg* types.
The ones that accept type names actually use the core grammar to parse
those. Now, we probably could fix the grammar to be non-throwing, but
it'd be very invasive and I'm not sure about the performance impact.
It might be best to content ourselves with soft reporting of lookup
failures, as opposed to syntax problems.

Regards,
Amul

Amul Sul <sulamul@gmail.com> writes:

Attaching a complete set of the patches changing function till this
except bpcharin, byteain jsonpath_in that Andrew is planning to look
in. I have skipped reg* functions.

I'll take a look at these shortly, unless Andrew is already on it.

regards, tom lane

On 2022-12-14 We 11:00, Tom Lane wrote:

Amul Sul <sulamul@gmail.com> writes:

Attaching a complete set of the patches changing function till this
except bpcharin, byteain jsonpath_in that Andrew is planning to look
in. I have skipped reg* functions.

I'll take a look at these shortly, unless Andrew is already on it.

Thanks, I have been looking at jsonpath, but I'm not quite sure how to
get the escontext argument to the yyerror calls in jsonath_scan.l. Maybe
I need to specify a lex-param setting?

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

Thanks, I have been looking at jsonpath, but I'm not quite sure how to
get the escontext argument to the yyerror calls in jsonath_scan.l. Maybe
I need to specify a lex-param setting?

You want a parse-param option in jsonpath_gram.y, I think; adding that
will persuade Bison to change the signatures of relevant functions.
Compare the mods I made in contrib/cube in ccff2d20e.

regards, tom lane

I wrote:

Amul Sul <sulamul@gmail.com> writes:

Attaching a complete set of the patches changing function till this
except bpcharin, byteain jsonpath_in that Andrew is planning to look
in. I have skipped reg* functions.

I'll take a look at these shortly, unless Andrew is already on it.

I've gone through these now and revised/pushed most of them.

I do not think that we need to touch any unimplemented I/O functions.
They can just as well be unimplemented for soft-error cases too;
I can't see any use-case where it could be useful to have them not
complain. So I discarded

v1-0004-Change-brin_bloom_summary_in-to-allow-non-throw-e.patch
v1-0005-Change-brin_minmax_multi_summary_in-to-allow-non-.patch
v1-0009-Change-gtsvectorin-to-allow-non-throw-error-repor.patch
v1-0018-Change-pg_mcv_list_in-to-allow-non-throw-error-re.patch
v1-0019-Change-pg_ndistinct_in-to-allow-non-throw-error-r.patch

As for the rest, some were closer to being committable than others.
You need to be more careful about handling error cases in subroutines:
you can't just ereturn from a subroutine and figure you're done,
because the caller will keep plugging along if you don't do something
to teach it not to. What that would often lead to is the caller
finding what it thinks is a new error condition, and overwriting the
original message with something that's much less on-point. This is
comparable to cascading errors from a compiler: anybody who's dealt
with those knows that errors after the first one are often just noise.
So we have to be careful to quit after we log the first error.

Also, I ended up ripping out the changes in line_construct, because
as soon as I tried to test them I tripped over the fact that lseg_sl
was still throwing hard errors, before we ever get to line_construct.
Perhaps it is worth fixing all that but I have to deem it very low
priority, because the two-input-points formulation isn't the mainstream
code path. (I kind of wonder too if there isn't a better, more
numerically robust conversion method ...) In any case I'm pretty sure
those changes in float.h would have drawn Andres' ire. We don't want
to be adding arguments to float_overflow_error/float_underflow_error;
if that were acceptable they'd not have looked like that to begin with.

Anyway, thanks for the work! That moved us a good ways.

I think I'm going to go fix bpcharin and varcharin, because those
are the last of the SQL-spec-defined types.

regards, tom lane

Here are some proposed patches for converting range_in and multirange_in.

0001 tackles the straightforward part, which is trapping syntax errors
and called-input-function errors. The only thing that I think might
be controversial here is that I chose to change the signatures of
the exposed functions range_serialize and make_range rather than
inventing xxx_safe variants. I think this is all right, because
AFAIK the only likely reason for extensions to call either of those
is that custom types' canonical functions would need to call
range_serialize --- and those will need to be touched anyway,
see 0002.

What 0001 does not cover is trapping errors occurring in range
canonicalize functions. I'd first thought maybe doing that wasn't
worth the trouble, but it's not really very hard to fix the built-in
canonicalize functions, as shown in 0002. Probably extensions would
not find it much harder, and in any case they're not really required
to make their errors soft.

Any objections?

regards, tom lane

On Thu, Dec 15, 2022 at 9:03 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Here are some proposed patches for converting range_in and multirange_in.

0001 tackles the straightforward part, which is trapping syntax errors
and called-input-function errors. The only thing that I think might
be controversial here is that I chose to change the signatures of
the exposed functions range_serialize and make_range rather than
inventing xxx_safe variants. I think this is all right, because
AFAIK the only likely reason for extensions to call either of those
is that custom types' canonical functions would need to call
range_serialize --- and those will need to be touched anyway,
see 0002.

What 0001 does not cover is trapping errors occurring in range
canonicalize functions. I'd first thought maybe doing that wasn't
worth the trouble, but it's not really very hard to fix the built-in
canonicalize functions, as shown in 0002. Probably extensions would
not find it much harder, and in any case they're not really required
to make their errors soft.

Any objections?

There are other a bunch of hard errors from get_multirange_io_data(),
get_range_io_data() and its subroutine can hit, shouldn't we care
about those?

Regards,
Amul

Amul Sul <sulamul@gmail.com> writes:

There are other a bunch of hard errors from get_multirange_io_data(),
get_range_io_data() and its subroutine can hit, shouldn't we care
about those?

I think those are all "internal" errors, ie not reachable as a
consequence of bad input data. Do you see a reason to think
differently?

regards, tom lane

On Thu, Dec 15, 2022 at 11:16 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Amul Sul <sulamul@gmail.com> writes:

There are other a bunch of hard errors from get_multirange_io_data(),
get_range_io_data() and its subroutine can hit, shouldn't we care
about those?

I think those are all "internal" errors, ie not reachable as a
consequence of bad input data. Do you see a reason to think
differently?

Make sense, I was worried about the internal errors as well as an
error that the user can cause while declaring multi-range e.g. shell
type, but realized that case gets checked at creating that multi-range
type.

Regards,
Amul

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

I've gone through these now and revised/pushed most of them.

Tom, I just want to extend huge thanks to you for working on this
infrastructure. jsonpath aside, I think this is going to pay dividends
in many ways for many years to come. It's something that we've needed
for a really long time, and I'm very happy that we're moving forward
with it.

Thanks so much.

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

Robert Haas <robertmhaas@gmail.com> writes:

Tom, I just want to extend huge thanks to you for working on this
infrastructure.

Thanks. I agree it's an important bit of work.

I'm going to step back from this for now and get on with other work,
but before that I thought there was one more input function I should
look at: xml_in, because xml.c is such a hairy can of worms. It
turns out to be not too bad, given our design principle that only
"bad input" errors should be reported softly. xml_parse() now has
two different ways of reporting errors depending on whether they're
hard or soft, but it didn't take an undue amount of refactoring to
make that work.

While fixing that, my attention was drawn to wellformed_xml(),
whose error handling is unbelievably horrid: it traps any longjmp
whatsoever (query cancel, for instance) and reports it as ill-formed XML.
0002 attached makes use of this new code to get rid of the need for any
PG_TRY there at all; instead, soft errors result in a "false" return
but hard errors are allowed to propagate. xml_is_document was much more
careful, but we can change it the same way to save code and cycles.

regards, tom lane

I wrote:

I'm going to step back from this for now and get on with other work,
but before that I thought there was one more input function I should
look at: xml_in, because xml.c is such a hairy can of worms.

Pushed that. For the record, my list of input functions still needing
attention stands at

Core:

jsonpath_in
regclassin
regcollationin
regconfigin
regdictionaryin
regnamespacein
regoperatorin
regoperin
regprocedurein
regprocin
regrolein
regtypein
tsqueryin
tsvectorin

Contrib:

hstore:
hstore_in
intarray:
bqarr_in
isn:
ean13_in
isbn_in
ismn_in
issn_in
upc_in
ltree:
ltree_in
lquery_in
ltxtq_in
seg:
seg_in

The reg* functions probably need a unified plan as to how far
down we want to push non-error behavior. The rest of these
I think just require turning the crank along the same lines
as in functions already dealt with.

While it'd be good to get all of these done before v16 feature
freeze, I can't see that any of them represent blockers for
building features based on soft input error handling.

regards, tom lane

On 2022-12-15 Th 09:12, Robert Haas wrote:

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

I've gone through these now and revised/pushed most of them.

Tom, I just want to extend huge thanks to you for working on this
infrastructure. jsonpath aside, I think this is going to pay dividends
in many ways for many years to come. It's something that we've needed
for a really long time, and I'm very happy that we're moving forward
with it.

Thanks so much.

Robert beat me to it, but I will heartily second this. Many thanks.

cheers

andrew

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

On 2022-12-14 We 17:37, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Thanks, I have been looking at jsonpath, but I'm not quite sure how to
get the escontext argument to the yyerror calls in jsonath_scan.l. Maybe
I need to specify a lex-param setting?

You want a parse-param option in jsonpath_gram.y, I think; adding that
will persuade Bison to change the signatures of relevant functions.
Compare the mods I made in contrib/cube in ccff2d20e.

Yeah, I started there, but it's substantially more complex - unlike cube
the jsonpath scanner calls the error routines as well as the parser.

Anyway, here's a patch.

cheers

andrew

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

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

The reg* functions probably need a unified plan as to how far
down we want to push non-error behavior. The rest of these
I think just require turning the crank along the same lines
as in functions already dealt with.

I would be in favor of an aggressive approach. For example, let's look
at regclassin(). It calls oidin(), stringToQualifiedNameList(),
makeRangeVarFromNameList(), and RangeVarGetRelidExtended(). Basically,
oidin() could fail if the input, known to be all digits, is out of
range; stringToQualifiedNameList() could fail due to mismatched
delimiters or improperly-separated names; makeRangeVarFromNameList()
doesn't want to have more than three name components
(db.schema.relation); and RangeVarGetRelidExtended() doesn't like
cross-database references or non-existent relations.

Now, one option here would be to distinguish between something that
could be valid in some database but isn't in this one, like a
non-existent relation name, and one that couldn't ever work anywhere,
like a relation name with four parts or bad quoting. You could decide
that the former kind of error will be reported softly but the latter
is hard error. But I think that is presuming that we know how users
will want to use this functionality, and I don't think we do. I also
think that it will be confusing to users. Finally, I think it's
different from what we do for other data types. You could equally well
argue that, for int4in, we ought to treat '9999999999' and 'potato'
differently, one a hard error and the other soft. I think it's hard to
puzzle out a decision that makes any sense there, and I don't think
this case is much different. I don't think it's too hard to mentally
separate errors about the validity of the input from, say, out of
memory errors -- but one distinguishing between one kind of input
validity check and another seems like a muddle.

It also doesn't seem too bad from an implementation point of view to
try to cover all the caes. The stickiest case looks to be
RangeVarGetRelidExtended() and we might need to give a bit of thought
to how to handle that one. The others don't seem like a big issue, and
oidin() is already done.

While it'd be good to get all of these done before v16 feature
freeze, I can't see that any of them represent blockers for
building features based on soft input error handling.

+1.

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

Robert Haas <robertmhaas@gmail.com> writes:

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

The reg* functions probably need a unified plan as to how far
down we want to push non-error behavior. The rest of these
I think just require turning the crank along the same lines
as in functions already dealt with.

I would be in favor of an aggressive approach.

I agree that anything based on implementation concerns is going
to look pretty unprincipled to end users. However ...

It also doesn't seem too bad from an implementation point of view to
try to cover all the caes.

... I guess you didn't read my remarks upthread about regtypein.
I do not want to try to make gram.y+scan.l non-error-throwing.

regards, tom lane

On Mon, Dec 19, 2022 at 11:44 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

It also doesn't seem too bad from an implementation point of view to
try to cover all the caes.

... I guess you didn't read my remarks upthread about regtypein.
I do not want to try to make gram.y+scan.l non-error-throwing.

Huh, for some reason I'm not seeing an email about that. Do you have a link?

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

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Dec 19, 2022 at 11:44 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

... I guess you didn't read my remarks upthread about regtypein.
I do not want to try to make gram.y+scan.l non-error-throwing.

Huh, for some reason I'm not seeing an email about that. Do you have a link?

In [1]/messages/by-id/1863335.1670783397@sss.pgh.pa.us I wrote

I'm a little concerned about the cost-benefit of fixing the reg* types.
The ones that accept type names actually use the core grammar to parse
those. Now, we probably could fix the grammar to be non-throwing, but
it'd be very invasive and I'm not sure about the performance impact.
It might be best to content ourselves with soft reporting of lookup
failures, as opposed to syntax problems.

regards, tom lane

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

On Mon, Dec 19, 2022 at 4:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

In [1] I wrote

I'm a little concerned about the cost-benefit of fixing the reg* types.
The ones that accept type names actually use the core grammar to parse
those. Now, we probably could fix the grammar to be non-throwing, but
it'd be very invasive and I'm not sure about the performance impact.
It might be best to content ourselves with soft reporting of lookup
failures, as opposed to syntax problems.

Ah right. I agree that invading the main grammar doesn't seem
terribly appealing. Setting regtypein aside could be a sensible
choice, then. Another option might be to have some way of parsing type
names outside of the main grammar, which would be more work and would
require keeping things in sync, but perhaps it would end up being less
ugly....

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

On 2022-12-18 Su 09:42, Andrew Dunstan wrote:

On 2022-12-14 We 17:37, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Thanks, I have been looking at jsonpath, but I'm not quite sure how to
get the escontext argument to the yyerror calls in jsonath_scan.l. Maybe
I need to specify a lex-param setting?

You want a parse-param option in jsonpath_gram.y, I think; adding that
will persuade Bison to change the signatures of relevant functions.
Compare the mods I made in contrib/cube in ccff2d20e.

Yeah, I started there, but it's substantially more complex - unlike cube
the jsonpath scanner calls the error routines as well as the parser.

Anyway, here's a patch.

And here's another for contrib/seg

I'm planning to commit these two in the next day or so.

cheers

andew

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

Andrew Dunstan <andrew@dunslane.net> writes:

And here's another for contrib/seg
I'm planning to commit these two in the next day or so.

I didn't look at the jsonpath one yet. The seg patch passes
an eyeball check, with one minor nit: in seg_atof,

+ *result = float4in_internal(value, NULL, "real", value, escontext);

don't we want to use "seg" as the type_name?

Even more nitpicky, in

+seg_yyerror(SEG *result, struct Node *escontext, const char *message)
 {
+	if (SOFT_ERROR_OCCURRED(escontext))
+		return;

I'd be inclined to add some explanation, say

+seg_yyerror(SEG *result, struct Node *escontext, const char *message)
 {
+	/* if we already reported an error, don't overwrite it */
+	if (SOFT_ERROR_OCCURRED(escontext))
+		return;

regards, tom lane

Andrew Dunstan <andrew@dunslane.net> writes:

Yeah, I started there, but it's substantially more complex - unlike cube
the jsonpath scanner calls the error routines as well as the parser.
Anyway, here's a patch.

I looked through this and it seems generally OK. A minor nitpick is
that we usually write "(Datum) 0" not "(Datum) NULL" for dont-care Datum
values. A slightly bigger issue is that makeItemLikeRegex still allows
an error to be thrown from RE_compile_and_cache if a bogus regex is
presented. But that could be dealt with later.

(I wonder why this is using RE_compile_and_cache at all, really,
rather than some other API. There doesn't seem to be value in
forcing the regex into the cache at this point.)

regards, tom lane

On 2022-12-22 Th 01:10, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

And here's another for contrib/seg
I'm planning to commit these two in the next day or so.

I didn't look at the jsonpath one yet. The seg patch passes
an eyeball check, with one minor nit: in seg_atof,

+ *result = float4in_internal(value, NULL, "real", value, escontext);

don't we want to use "seg" as the type_name?

Even more nitpicky, in

+seg_yyerror(SEG *result, struct Node *escontext, const char *message)
{
+	if (SOFT_ERROR_OCCURRED(escontext))
+		return;

I'd be inclined to add some explanation, say

+seg_yyerror(SEG *result, struct Node *escontext, const char *message)
{
+	/* if we already reported an error, don't overwrite it */
+	if (SOFT_ERROR_OCCURRED(escontext))
+		return;

Thanks for the review.

Fixed both of these and pushed.

cheers

andrew

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

On 2022-12-22 Th 11:44, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Yeah, I started there, but it's substantially more complex - unlike cube
the jsonpath scanner calls the error routines as well as the parser.
Anyway, here's a patch.

I looked through this and it seems generally OK. A minor nitpick is
that we usually write "(Datum) 0" not "(Datum) NULL" for dont-care Datum
values.

Fixed in the new version attached.

A slightly bigger issue is that makeItemLikeRegex still allows
an error to be thrown from RE_compile_and_cache if a bogus regex is
presented. But that could be dealt with later.

I'd rather fix it now while we're paying attention.

(I wonder why this is using RE_compile_and_cache at all, really,
rather than some other API. There doesn't seem to be value in
forcing the regex into the cache at this point.)

I agree. The attached uses pg_regcomp instead. I had a lift a couple of
lines from regexp.c, but not too many.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-22 Th 11:44, Tom Lane wrote:

(I wonder why this is using RE_compile_and_cache at all, really,
rather than some other API. There doesn't seem to be value in
forcing the regex into the cache at this point.)

I agree. The attached uses pg_regcomp instead. I had a lift a couple of
lines from regexp.c, but not too many.

LGTM. No further comments.

regards, tom lane

On Fri, Dec 23, 2022 at 9:20 AM Andrew Dunstan <andrew@dunslane.net> wrote:

On 2022-12-22 Th 11:44, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Yeah, I started there, but it's substantially more complex - unlike cube
the jsonpath scanner calls the error routines as well as the parser.
Anyway, here's a patch.

I looked through this and it seems generally OK. A minor nitpick is
that we usually write "(Datum) 0" not "(Datum) NULL" for dont-care Datum
values.

Fixed in the new version attached.

A slightly bigger issue is that makeItemLikeRegex still allows
an error to be thrown from RE_compile_and_cache if a bogus regex is
presented. But that could be dealt with later.

I'd rather fix it now while we're paying attention.

(I wonder why this is using RE_compile_and_cache at all, really,
rather than some other API. There doesn't seem to be value in
forcing the regex into the cache at this point.)

I agree. The attached uses pg_regcomp instead. I had a lift a couple of
lines from regexp.c, but not too many.

cheers

andrew

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

Hi,
In makeItemLikeRegex :

+                       /* See regexp.c for explanation */
+                       CHECK_FOR_INTERRUPTS();
+                       pg_regerror(re_result, &re_tmp, errMsg,
sizeof(errMsg));
+                       ereturn(escontext, false,

Since an error is returned, I wonder if the `CHECK_FOR_INTERRUPTS` call is
still necessary.

Cheers

Ted Yu <yuzhihong@gmail.com> writes:

In makeItemLikeRegex :

+                       /* See regexp.c for explanation */
+                       CHECK_FOR_INTERRUPTS();
+                       pg_regerror(re_result, &re_tmp, errMsg,
sizeof(errMsg));
+                       ereturn(escontext, false,

Since an error is returned, I wonder if the `CHECK_FOR_INTERRUPTS` call is
still necessary.

Yes, it is. We don't want a query-cancel transformed into a soft error.

regards, tom lane

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

Ted Yu <yuzhihong@gmail.com> writes:

In makeItemLikeRegex :

+                       /* See regexp.c for explanation */
+                       CHECK_FOR_INTERRUPTS();
+                       pg_regerror(re_result, &re_tmp, errMsg,
sizeof(errMsg));
+                       ereturn(escontext, false,

Since an error is returned, I wonder if the `CHECK_FOR_INTERRUPTS` call

is

still necessary.

Yes, it is. We don't want a query-cancel transformed into a soft error.

regards, tom lane

Hi,
`ereturn(escontext` calls appear in multiple places in the patch.
What about other callsites (w.r.t. checking interrupt) ?

Cheers

Ted Yu <yuzhihong@gmail.com> writes:

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

Ted Yu <yuzhihong@gmail.com> writes:

+                       /* See regexp.c for explanation */
+                       CHECK_FOR_INTERRUPTS();

Yes, it is. We don't want a query-cancel transformed into a soft error.

`ereturn(escontext` calls appear in multiple places in the patch.
What about other callsites (w.r.t. checking interrupt) ?

What about them? The reason this one is special is that backend/regexp
might return a failure code that's specifically "I gave up because
there's a query cancel pending". We don't want to report that as a soft
error. It's true that we might cancel the query for real a bit later on
even if this check weren't here, but that doesn't mean it's okay to go
down the soft error path and hope that there'll be a CHECK_FOR_INTERRUPTS
sometime before there's any visible evidence that we did the wrong thing.

regards, tom lane

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

Ted Yu <yuzhihong@gmail.com> writes:

In makeItemLikeRegex :

+                       /* See regexp.c for explanation */
+                       CHECK_FOR_INTERRUPTS();
+                       pg_regerror(re_result, &re_tmp, errMsg,
sizeof(errMsg));
+                       ereturn(escontext, false,

Since an error is returned, I wonder if the `CHECK_FOR_INTERRUPTS` call

is

still necessary.

Yes, it is. We don't want a query-cancel transformed into a soft error.

regards, tom lane

Hi,
For this case (`invalid regular expression`), the potential user
interruption is one reason for stopping execution.
I feel surfacing user interruption somehow masks the underlying error.

The same regex, without user interruption, would exhibit an `invalid
regular expression` error.
I think it would be better to surface the error.

Cheers

On 2022-12-24 Sa 04:51, Ted Yu wrote:

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

Ted Yu <yuzhihong@gmail.com> writes:

In makeItemLikeRegex :

+                       /* See regexp.c for explanation */
+                       CHECK_FOR_INTERRUPTS();
+                       pg_regerror(re_result, &re_tmp, errMsg,
sizeof(errMsg));
+                       ereturn(escontext, false,

Since an error is returned, I wonder if the

`CHECK_FOR_INTERRUPTS` call is

still necessary.

Yes, it is.  We don't want a query-cancel transformed into a soft
error.

                        regards, tom lane

Hi,
For this case (`invalid regular expression`), the potential user
interruption is one reason for stopping execution.
I feel surfacing user interruption somehow masks the underlying error.

The same regex, without user interruption, would exhibit an `invalid
regular expression` error.
I think it would be better to surface the error.

All that this patch is doing is replacing a call to
RE_compile_and_cache, which calls CHECK_FOR_INTERRUPTS, with similar
code, which gives us the opportunity to call ereturn instead of ereport.
Note that where escontext is NULL (the common case), ereturn functions
identically to ereport. So unless you want to argue that the logic in
RE_compile_and_cache is wrong I don't see what we're arguing about. If
instead I had altered the API of RE_compile_and_cache to include an
escontext parameter we wouldn't be having this argument at all. The only
reason I didn't do that was the point Tom quite properly raised about
why we're doing any caching here anyway.

cheers

andrew

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

On 2022-12-23 Fr 13:53, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-12-22 Th 11:44, Tom Lane wrote:

(I wonder why this is using RE_compile_and_cache at all, really,
rather than some other API. There doesn't seem to be value in
forcing the regex into the cache at this point.)

I agree. The attached uses pg_regcomp instead. I had a lift a couple of
lines from regexp.c, but not too many.

LGTM. No further comments.

As I was giving this a final polish I noticed this in jspConvertRegexFlags:

    /*
     * We'll never need sub-match details at execution.  While
     * RE_compile_and_execute would set this flag anyway, force it on
here to
     * ensure that the regex cache entries created by makeItemLikeRegex are
     * useful.
     */
    cflags |= REG_NOSUB;

Clearly the comment would no longer be true. I guess I should just
remove this?

cheers

andrew

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

On Sat, Dec 24, 2022 at 4:38 AM Andrew Dunstan <andrew@dunslane.net> wrote:

On 2022-12-24 Sa 04:51, Ted Yu wrote:

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

Ted Yu <yuzhihong@gmail.com> writes:

In makeItemLikeRegex :

+                       /* See regexp.c for explanation */
+                       CHECK_FOR_INTERRUPTS();
+                       pg_regerror(re_result, &re_tmp, errMsg,
sizeof(errMsg));
+                       ereturn(escontext, false,

Since an error is returned, I wonder if the

`CHECK_FOR_INTERRUPTS` call is

still necessary.

Yes, it is. We don't want a query-cancel transformed into a soft
error.

regards, tom lane

Hi,
For this case (`invalid regular expression`), the potential user
interruption is one reason for stopping execution.
I feel surfacing user interruption somehow masks the underlying error.

The same regex, without user interruption, would exhibit an `invalid
regular expression` error.
I think it would be better to surface the error.

All that this patch is doing is replacing a call to
RE_compile_and_cache, which calls CHECK_FOR_INTERRUPTS, with similar
code, which gives us the opportunity to call ereturn instead of ereport.
Note that where escontext is NULL (the common case), ereturn functions
identically to ereport. So unless you want to argue that the logic in
RE_compile_and_cache is wrong I don't see what we're arguing about. If
instead I had altered the API of RE_compile_and_cache to include an
escontext parameter we wouldn't be having this argument at all. The only
reason I didn't do that was the point Tom quite properly raised about
why we're doing any caching here anyway.

cheers

andrew

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

Andrew:

Thanks for the response.

Andrew Dunstan <andrew@dunslane.net> writes:

As I was giving this a final polish I noticed this in jspConvertRegexFlags:

    /*
     * We'll never need sub-match details at execution.  While
     * RE_compile_and_execute would set this flag anyway, force it on here to
     * ensure that the regex cache entries created by makeItemLikeRegex are
     * useful.
     */
    cflags |= REG_NOSUB;

Clearly the comment would no longer be true. I guess I should just
remove this?

Yeah, we can just drop that I guess. I'm slightly worried that we might
need it again after some future refactoring; but it's not really worth
devising a re-worded comment to justify keeping it.

Also, I realized that I failed in my reviewerly duty by not noticing
that you'd forgotten to pg_regfree the regex after successful
compilation. Running something like this exposes the memory leak
very quickly:

select pg_input_is_valid('$ ? (@ like_regex "pattern" flag "smixq")', 'jsonpath')
from generate_series(1,10000000);

The attached delta patch takes care of it. (Per comment at pg_regcomp,
we don't need this after a failure return.)

regards, tom lane

Ted Yu <yuzhihong@gmail.com> writes:

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

Yes, it is. We don't want a query-cancel transformed into a soft error.

The same regex, without user interruption, would exhibit an `invalid
regular expression` error.

On what grounds do you claim that? The timing of arrival of the SIGINT
is basically chance --- it might happen while we're inside backend/regex,
or not. I mean, sure you could claim that a bad regex might run a long
time and thereby be more likely to cause the user to issue a query
cancel, but that's a stretched line of reasoning.

regards, tom lane

On 2022-12-24 Sa 10:42, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

As I was giving this a final polish I noticed this in jspConvertRegexFlags:
    /*
     * We'll never need sub-match details at execution.  While
     * RE_compile_and_execute would set this flag anyway, force it on here to
     * ensure that the regex cache entries created by makeItemLikeRegex are
     * useful.
     */
    cflags |= REG_NOSUB;
Clearly the comment would no longer be true. I guess I should just
remove this?

Yeah, we can just drop that I guess. I'm slightly worried that we might
need it again after some future refactoring; but it's not really worth
devising a re-worded comment to justify keeping it.

Also, I realized that I failed in my reviewerly duty by not noticing
that you'd forgotten to pg_regfree the regex after successful
compilation. Running something like this exposes the memory leak
very quickly:

select pg_input_is_valid('$ ? (@ like_regex "pattern" flag "smixq")', 'jsonpath')
from generate_series(1,10000000);

The attached delta patch takes care of it. (Per comment at pg_regcomp,
we don't need this after a failure return.)

Thanks, pushed with those changes.

cheers

andrew

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

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Dec 16, 2022 at 1:31 PM Tom Lane <tgl@sss.pgh.pa.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

I got annoyed by the fact that types cid, xid, xid8 don't throw
error even for obvious garbage, because they just believe the
result of strtoul or strtoull without any checking. That was
probably up to project standards when cidin and xidin were
written; but surely it's not anymore, especially when we can
piggyback on work already done for type oid.

Anybody have an objection to the following? One note is that
because we already had test cases checking that xid would
accept hex input, I made the common subroutines use "0" not
"10" for strtoul's last argument, meaning that oid will accept
hex now too.

regards, tom lane

On 2022-12-25 Su 12:13, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Dec 16, 2022 at 1:31 PM Tom Lane <tgl@sss.pgh.pa.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.

It's a judgement call, but I'm not too fussed about stopping here for
v16. I see the reg* items as probably the lowest priority to fix.

cheers

andrew

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