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

From jian he
Subject Re: CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions
Date
Msg-id CACJufxF86NMYChEXLDWBhDKRrD8qM1Vs-uoqrH-AjLGScrJVqA@mail.gmail.com
Whole thread
In response to CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions  (Corey Huinker <corey.huinker@gmail.com>)
List pgsql-hackers
The attached v33 has nontrivial changes compared with v32.

previously v32 I did:
1. add coerce_to_target_type_extended _extended function seems not
future-proof enough.
--- a/src/include/parser/parse_coerce.h
+++ b/src/include/parser/parse_coerce.h
@@ -43,15 +43,29 @@ extern Node *coerce_to_target_type(ParseState *pstate,
    CoercionContext ccontext,
    CoercionForm cformat,
    int location);
+extern Node *coerce_to_target_type_extended(ParseState *pstate,
+ Node *expr,
+ Oid exprtype,
+ Oid targettype,
+ int32 targettypmod,
+ CoercionContext ccontext,
+ CoercionForm cformat,
+ int location,
+ Node *escontext);

To support CAST('error' AS integer DEFAULT 1 ON CONVERSION ERROR), the
conversion of an unknown literal to a Const, done inside coerce_to_target_type,
must be able to fail softly.  Passing an ErrorSaveContext pointer down through
those functions is ugly and not future-proof, so adding a p_escontext field to
struct ParseState. This also lets other parse-analysis code do error-safe
conversions the same way, if they want.

2.
+/*
+ * SafeTypeCastExpr -
+ * Transformed representation of
+ * CAST(expr AS typename DEFAULT expr ON CONVERSION ERROR)
+ */
+typedef struct SafeTypeCastExpr
+{
+ Expr xpr;
+
+ /*
+ * The transformed source expression.
+ *
+ * Cases like ``CAST(1 AS date DEFAULT NULL ON CONVERSION ERROR)`` where
+ * the castexpr evaluates to NULL, we need this field to reconstruct the
+ * original query.
+ */
+ Expr   *source;
+
+ /*
+ * transformed cast expression, NULL means cannot coerce to target type.
+ *
+ * For query jumbling, ignore this node because castexpr expression may
+ * contain the "source" expression. Including it would cause the same
+ * expression to be jumbled twice.
+ */
+ Expr   *castexpr pg_node_attr(query_jumble_ignore);

"castexpr" could already contain the "source" expression, so every
tree walker would see the same subtree twice.
That has unintended ramification, for example a
SubLink or an Aggref in the argument would be processed twice, and avoiding
that requires special care at each such place, which is fragile.
So instead "castexpr" is built over a CaseTestExpr placeholder, and
the argument is stored once.

3.
For FuncExpr, we should do something equivalent to ExecEvalCoerceViaIOSafe.

As I mentioned before, our error safe type cast evaluation mainly involves
processing/evaluating FuncExpr and CoerceViaIO nodes.  For CoerceViaIO,
ExecEvalCoerceViaIOSafe already sets resvalue and resnull on a soft error.
Function calls (FuncExpr) need the same treatment.  A function that reports an
error softly returns a dummy datum with isnull *still* false! and the next step
would consume it.  After a soft error the step must instead produce resvalue = 0
*and* resnull = true.  That has to happen at the call site, not in the consumer.

    CAST('\x01'::bytea::uuid AS text DEFAULT 'x1' ON CONVERSION ERROR)

Here, function bytea_uuid() reports the bad length with ereturn(), which records
the error and returns Datum 0 without setting fcinfo->isnull.  The next step
would then call uuid_out() on a null pointer.  By the errsave convention the
return value is garbage and the caller must check the context, so the
function-call step does that and returns NULL instead.

So four ExprEvalOp opcodes are added:
EEOP_FUNCEXPR_SAFE,
EEOP_FUNCEXPR_STRICT_SAFE,
EEOP_FUNCEXPR_SAFE_FUSAGE,
EEOP_FUNCEXPR_STRICT_SAFE_FUSAGE

4. extensive changes in clauses.c also required.

Rationale:
    CAST(('abc'::text)::int + abs(NULL) AS int DEFAULT 3 ON CONVERSION ERROR)

To make the above query does not fail, which is what we want (at least, I think
so), we need to make evaluate_expr handle soft errors.
This means evaluate_expr needs an ErrorSaveContext node. As a result, we’ll need
to refactor every occurrence of ece_evaluate_expr.



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

Attachment

pgsql-hackers by date:

Previous
From: Ewan Young
Date:
Subject: Re: ERROR: no relation entry for relid 3
Next
From: Peter Smith
Date:
Subject: Re: Distinguish publication exclusions in object addresses