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

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.

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

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

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

+   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.


Regards,
Amul