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

On Fri, Jan 2, 2026 at 2:08 PM Amul Sul <sulamul@gmail.com> wrote:
>
> Hi,
>
> I am still thinking through a design that avoids having two different
> code paths for type casting. Can't we avoid adding a new SafeTypeCast
> structure by simply adding a raw_default variable (name could be
> simply default) to the existing TypeCast structure? If we do that, we
> would need to update transformTypeCast() and other places (like
> ExecInterpExpr()) to handle the raw_default. This approach would allow
> us to avoid the extra code required for a new node structure (e.g.,
> T_SafeTypeCastExpr) and a separate EEOP_SAFETYPE_CAST step.
>

Hi.

transformTypeCast transforms a TypeCast node and may produce one of the
following nodes: FuncExpr, CollateExpr, CoerceToDomain, ArrayCoerceExpr, or
CoerceViaIO.

To avoid EEOP_SAFETY_CAST, the returned node would need an
additional field to store the transformed DEFAULT expression.
This implies adding such a field to the aforementioned node types; otherwise,
the information about the transformed default expression would be lost.

However, adding an extra field to nodes such as FuncExpr seems not doable.
It is not generally applicable to FuncExpr, but rather only relevant to a
specific usage scenario. In addition, it may introduce unnecessary overhead.

T_SafeTypeCastExpr is still needed for holding the transformed cast expression
and default expression, I think.

However, we can add a field to node TypeCast for the raw default expression.
transformTypeSafeCast seems not needed, so I consolidated
the parsing analysis into transformTypeCast.

> Here are few other comments:
>
> vv16-0019:
>
> +float8_div_safe(const float8 val1, const float8 val2, struct Node *escontext)
>
> Patches show an inconsistent use of Node* and struct Note * for the
> escontext argument. I suggest standardising on Note * to maintain
> consistency throughout the code.
> --
>

This inconsistency already exists in the codebase,
we already have many files using "struct Node *escontext".
I guess the reason is to avoid "#include "nodes/nodes.h"
in src/include/utils/float.h

> v16-0020:
>
> @@ -839,7 +839,7 @@ box_distance(PG_FUNCTION_ARGS)
>     box_cn(&a, box1);
>     box_cn(&b, box2);
>
> -   PG_RETURN_FLOAT8(point_dt(&a, &b));
> +   PG_RETURN_FLOAT8(point_dt(&a, &b, NULL));
>
> I think user-callable functions that accept PG_FUNCTION_ARGS;
> should directly pass fcinfo->context instead of NULL.
> --
>

This seems overall good for me. since next time, if we want
other functions to be error safe, we don't need to do this again.


> v16-0022:
>
> +Sometimes a type cast may fail; to avoid such fail case, an
> <literal>ON ERROR</literal> clause can be
> ..
> +    default <replaceable>expression</replaceable> in <literal>ON
> ERROR</literal> clause.
>
> Shouldn't it be ON CONVERSION ERROR instead of ON ERROR ?
> --
>

ON CONVERSION ERROR is ok for me.


> +   state->escontext = makeNode(ErrorSaveContext);
> +   state->escontext->type = T_ErrorSaveContext;
> +   state->escontext->error_occurred = false;
> +   state->escontext->details_wanted = false;
>
> No need to assign values to the rest of the escontext members;
> makeNode(ErrorSaveContext) is sufficient. I also think
> ExecInitExprSafe() should receive escontext from the caller. Instead
> of passing an error_safe boolean to evaluate_expr, you can pass the
> escontext itself; this can then be passed down to ExecInitExprSafe,
> helping capture soft error information at a much higher level.
>
> In that way, you can simply call ExecInitExprSafe() from
> ExecInitExpr() and pass NULL for the escontext. This reduces code
> duplication, since most of the code is similar except for the
> aforementioned initialization lines.
>

now i changed it to:

ExprState *
ExecInitExpr(Expr *node, PlanState *parent)
{
    return ExecInitExprExtended(node, NULL, parent);
}

ExprState *
ExecInitExprExtended(Expr *node, Node *escontext, PlanState *parent)


--
jian
https://www.enterprisedb.com/