public inbox for [email protected]
help / color / mirror / Atom feedFrom: Amul Sul <[email protected]>
To: jian he <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: Corey Huinker <[email protected]>
Cc: Vik Fearing <[email protected]>
Cc: Isaac Morland <[email protected]>
Cc: [email protected]
Subject: Re: CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions
Date: Tue, 13 Jan 2026 11:19:36 +0530
Message-ID: <CAAJ_b97+iUAQRoOpogZHRHdr+YQug0TaEeAp9GnT5Bnp8-6oMg@mail.gmail.com> (raw)
In-Reply-To: <CACJufxHw9Y3fvh+rZj4ukLo=v54Dpafzk7Xvee_wi9zFZ6pOfg@mail.gmail.com>
References: <CADkLM=fv1JfY4Ufa-jcwwNbjQixNViskQ8jZu3Tz_p656i_4hQ@mail.gmail.com>
<CAMsGm5dpfm2PHL8XZvC-JSd+UPkgx3rpReUA=G=4+rUCH+Ntcw@mail.gmail.com>
<CADkLM=eD_S8mGhPfu5+hXXvXgR0-cxGpGd9dgPzD+nCuO7HFaQ@mail.gmail.com>
<CACJufxHCMzrHOW=wRe8L30rMhB3sjwAv1LE928Fa7sxMu1Tx-g@mail.gmail.com>
<[email protected]>
<CACJufxGRAnwJzu7nMq4ZP=yqa1Sz=qR+mR1TmY0aCDjJoJRRtg@mail.gmail.com>
<[email protected]>
<CACJufxFy+DFpJ2e-czyCTAgSJXNFaQGWFKA4mjbW-LAMGc1YBA@mail.gmail.com>
<CADkLM=f1Jv81=s5Ckazx3zZq=M5KoBJMJkOZux_-L+gezODCEQ@mail.gmail.com>
<CACJufxGw_OY7K3rfG4kDb902O2guhT-wgTjTJQ=pWeVWRTHpHQ@mail.gmail.com>
<CADkLM=cFSg3+6Sk00dLAF7Q7jnrKBk6+N5gRxT5BCxRvaGtR-g@mail.gmail.com>
<CACJufxE_aO5FtBGwhDym-Fwe7k8oJY7a8jcYDx77=t3maPvG0g@mail.gmail.com>
<CADkLM=chahh6ddZFjLL6AUdqzL_Px0raTu-5Jzn2WN8yELtmJw@mail.gmail.com>
<CACJufxE053=bO3pDUpGba6Yz3VGpU_XCbg4HO6Rew5EJ7k7VnQ@mail.gmail.com>
<CACJufxF--5d=fmoRBHfqJE9Vy38dCURNKYOKKpujRCnoTEQ7nQ@mail.gmail.com>
<CACJufxHpMJn22Nu_wmG6eV_S8SAM6KM+GhMO7GuzVb=d9q5C4A@mail.gmail.com>
<CACJufxHM2e3DQmbRdDZvWyG3ZCLyOg6XFifvOz_TGy1tGw7NHw@mail.gmail.com>
<CADkLM=daTLuRcwzc6Egtwvh4XYgtABWuMBVnEznd-dXqmXfzUw@mail.gmail.com>
<CACJufxEcrrcaeFW+zYsjgb6r+ijzwszyxeHk3wxGY+3idiA2ZA@mail.gmail.com>
<CADkLM=ehavqENDBCcYQufPFKboV90+o_uFdhcrh=Ymq_TNqo=A@mail.gmail.com>
<CADkLM=ecTybe9Z9TSRD-NKZ=-V4DuGVRtXZGO6+F7=m3Gg9GGQ@mail.gmail.com>
<CACJufxH5OSeY0-qirksn8S2FUycxON-O=iwc0-Nne1MTAguGhQ@mail.gmail.com>
<CADkLM=eFasBpS1cqf67TpKGbKoUSy00FuT05Yz4RpXQBpqktuw@mail.gmail.com>
<CACJufxHrE0s7G0xg1frWo2+tFLTLaikKCObixH-4p9zMYKtHFw@mail.gmail.com>
<CACJufxFEzD3mqc+MDpgzvdt+4Azbn2pF6TWW=dSCqSK7OHoL6A@mail.gmail.com>
<CALdSSPjd2fJHw8TrugumZSQwJ8VmSVn55OfQ+Wuogaq0ss=HGQ@mail.gmail.com>
<CACJufxFzqAshLFw-xTqpz3Mu=6nMLnPiD8bBhbqX6KcFPVjEHw@mail.gmail.com>
<CAAJ_b95nmvAYhxt2NwRAdGtvR-6STbiaFguMSLh3PmjUVE7rdg@mail.gmail.com>
<CACJufxHw9Y3fvh+rZj4ukLo=v54Dpafzk7Xvee_wi9zFZ6pOfg@mail.gmail.com>
On Mon, Jan 5, 2026 at 11:31 AM jian he <[email protected]> wrote:
>
> On Fri, Jan 2, 2026 at 2:08 PM Amul Sul <[email protected]> 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
view thread (69+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions
In-Reply-To: <CAAJ_b97+iUAQRoOpogZHRHdr+YQug0TaEeAp9GnT5Bnp8-6oMg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox