Skip to content

Commit 716bd12

Browse files
amitlanamitlan
committed
SQL/JSON: Always coerce JsonExpr result at runtime
Instead of looking up casts at parse time for converting the result of JsonPath* query functions to the specified or the default RETURNING type, always perform the conversion at runtime using either the target type's input function or the function json_populate_type(). There are two motivations for this change: 1. json_populate_type() coerces to types with typmod such that any string values that exceed length limit cause an error instead of silent truncation, which is necessary to be standard-conforming. 2. It was possible to end up with a cast expression that doesn't support soft handling of errors causing bugs in the of handling ON ERROR clause. JsonExpr.coercion_expr which would store the cast expression is no longer necessary, so remove. Bump catversion because stored rules change because of the above removal. Reported-by: Alvaro Herrera <alvherre@alvh.no-ip.org> Reviewed-by: Jian He <jian.universality@gmail.com> Discussion: Discussion: https://postgr.es/m/202405271326.5a5rprki64aw%40alvherre.pgsql
1 parent c2d93c3 commit 716bd12

File tree

15 files changed

+213
-309
lines changed

15 files changed

+213
-309
lines changed

‎src/backend/executor/execExpr.c‎

Lines changed: 17 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ static void ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state,
9292
Datum *resv, bool *resnull,
9393
ExprEvalStep *scratch);
9494
static void ExecInitJsonCoercion(ExprState *state, JsonReturning *returning,
95-
ErrorSaveContext *escontext,
95+
ErrorSaveContext *escontext, bool omit_quotes,
9696
Datum *resv, bool *resnull);
9797

9898

@@ -4313,13 +4313,15 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state,
43134313
ExprEvalPushStep(state, scratch);
43144314

43154315
/*
4316-
* Jump to coerce the NULL using coercion_expr if present. Coercing NULL
4317-
* is only interesting when the RETURNING type is a domain whose
4318-
* constraints must be checked. jsexpr->coercion_expr containing a
4319-
* CoerceToDomain node must have been set in that case.
4316+
* Jump to coerce the NULL using json_populate_type() if needed. Coercing
4317+
* NULL is only interesting when the RETURNING type is a domain whose
4318+
* constraints must be checked. jsexpr->use_json_coercion must have been
4319+
* set in that case.
43204320
*/
4321-
if (jsexpr->coercion_expr)
4321+
if (get_typtype(jsexpr->returning->typid) == TYPTYPE_DOMAIN &&
4322+
DomainHasConstraints(jsexpr->returning->typid))
43224323
{
4324+
Assert(jsexpr->use_json_coercion);
43234325
scratch->opcode = EEOP_JUMP;
43244326
scratch->d.jump.jumpdone = state->steps_len + 1;
43254327
ExprEvalPushStep(state, scratch);
@@ -4337,33 +4339,12 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state,
43374339
* NULL returned on NULL input as described above.
43384340
*/
43394341
jsestate->jump_eval_coercion = -1;
4340-
if (jsexpr->coercion_expr)
4341-
{
4342-
Datum *save_innermost_caseval;
4343-
bool *save_innermost_casenull;
4344-
ErrorSaveContext *save_escontext;
4345-
4346-
jsestate->jump_eval_coercion = state->steps_len;
4347-
4348-
save_innermost_caseval = state->innermost_caseval;
4349-
save_innermost_casenull = state->innermost_casenull;
4350-
save_escontext = state->escontext;
4351-
4352-
state->innermost_caseval = resv;
4353-
state->innermost_casenull = resnull;
4354-
state->escontext = escontext;
4355-
4356-
ExecInitExprRec((Expr *) jsexpr->coercion_expr, state, resv, resnull);
4357-
4358-
state->innermost_caseval = save_innermost_caseval;
4359-
state->innermost_casenull = save_innermost_casenull;
4360-
state->escontext = save_escontext;
4361-
}
4362-
else if (jsexpr->use_json_coercion)
4342+
if (jsexpr->use_json_coercion)
43634343
{
43644344
jsestate->jump_eval_coercion = state->steps_len;
43654345

4366-
ExecInitJsonCoercion(state, jsexpr->returning, escontext, resv, resnull);
4346+
ExecInitJsonCoercion(state, jsexpr->returning, escontext,
4347+
jsexpr->omit_quotes, resv, resnull);
43674348
}
43684349
else if (jsexpr->use_io_coercion)
43694350
{
@@ -4435,8 +4416,8 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state,
44354416

44364417
/* Step to coerce the ON ERROR expression if needed */
44374418
if (jsexpr->on_error->coerce)
4438-
ExecInitJsonCoercion(state, jsexpr->returning, escontext, resv,
4439-
resnull);
4419+
ExecInitJsonCoercion(state, jsexpr->returning, escontext,
4420+
jsexpr->omit_quotes, resv, resnull);
44404421

44414422
/* JUMP to end to skip the ON EMPTY steps added below. */
44424423
jumps_to_end = lappend_int(jumps_to_end, state->steps_len);
@@ -4468,8 +4449,8 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state,
44684449

44694450
/* Step to coerce the ON EMPTY expression if needed */
44704451
if (jsexpr->on_empty->coerce)
4471-
ExecInitJsonCoercion(state, jsexpr->returning, escontext, resv,
4472-
resnull);
4452+
ExecInitJsonCoercion(state, jsexpr->returning, escontext,
4453+
jsexpr->omit_quotes, resv, resnull);
44734454
}
44744455

44754456
foreach(lc, jumps_to_end)
@@ -4488,7 +4469,7 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state,
44884469
*/
44894470
static void
44904471
ExecInitJsonCoercion(ExprState *state, JsonReturning *returning,
4491-
ErrorSaveContext *escontext,
4472+
ErrorSaveContext *escontext, bool omit_quotes,
44924473
Datum *resv, bool *resnull)
44934474
{
44944475
ExprEvalStep scratch = {0};
@@ -4501,5 +4482,6 @@ ExecInitJsonCoercion(ExprState *state, JsonReturning *returning,
45014482
scratch.d.jsonexpr_coercion.targettypmod = returning->typmod;
45024483
scratch.d.jsonexpr_coercion.json_populate_type_cache = NULL;
45034484
scratch.d.jsonexpr_coercion.escontext = escontext;
4485+
scratch.d.jsonexpr_coercion.omit_quotes = omit_quotes;
45044486
ExprEvalPushStep(state, &scratch);
45054487
}

‎src/backend/executor/execExprInterp.c‎

Lines changed: 23 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -4303,8 +4303,14 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op,
43034303

43044304
if (!error)
43054305
{
4306-
*op->resvalue = BoolGetDatum(exists);
43074306
*op->resnull = false;
4307+
if (jsexpr->use_json_coercion)
4308+
*op->resvalue = DirectFunctionCall1(jsonb_in,
4309+
BoolGetDatum(exists) ?
4310+
CStringGetDatum("true") :
4311+
CStringGetDatum("false"));
4312+
else
4313+
*op->resvalue = BoolGetDatum(exists);
43084314
}
43094315
}
43104316
break;
@@ -4316,22 +4322,6 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op,
43164322
jsexpr->column_name);
43174323

