Re: CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions

Thank you for your continued work on this patch.

To support CAST('error' AS integer DEFAULT 1 ON CONVERSION ERROR), the
> conversion of an unknown literal to a Const, done inside
> coerce_to_target_type,
> must be able to fail softly. Passing an ErrorSaveContext pointer down
> through
> those functions is ugly and not future-proof, so adding a p_escontext
> field to
> struct ParseState. This also lets other parse-analysis code do error-safe
> conversions the same way, if they want.
>

I suspect that this may be the cause of the d_nota test results at the
bottom of this review.

>
> 4. extensive changes in clauses.c also required.
>
> Rationale:
> CAST(('abc'::text)::int + abs(NULL) AS int DEFAULT 3 ON CONVERSION
> ERROR)
>
> To make the above query does not fail, which is what we want (at least, I
> think
> so), we need to make evaluate_expr handle soft errors.
> This means evaluate_expr needs an ErrorSaveContext node. As a result,
> we’ll need
> to refactor every occurrence of ece_evaluate_expr.
>

I disagree on this point. I believe that the leading a_expr

('abc'::text)::int

should fail. We're looking for conversion errors on the cast operation
itself, not before.

REVIEW

Sorry for the delays in reviewing this. It's been a while, so I tried to
give everything a fresh look. As such, I had to fall back to an earlier
commit to get it to apply. My first attempt was c9c660e6ae03, which I chose
it because the commit date was near the patch date, and since the patch
applied clean I didn't go looking for a better one, knowing that I'd have
to rebase.

Applies over c9c660e6ae03 and cleanly rebases to master, passes.

0001
====

Everything is clear here, no notes.

0002
====

Overall the new ERROR SAFE/UNSAFE configuration is exactly what I had hoped
for, it simplifies whether a cast can be made safe or not, allows types
from extensions and other custom datatypes to continue to function as-is,
and be able to use the on-error-default functionality as soon as they make
themselves (soft) error-safe.

I see that there is nothing stopping language SQL and language plpgsql
functions from being declared error safe, even though there is currently no
mechanism for making them functionally error safe. I don't think this is a
problem in the short term, as this safe/unsafe check is initially used only
for cast functions, so for the moment I'm ok with allowing this impossible
assertion to be made. It isn't that much different than our ability to
declare a function immutable with no check as to whether it actually is
immutable. Plus, it opens the possiblity of adding such functionality to
non-C functions later on.

I like the test that looks for any builtin input functions that are not
declared error safe. For the time being, I would suggest temporarily adding
another test looking for any functions that are declared error safe but are
not input/cast related. Obviously this proposed second check will outlive
its usefulness once we find a second use for error safe declarations, so it
may only make sense for the committer's peace of mind during testing and be
discarded thereafter.

0003
====

- The tests on bpchar are good

doc/src/sgml/syntax.sgml
----------------------------------

+ A type cast may occasionally fail at run time. To guard against such
+ failures, you can provide an <literal>ON CONVERSION ERROR</literal>
+ clause to control what happens when a cast fails. The available forms
+ are:

The wording here is awkward. The word "may occasionaly fail" implies that
success or failure of a cast is not deterministic or depends on some input
other than the value to be cast.

I suggest replacing

A type cast may occasionally fail at run time.

with

A type cast will fail if the input value cannot be resolved to a value
of the destination type.

or

A type cast will fail if the input value has no equivalent value of the
destination type.

+ With <literal>DEFAULT <replaceable>expression</replaceable> ON
CONVERSION ERROR</literal>,
+ the default <replaceable>expression</replaceable> is evaluated and
+ returned instead. The <literal>ERROR ON CONVERSION ERROR</literal>
+ form raises an error on failure, which is also the default behavior
+ when no <literal>ON CONVERSION ERROR</literal> clause is specified.
+ Note: The <literal>DEFAULT</literal>
<replaceable>expression</replaceable>
+ is itself evaluated normally, so it may also raise an error during
that evaluation.

I suggest:

When <literal>DEFAULT <replaceable>expression</replaceable> ON
CONVERSION ERROR</literal>
is specified, if the first <replaceable>expression</replaceable> cannot
be cast to the destination type,
then the specified <literal>DEFAULT</literal>
<replaceable>expression</replaceable>
is evaluated and cast to the destination type, which itself will raise
an error if the value cannot
be cast to the destination type.

src/backend/executor/execExpr.c
--------------------------------------------

+ fusage = (pgstat_track_functions > flinfo->fn_stats);

