I have committed patches v23 0001-0003 and 0005-0016.
In patch 0003, I did not change the NO_XML_SUPPORT() in xmlparse(). I
don't think we should make lack of compiled-in XML support a soft
error.
In patch 0012, I did not apply the changes involving
float_*flow_error() in dtof(). These should be considered together
with the error handling changes in patch 0018 (about which below).
Patch 0004 is probably ok, I just need to study a bit more, since it's
a bit different from the other ones.
In the comment for patch 0008 you write that supporting soft errors in
int4_cash() is not easy. I suppose this is because of the external
function call to int8mul()? Maybe that was necessary in ancient
times, but int8mul() is nowadays just a fmgr wrapper around
pg_mul_s64_overflow(), which you can call directly in int4_cash(), and
then it should be easy.
About patch 0017 (jsonb): Calling ereturn() in a function that returns
void, well, you make that work, but it seems a bit weird. Maybe
cannotCastJsonbValue() could be changed to return a dummy datum, and
then you could just call it like
return cannotCastJsonbValue(...);
Similarly, about patch 0018 (refactor float error): I think this is a
fragile API. float_overflow_error() now will either ereport or just
errsave. But nothing enforces that callers do the right thing. For
example, in patch 0019, you change float_zero_divide_error(NULL) to
float_zero_divide_error(escontext), but don't insert a return, so the
code will continue to perform the division. Similar to patch 0017,
maybe these functions should be changed to have a non-void return
value. Or maybe make separate functions to be used in places where we
want soft error handling.
In the comment for patch 0021 you write that the function casting type
circle to type polygon cannot be error safe, because it's a SQL
language function. I suggest to convert this to a C function and make
the required changes there.
About the main feature patch: I'm not a fan of the CREATE CAST
... WITH SAFE FUNCTION syntax. First, the CREATE CAST syntax is part
of the SQL standard. It would be weird if we needed a nonstandard
syntax to make two standard features work together. Second, we didn't
do this when we introduced error-safe type input functions. There is
a note on the CREATE TYPE man page that use of ereturn() is
encouraged, and then we left it to extension authors to do the right
thing. (And we should now put a similar note on the CREATE CAST man
page.) And third, requiring this would require a lot of churn in all
affected extensions, requiring new extension SQL files and forcing
upgrades. The changes you did in patch 0023 don't do this correctly,
for example.
Tactical suggestion: Add an SQL-callable function, say,
pg_cast_conversion_succeeds(srcvalue, desttype) that checks whether
the cast would succeed. This would be similar to the
pg_input_is_valid() function that we added to test the type input
functions. (I did not call my proposal pg_cast_is_valid() because
that might indicate merely that a casting path exists.) With that,
the higher-level functionality can be constructed by hand (CASE WHEN
pg_cast_conversion_succeeds(...) THEN CAST(...) ELSE 'default value'
END). And then we can later work on building out the higher-level
functionality and make more cast functions error safe.
