On Fri, Jul 26, 2024 at 4:53 PM Amit Langote <amitlangot...@gmail.com> wrote:
>
>
> Pushed 0003-0005 ahead of 0001-0002. Will try to push them over the
> weekend. Rebased for now.
{
...
/*
* For expression nodes that support soft errors. Should be set to NULL
* before calling ExecInitExprRec() if the caller wants errors thrown.
*/
ErrorSaveContext *escontext;
} ExprState;
i believe by default makeNode will set escontext to NULL.
So the comment should be, if you want to catch the soft errors, make
sure the escontext pointing to an allocated ErrorSaveContext.
or maybe just say, default is NULL.
Otherwise, the original comment's meaning feels like: we need to
explicitly set it to NULL
for certain operations, which I believe is false?
struct
{
Oid targettype;
int32 targettypmod;
bool omit_quotes;
bool exists_coerce;
bool exists_cast_to_int;
bool check_domain;
void *json_coercion_cache;
ErrorSaveContext *escontext;
} jsonexpr_coercion;
do we need to comment that "check_domain" is only used for JSON_EXISTS_OP?
While reviewing it, I found another minor issue.
json_behavior_type:
ERROR_P { $$ = JSON_BEHAVIOR_ERROR; }
| NULL_P { $$ = JSON_BEHAVIOR_NULL; }
| TRUE_P { $$ = JSON_BEHAVIOR_TRUE; }
| FALSE_P { $$ = JSON_BEHAVIOR_FALSE; }
| UNKNOWN { $$ = JSON_BEHAVIOR_UNKNOWN; }
| EMPTY_P ARRAY { $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
| EMPTY_P OBJECT_P { $$ = JSON_BEHAVIOR_EMPTY_OBJECT; }
/* non-standard, for Oracle compatibility only */
| EMPTY_P { $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
;
EMPTY_P behaves the same as EMPTY_P ARRAY
so for function GetJsonBehaviorConst, the following "case
JSON_BEHAVIOR_EMPTY:" is wrong?
case JSON_BEHAVIOR_NULL:
case JSON_BEHAVIOR_UNKNOWN:
case JSON_BEHAVIOR_EMPTY:
val = (Datum) 0;
isnull = true;
typid = INT4OID;
len = sizeof(int32);
isbyval = true;
break;
also src/backend/utils/adt/ruleutils.c
if (jexpr->on_error->btype != JSON_BEHAVIOR_EMPTY)
get_json_behavior(jexpr->on_error, context, "ERROR");
for json_value, json_query, i believe we can save some circles in
ExecInitJsonExpr
if you don't specify on error, on empty
can you please check the attached, based on your latest attachment.
From 618b48991d5239eb924070b0357c5208a9d1ca5c Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Fri, 26 Jul 2024 21:15:41 +0800
Subject: [PATCH v2 1/1] save some circle in ExecInitJsonExpr
if returning type is not domain and on_error->btype is JSON_BEHAVIOR_NULL
or
if returning type is not domain and on_empty->btype is JSON_BEHAVIOR_NULL
---
src/backend/executor/execExpr.c | 139 +++++++++++++++++---------------
1 file changed, 75 insertions(+), 64 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 3c4e503b..48324e98 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -4237,6 +4237,8 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state,
jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR ?
&jsestate->escontext : NULL;
+ bool returning_is_domain =
+ get_typtype(jsexpr->returning->typid) == TYPTYPE_DOMAIN;
jsestate->jsexpr = jsexpr;
/*
@@ -4388,8 +4390,6 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state,
if (jsexpr->on_error &&
jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR)
{
- ErrorSaveContext *saved_escontext;
-
jsestate->jump_error = state->steps_len;
/* JUMP to end if false, that is, skip the ON ERROR expression. */
@@ -4400,42 +4400,46 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state,
scratch->d.jump.jumpdone = -1; /* set below */
ExprEvalPushStep(state, scratch);
- /*
- * Steps to evaluate the ON ERROR expression; handle errors softly to
- * rethrow them in COERCION_FINISH step that will be added later.
- */
- saved_escontext = state->escontext;
- state->escontext = escontext;
- ExecInitExprRec((Expr *) jsexpr->on_error->expr,
- state, resv, resnull);
- state->escontext = saved_escontext;
-
- /* Step to coerce the ON ERROR expression if needed */
- if (jsexpr->on_error->coerce)
- ExecInitJsonCoercion(state, jsexpr->returning, escontext,
- jsexpr->omit_quotes, false,
- resv, resnull);
-
- /*
- * Add a COERCION_FINISH step to check for errors that may occur when
- * coercing and rethrow them.
- */
- if (jsexpr->on_error->coerce ||
- IsA(jsexpr->on_error->expr, CoerceViaIO) ||
- IsA(jsexpr->on_error->expr, CoerceToDomain))
+ if (returning_is_domain || jsexpr->on_error->btype != JSON_BEHAVIOR_NULL)
{
- scratch->opcode = EEOP_JSONEXPR_COERCION_FINISH;
- scratch->resvalue = resv;
- scratch->resnull = resnull;
- scratch->d.jsonexpr.jsestate = jsestate;
+ ErrorSaveContext *saved_escontext;
+ /*
+ * Steps to evaluate the ON ERROR expression; handle errors softly to
+ * rethrow them in COERCION_FINISH step that will be added later.
+ */
+ saved_escontext = state->escontext;
+ state->escontext = escontext;
+ ExecInitExprRec((Expr *) jsexpr->on_error->expr,
+ state, resv, resnull);
+ state->escontext = saved_escontext;
+
+ /* Step to coerce the ON ERROR expression if needed */
+ if (jsexpr->on_error->coerce)
+ ExecInitJsonCoercion(state, jsexpr->returning, escontext,
+ jsexpr->omit_quotes, false,
+ resv, resnull);
+
+ /*
+ * Add a COERCION_FINISH step to check for errors that may occur when
+ * coercing and rethrow them.
+ */
+ if (jsexpr->on_error->coerce ||
+ IsA(jsexpr->on_error->expr, CoerceViaIO) ||
+ IsA(jsexpr->on_error->expr, CoerceToDomain))
+ {
+ scratch->opcode = EEOP_JSONEXPR_COERCION_FINISH;
+ scratch->resvalue = resv;
+ scratch->resnull = resnull;
+ scratch->d.jsonexpr.jsestate = jsestate;
+ ExprEvalPushStep(state, scratch);
+ }
+
+ /* JUMP to end to skip the ON EMPTY steps added below. */
+ jumps_to_end = lappend_int(jumps_to_end, state->steps_len);
+ scratch->opcode = EEOP_JUMP;
+ scratch->d.jump.jumpdone = -1;
ExprEvalPushStep(state, scratch);
}
-
- /* JUMP to end to skip the ON EMPTY steps added below. */
- jumps_to_end = lappend_int(jumps_to_end, state->steps_len);
- scratch->opcode = EEOP_JUMP;
- scratch->d.jump.jumpdone = -1;
- ExprEvalPushStep(state, scratch);
}
/*
@@ -4445,8 +4449,6 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state,
if (jsexpr->on_empty != NULL &&
jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR)
{
- ErrorSaveContext *saved_escontext;
-
jsestate->jump_empty = state->steps_len;
/* JUMP to end if false, that is, skip the ON EMPTY expression. */
@@ -4458,35 +4460,44 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state,
ExprEvalPushStep(state, scratch);
/*
- * Steps to evaluate the ON EMPTY expression; handle errors softly to
- * rethrow them in COERCION_FINISH step that will be added later.
- */
- saved_escontext = state->escontext;
- state->escontext = escontext;
- ExecInitExprRec((Expr *) jsexpr->on_empty->expr,
- state, resv, resnull);
- state->escontext = saved_escontext;
-
- /* Step to coerce the ON EMPTY expression if needed */
- if (jsexpr->on_empty->coerce)
- ExecInitJsonCoercion(state, jsexpr->returning, escontext,
- jsexpr->omit_quotes, false,
- resv, resnull);
-
- /*
- * Add a COERCION_FINISH step to check for errors that may occur when
- * coercing and rethrow them.
- */
- if (jsexpr->on_empty->coerce ||
- IsA(jsexpr->on_empty->expr, CoerceViaIO) ||
- IsA(jsexpr->on_empty->expr, CoerceToDomain))
+ * only json_query, json_value can set on_empty if on_empty is default
+ * JSON_BEHAVIOR_NULL, the above EEOP_JUMP_IF_NOT_TRUE action is enough.
+ * but domain can have not null, so we still need coercion steps.
+ */
+ if (returning_is_domain || jsexpr->on_empty->btype != JSON_BEHAVIOR_NULL)
{
+ ErrorSaveContext *saved_escontext;
+ /*
+ * Steps to evaluate the ON EMPTY expression; handle errors softly to
+ * rethrow them in COERCION_FINISH step that will be added later.
+ */
+ saved_escontext = state->escontext;
+ state->escontext = escontext;
+ ExecInitExprRec((Expr *) jsexpr->on_empty->expr,
+ state, resv, resnull);
+ state->escontext = saved_escontext;
- scratch->opcode = EEOP_JSONEXPR_COERCION_FINISH;
- scratch->resvalue = resv;
- scratch->resnull = resnull;
- scratch->d.jsonexpr.jsestate = jsestate;
- ExprEvalPushStep(state, scratch);
+ /* Step to coerce the ON EMPTY expression if needed */
+ if (jsexpr->on_empty->coerce)
+ ExecInitJsonCoercion(state, jsexpr->returning, escontext,
+ jsexpr->omit_quotes, false,
+ resv, resnull);
+
+ /*
+ * Add a COERCION_FINISH step to check for errors that may occur when
+ * coercing and rethrow them.
+ */
+ if (jsexpr->on_empty->coerce ||
+ IsA(jsexpr->on_empty->expr, CoerceViaIO) ||
+ IsA(jsexpr->on_empty->expr, CoerceToDomain))
+ {
+
+ scratch->opcode = EEOP_JSONEXPR_COERCION_FINISH;
+ scratch->resvalue = resv;
+ scratch->resnull = resnull;
+ scratch->d.jsonexpr.jsestate = jsestate;
+ ExprEvalPushStep(state, scratch);
+ }
}
}
--
2.34.1
