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

>
>
>
> SELECT CAST(ARRAY[a, '7'] AS int[] DEFAULT '{1}' ON CONVERSION ERROR)
> FROM (VALUES('a'), ('1')) t(a); -- QueryA
> SELECT CAST(ARRAY[a::int] AS int[] DEFAULT NULL ON CONVERSION ERROR)
> FROM (VALUES('a'), ('1')) t(a); -- QueryB
>
> We want the QueryA to fail and QueryB not to. It is not trivial to do that.
>

I believe the exact opposite should be true, and your later comments imply
that you agree, so this might just be a typo. In case it isn't, I believe
ARRAY[a,7] will correctly evaluate to ARRAY['a','7'] on the first row and
ARRAY['1','7'] on the second row. the first row will have a conversion
error to the result set will be:

{1}
{1,7}

The second one should fail outright when 'a'::int is executed, which means
the array expression never completed, so the cast was never attempted.

>
> transformArrayExpr has to transform each element of the ARRAY
> constructor (ARRAY[expr]) the
> same way as a non-array argument: the element expression itself is
> evaluated
> normally, and only its conversion to the target element type is
> error-safe. The
> result is an ArrayExpr whose element conversions report into the cast's
> ErrorSaveContext, so that ArrayExpr must be evaluated under that context.
> Otherwise the conversions cannot fail softly.
>
> However, always evaluating SafeTypeCastExpr->arg under the cast's
> ErrorSaveContext would make the source expression error-safe as well, so
> that
>

I agree, what I don't understand is how the expression ('abc'::text)::int
knows it's part of a larger CAST ON DEFAULT expression.

>
> CAST(('abc'::text)::int AS int DEFAULT 3 ON CONVERSION ERROR)
> would return 3 instead of failing.
>

I agree that we want CAST(('abc'::text)::int AS int DEFAULT 3 ON CONVERSION
ERROR) to fail outright.

> For SafeTypeCastExpr->arg, the escontext needs to be passed down to
> arg only when arg is an ARRAY constructor (ARRAY[expr]), not for any
> other expression type.
> Hence add a boolean field arg_errorsafe to SafeTypeCastExpr to record this.
>
> I've attached inconsistency.sql; let me know which SQL query output is
> unexpected.
>

Some comments

-- 6. An element whose conversion is known to fail before run time makes the
-- result the DEFAULT, whatever its position, and the argument is not
-- evaluated. An untyped literal like 'x' fails at parse time; a typed
-- constant like 'x'::text, or a parameter in a custom plan, fails when the
-- planner folds its conversion.

Unfortunately, I don't think we can elide these cant-ever-succeed casts
into the default expression precisely because they might themselves contain
expressions that blow up the query, but that feeling is coming from an
assumption that we evaluate nodes for elimination bottom-up, and if the SQL
standard says we should do it top-down, then my expectations would change.

SELECT CAST(1 / z AS point DEFAULT NULL ON CONVERSION ERROR) FROM tinc; --
ok

Oddly enough, I think this should become a N-by-1 result set of NULL
values, unless a z value causes a div-by-zero or similar exception. Again,
if the standards people feel that top-down elimination is how its defined,
then I'd feel differently.

SELECT CAST(CAST('{a}'::text[] AS date[]) AS int[] DEFAULT NULL ON
CONVERSION ERROR); -- ok

Similar thing, it's an error if we evaluate bottom-up, and ok if it's
top-down.

> > 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.
> I do not fully understand this part.
> Can you expand on this?
>

It may be that we could expand our PLs in a way that allows them to fetch
these error contexts and handle them instead of having to create an
expensive exception block.

Patch applies, but I get this

[441/1149] Compiling C object
src/backend/postgres_lib.a.p/parser_parse_collate.c.o
../src/backend/parser/parse_collate.c:461:8: warning: variable 'collation'
is used uninitialized whenever switch case is taken
[-Wsometimes-uninitialized]
461 | case T_SafeCastElement:
| ^~~~~~~~~~~~~~~~~
../src/backend/parser/parse_collate.c:792:24: note: uninitialized use
occurs here
792 | merge_collation_state(collation,
| ^~~~~~~~~
../src/backend/parser/parse_collate.c:445:8: warning: variable 'collation'
is used uninitialized whenever switch case is taken
[-Wsometimes-uninitialized]
445 | case T_SafeTypeCastExpr:
| ^~~~~~~~~~~~~~~~~~
../src/backend/parser/parse_collate.c:792:24: note: uninitialized use
occurs here
792 | merge_collation_state(collation,
| ^~~~~~~~~
../src/backend/parser/parse_collate.c:258:17: note: initialize the variable
'collation' to silence this warning
258 | Oid collation;
| ^
| = 0
../src/backend/parser/parse_collate.c:461:8: warning: variable 'strength'
is used uninitialized whenever switch case is taken
[-Wsometimes-uninitialized]
461 | case T_SafeCastElement:
| ^~~~~~~~~~~~~~~~~
../src/backend/parser/parse_collate.c:793:9: note: uninitialized use occurs
here
793 | strength,
| ^~~~~~~~
../src/backend/parser/parse_collate.c:445:8: warning: variable 'strength'
is used uninitialized whenever switch case is taken
[-Wsometimes-uninitialized]
445 | case T_SafeTypeCastExpr:
| ^~~~~~~~~~~~~~~~~~
../src/backend/parser/parse_collate.c:793:9: note: uninitialized use occurs
here
793 | strength,
| ^~~~~~~~
../src/backend/parser/parse_collate.c:259:2: note: variable 'strength' is
declared here
259 | CollateStrength strength;
| ^
../src/backend/parser/parse_collate.c:461:8: warning: variable 'location'
is used uninitialized whenever switch case is taken
[-Wsometimes-uninitialized]
461 | case T_SafeCastElement:
| ^~~~~~~~~~~~~~~~~
../src/backend/parser/parse_collate.c:794:9: note: uninitialized use occurs
here
794 | location,
| ^~~~~~~~
../src/backend/parser/parse_collate.c:445:8: warning: variable 'location'
is used uninitialized whenever switch case is taken
[-Wsometimes-uninitialized]
445 | case T_SafeTypeCastExpr:
| ^~~~~~~~~~~~~~~~~~
../src/backend/parser/parse_collate.c:794:9: note: uninitialized use occurs
here
794 | location,
| ^~~~~~~~
../src/backend/parser/parse_collate.c:260:16: note: initialize the variable
'location' to silence this warning
260 | int location;
| ^
| = 0

This new node type seems to cross-cut the potential solutions, but I
suspect that the solution is to do what T_Var/T_Const/etc do in the same
situation.

Moving onto the code

0001 - no changes from what I can detect.
0002 - no changes from what I can detect.

0003

I think there might be a bug here:

if (stcexpr->castexpr == NULL)
{
state->escontext = saved_escontext;
ExecInitExprRec(stcexpr->defexpr, state, resv, resnull);
return;
}

In this case, the comment says we want to execute the expression so it can
raise errors, but we caught those in an errcontext and we never look at the
errcontext to see if it found anything.

I'm going to stop there for now.