This line is a bit unclear, and I can find no equivalent usage in the
codebase. I can see the "collect stats if track_functions > this" comment
in fmgr.h, so you're following the prescribed pattern, but it looks weird
to me because we're comparing a value assigned from an enum to an unsigned
char, so there's an ick-factor here. Clearly it is working, but I wonder if
we want to explicitly type that enum in C23, or encapsulate that test in a
macro in the mean time...though all of these concerns are outside the scope
for this patch.

src/backend/executor/execExprInterp.c
----------------------------------------------------

Everything here seems ok, but I'd feel better if somebody with more
executor experience had a look.

cast.out
-----------

+SELECT CAST('2'::jsonb AS text DEFAULT 'fallback' ON CONVERSION ERROR);
+ text
+------
+ 2
+(1 row)

Was this meant to be a double-fail? I ask mostly because all CASTs up to
this point have been testing whether the datatype can fail over to default,
and this tests whether a default is ignored on a correct cast. It might
help to have a comment line explaining what part of the functionality is
being tested before each block of CASTs of that test type.

+-- source expression is a unknown const
+SELECT CAST('a' as int DEFAULT sum(1) ON CONVERSION ERROR); -- error
+ERROR: aggregate functions are not allowed in CAST DEFAULT expressions
+LINE 1: SELECT CAST('a' as int DEFAULT sum(1) ON CONVERSION ERROR);

Header comment doesn't match what is being tested.

+SELECT CAST('a' as int DEFAULT (SELECT NULL) ON CONVERSION ERROR); -- error
+ERROR: cannot use subquery in CAST DEFAULT expression
+LINE 1: SELECT CAST('a' as int DEFAULT (SELECT NULL) ON CONVERSION E...

I hadn't thought about this case until now, but it seems like a reasonable
limitation.

+SELECT CAST('a' as int DEFAULT 'b' ON CONVERSION ERROR); -- error
+ERROR: invalid input syntax for type integer: "b"
+LINE 1: SELECT CAST('a' as int DEFAULT 'b' ON CONVERSION ERROR);

This test is especially important as it demonstrates some of the fact that
the second expression is casted error-unsafe.

To that end we should have a test like this to drive home that this can be
chained if you really need such a thing

SELECT CAST ('b' AS integer
DEFAULT CAST('c' AS INTEGER
DEFAULT 0 ON CONVERSION
ERROR)
ON CONVERSION ERROR)

+-- source expression contain subquery
+SELECT CAST((SELECT b FROM generate_series(1, 1), (VALUES('H')) s(b))
+ AS int2vector[] DEFAULT '{1 3}' ON CONVERSION ERROR);

I'm curious why this doesn't work, both the initial expression and the
default expression are both a_expr. I can live with the limitation if it is
truly necessary, but it does feel mildly inconsistent.

+SET jit_above_cost TO DEFAULT;
+SELECT CAST(a AS int2 DEFAULT NULL ON CONVERSION ERROR) FROM tcast0;

What is this testing? We should add a comment to indicate its purpose.

+CREATE DOMAIN ddomain AS numeric CHECK ((1/ (VALUE - 1)) > 0);
+SELECT CAST ('1' AS ddomain DEFAULT NULL ON CONVERSION ERROR); -- error
+ERROR: division by zero
+SELECT CAST (1 AS ddomain DEFAULT NULL ON CONVERSION ERROR); -- error
+ERROR: division by zero

Good test, needs a comment to explain why this sort of error should be
raised because it's not a _conversion_ error.

+SELECT CAST (CAST('a' AS int) AS text DEFAULT NULL ON CONVERSION ERROR)
FROM tcast1; -- error
+ERROR: invalid input syntax for type integer: "a"
+LINE 1: SELECT CAST (CAST('a' AS int) AS text DEFAULT NULL ON CONVER...

Also a good test. Needs comment. We shouldn't waste any opportunity to
explain the rules of cast on error.

+SELECT CAST (CAST(a AS d_nota) AS text DEFAULT NULL ON CONVERSION ERROR)
FROM tcast1;
+ a
+---
+
+(1 row)
+
+SELECT CAST (CAST('a' AS d_nota) AS text DEFAULT NULL ON CONVERSION ERROR)
FROM tcast1;
+ text
+------
+
+(1 row)

These should be failures at the inner cast level, before the on-conversion
cast is attempted.

Overall
======

I'm concerned about the d_nota cast results as well as the no-subquery in
the default expression limitation, but everything else is looking pretty
good.

It may be worth thinking about whether the SAFE/UNSAFE function designation
has utility outside of cast-default, as that may be committable independent
of the cast changes.