43184324
*op->resnull = (DatumGetPointer(*op->resvalue) == NULL);
4319-
4320-
/* Handle OMIT QUOTES. */
4321-
if (!*op->resnull && jsexpr->omit_quotes)
4322-
{
4323-
val_string = JsonbUnquote(DatumGetJsonbP(*op->resvalue));
4324-
4325-
/*
4326-
* Pass the string as a text value to the cast expression if
4327-
* one present. If not, use the input function call below to
4328-
* do the coercion.
4329-
*/
4330-
if (jump_eval_coercion >= 0)
4331-
*op->resvalue =
4332-
DirectFunctionCall1(textin,
4333-
PointerGetDatum(val_string));
4334-
}
43354325
break;
43364326

43374327
case JSON_VALUE_OP:
@@ -4343,7 +4333,7 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op,
43434333

43444334
if (jbv == NULL)
43454335
{
4346-
/* Will be coerced with coercion_expr, if any. */
4336+
/* Will be coerced with json_populate_type(), if needed. */
43474337
*op->resvalue = (Datum) 0;
43484338
*op->resnull = true;
43494339
}
@@ -4355,18 +4345,22 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op,
43554345
val_string = DatumGetCString(DirectFunctionCall1(jsonb_out,
43564346
JsonbPGetDatum(JsonbValueToJsonb(jbv))));
43574347
}
4348+
else if (jsexpr->use_json_coercion)
4349+
{
4350+
*op->resvalue = JsonbPGetDatum(JsonbValueToJsonb(jbv));
4351+
*op->resnull = false;
4352+
}
43584353
else
43594354
{
43604355
val_string = ExecGetJsonValueItemString(jbv, op->resnull);
43614356

43624357
/*
4363-
* Pass the string as a text value to the cast
4364-
* expression if one present. If not, use the input
4365-
* function call below to do the coercion.
4358+
* Simply convert to the default RETURNING type (text)
4359+
* if no coercion needed.
43664360
*/
4367-
*op->resvalue = PointerGetDatum(val_string);
4368-
if (jump_eval_coercion >= 0)
4369-
*op->resvalue = DirectFunctionCall1(textin, *op->resvalue);
4361+
if (!jsexpr->use_io_coercion)
4362+
*op->resvalue = DirectFunctionCall1(textin,
4363+
CStringGetDatum(val_string));
43704364
}
43714365
}
43724366
break;
@@ -4545,13 +4539,14 @@ ExecEvalJsonCoercion(ExprState *state, ExprEvalStep *op,
45454539
op->d.jsonexpr_coercion.targettypmod,
45464540
&op->d.jsonexpr_coercion.json_populate_type_cache,
45474541
econtext->ecxt_per_query_memory,
4548-
op->resnull, (Node *) escontext);
4542+
op->resnull,
4543+
op->d.jsonexpr_coercion.omit_quotes,
4544+
(Node *) escontext);
45494545
}
45504546

