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

On Mon, Jan 5, 2026 at 11:31 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> On Fri, Jan 2, 2026 at 2:08 PM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> >
> 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.
>

Oh okay.

> [...]
> 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.
>

Yeah, but I am a bit unsure about the naming (i.e., raw_default) and
the comment stating this is an "untransformed DEFAULT expression".
What is the status of the arg member of the TypeCast structure -- is
it transformed or untransformed? If that is also untransformed, we can
simply drop "untransformed" word from the comment and rename
raw_default to something more descriptive, such as defaultExpr.
Currently, raw_default doesn't clearly indicate that it represents an
expression, IMO.

Also, a few comments still refer to old transformTypeSafeCast(), which
needs to be corrected.

> > 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)
>

Yes, that looks like what I was thinking, but with the Node *escontext
at the last position.

I am not fully confident that the v17-0022 patch is complete. I
believe there is a good chance to improve the patch in terms of
coding, formatting, function and variable naming, as well as some
refactoring and optimization that I still need to think on. Here are a
few comments:

+ /*
+ * The collation of DEFAULT expression must match the collation of the
+ * target type.
+ */
+ defColl = exprCollation(defexpr);
+
+ if (OidIsValid(targetTypecoll) && OidIsValid(defColl) &&
+ targetTypecoll != defColl)
+ ereport(ERROR,
+ errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("the collation of CAST DEFAULT expression
conflicts with target type collation"),

What if one collation is an invalid OID and the other is valid?
Shouldn't we throw an error in that case?
--

+ SafeTypeCastState *stcstate;
+
+ stcstate = palloc0(sizeof(SafeTypeCastState));
+ stcstate->stcexpr = stcexpr;
+ stcstate->escontext.type = T_ErrorSaveContext;
+ state->escontext = &stcstate->escontext;

Can't we reuse the state->escontext pointer in the SafeTypeCastState
instead of allocating a separate ErrorSaveContext?
--

+ ErrorSaveContext *saved_escontext;
+
+ saved_escontext = state->escontext;
+
+ state->escontext = NULL;
+
+ ExecInitExprRec((Expr *) stcstate->stcexpr->default_expr,
+ state, resv, resnull);
+
+ state->escontext = saved_escontext;

Here should be some comment explanation as to why you are setting
state->escontext = NULL, and why that is needed for the default
expression initialization but not for the case_expr evaluation.
--

+
+ fmgr_info(functionId, &flinfo);
+
+ return InputFunctionCallSafe(&flinfo, str, typioparam, typmod,
+ escontext, result);

Remove the empty line between these two for consistency with other
function's formatting.
--

+ ? errhint("Safe type cast for user-defined types are
not yet supported")
+ : errhint("Explicit cast is defined but definition is
not error safe"),

Hints should end with a period.
--

+ /*
+ * Here, we cannot deparsing cast_expr directly, since
+ * transformTypeSafeCast may have folded it into a simple
+ * constant or NULL. Instead, we use source_expr and
+ * default_expr to reconstruct the CAST DEFAULT clause.
+ */

I am wondering how other places handle expression deparsing; do they
hold the original expression along with the transformed expression?
Also, why are we only folding constants from the source_expr but not
from the default expression?

Regards,
Amul