45514547
/*
4552-
* Checks if an error occurred either when evaluating JsonExpr.coercion_expr or
4553-
* in ExecEvalJsonCoercion(). If so, this sets JsonExprState.error to trigger
4554-
* the ON ERROR handling steps.
4548+
* Checks if an error occurred in ExecEvalJsonCoercion(). If so, this sets
4549+
* JsonExprState.error to trigger the ON ERROR handling steps.
45554550
*/
45564551
void
45574552
ExecEvalJsonCoercionFinish(ExprState *state, ExprEvalStep *op)

‎src/backend/jit/llvm/llvmjit_expr.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2010,7 +2010,7 @@ llvm_compile_expr(ExprState *state)
20102010
v_jump_coercion = l_int32_const(lc, jsestate->jump_eval_coercion);
20112011
LLVMAddCase(v_switch, v_jump_coercion, b_coercion);
20122012
}
2013-
/* coercion_expr code */
2013+
/* jump_eval_coercion code */
20142014
LLVMPositionBuilderAtEnd(b, b_coercion);
20152015
if (jsestate->jump_eval_coercion >= 0)
20162016
LLVMBuildBr(b, opblocks[jsestate->jump_eval_coercion]);

‎src/backend/nodes/nodeFuncs.c‎

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1006,10 +1006,7 @@ exprCollation(const Node *expr)
10061006
{
10071007
const JsonExpr *jsexpr = (JsonExpr *) expr;
10081008

1009-
if (jsexpr->coercion_expr)
1010-
coll = exprCollation(jsexpr->coercion_expr);
1011-
else
1012-
coll = jsexpr->collation;
1009+
coll = jsexpr->collation;
10131010
}
10141011
break;
10151012
case T_JsonBehavior:
@@ -1265,10 +1262,7 @@ exprSetCollation(Node *expr, Oid collation)
12651262
{
12661263
JsonExpr *jexpr = (JsonExpr *) expr;
12671264

1268-
if (jexpr->coercion_expr)
1269-
exprSetCollation((Node *) jexpr->coercion_expr, collation);
1270-
else
1271-
jexpr->collation = collation;
1265+
jexpr->collation = collation;
12721266
}
12731267
break;
12741268
case T_JsonBehavior:
@@ -2368,8 +2362,6 @@ expression_tree_walker_impl(Node *node,
23682362
return true;
23692363
if (WALK(jexpr->path_spec))
23702364
return true;
2371-
if (WALK(jexpr->coercion_expr))
2372-
return true;
23732365
if (WALK(jexpr->passing_values))
23742366
return true;
23752367
/* we assume walker doesn't care about passing_names */
@@ -3411,7 +3403,6 @@ expression_tree_mutator_impl(Node *node,
34113403
FLATCOPY(newnode, jexpr, JsonExpr);
34123404
MUTATE(newnode->formatted_expr, jexpr->formatted_expr, Node *);
34133405
MUTATE(newnode->path_spec, jexpr->path_spec, Node *);
3414-
MUTATE(newnode->coercion_expr, jexpr->coercion_expr, Node *);
34153406
MUTATE(newnode->passing_values, jexpr->passing_values, List *);
34163407
/* assume mutator does not care about passing_names */
34173408
MUTATE(newnode->on_empty, jexpr->on_empty, JsonBehavior *);

0 commit comments

Comments
 (0)