Subject: Re: remaining sql/json patches

From: Alvaro Herrera <[email-masked]>
Date: Thu, 7 Dec 2023 11:51:53 +0100

On 2023-Dec-07, Erik Rijkers wrote:

> Hm, this set doesn't apply for me. 0003 gives error, see below (sorrty for
> my interspersed bash echoing - seemed best to leave it in.
> (I'm using patch; should be all right, no? Am I doing it wrong?)

There's definitely something wrong with the patch file; that binary file
should not be there.  OTOH clearly if we ever start including binary
files in our tree, `patch` is no longer going to cut it.  Maybe we won't
ever do that, though.

There's also a complaint about whitespace.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"El destino baraja y nosotros jugamos" (A. Schopenhauer)

---

From: Amit Langote <[email-masked]>
Date: Thu, 7 Dec 2023 21:07:59 +0900

> There's also a complaint about whitespace.

Looks like I messed something up when using git (rebase -i). :-(

Apply-able patches attached, including fixes based on jian he's comments.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

---

From: Andres Freund <[email-masked]>
Date: Fri, 8 Dec 2023 09:30:14 -0800

Hi,

On 2023-12-07 21:07:59 +0900, Amit Langote wrote:
> --- a/src/include/executor/execExpr.h
> +++ b/src/include/executor/execExpr.h
> @@ -16,6 +16,7 @@
>  
>  #include "executor/nodeAgg.h"
>  #include "nodes/execnodes.h"
> +#include "nodes/miscnodes.h"
>  
>  /* forward references to avoid circularity */
>  struct ExprEvalStep;
> @@ -168,6 +169,7 @@ typedef enum ExprEvalOp
>  
>  	/* evaluate assorted special-purpose expression types */
>  	EEOP_IOCOERCE,
> +	EEOP_IOCOERCE_SAFE,
>  	EEOP_DISTINCT,
>  	EEOP_NOT_DISTINCT,
>  	EEOP_NULLIF,
> @@ -547,6 +549,7 @@ typedef struct ExprEvalStep
>  			bool	   *checknull;
>  			/* OID of domain type */
>  			Oid			resulttype;
> +			ErrorSaveContext *escontext;
>  		}			domaincheck;
>  
>  		/* for EEOP_CONVERT_ROWTYPE */
> @@ -776,6 +779,7 @@ extern void ExecEvalParamExec(ExprState *state, ExprEvalStep *op,
>  							  ExprContext *econtext);
>  extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep *op,
>  								ExprContext *econtext);
> +extern void ExecEvalCoerceViaIOSafe(ExprState *state, ExprEvalStep *op);
>  extern void ExecEvalSQLValueFunction(ExprState *state, ExprEvalStep *op);
>  extern void ExecEvalCurrentOfExpr(ExprState *state, ExprEvalStep *op);
>  extern void ExecEvalNextValueExpr(ExprState *state, ExprEvalStep *op);
> diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
> index 5d7f17dee0..6a7118d300 100644
> --- a/src/include/nodes/execnodes.h
> +++ b/src/include/nodes/execnodes.h
> @@ -34,6 +34,7 @@
>  #include "fmgr.h"
>  #include "lib/ilist.h"
>  #include "lib/pairingheap.h"
> +#include "nodes/miscnodes.h"
>  #include "nodes/params.h"
>  #include "nodes/plannodes.h"
>  #include "nodes/tidbitmap.h"
> @@ -129,6 +130,12 @@ typedef struct ExprState
>  
>  	Datum	   *innermost_domainval;
>  	bool	   *innermost_domainnull;
> +
> +	/*
> +	 * For expression nodes that support soft errors.  Should be set to NULL
> +	 * before calling ExecInitExprRec() if the caller wants errors thrown.
> +	 */
> +	ErrorSaveContext *escontext;
>  } ExprState;

Why do we need this both in ExprState *and* in ExprEvalStep?

> From 38b53297b2d435d5cebf78c1f81e4748fed6c8b6 Mon Sep 17 00:00:00 2001
> From: Amit Langote <[email-masked]>
> Date: Wed, 22 Nov 2023 13:18:49 +0900
> Subject: [PATCH v30 2/5] Add soft error handling to populate_record_field()
> 
> An uncoming patch would like the ability to call it from the
> executor for some SQL/JSON expression nodes and ask to suppress any
> errors that may occur.
> 
> This commit does two things mainly:
> 
> * It modifies the various interfaces internal to jsonfuncs.c to pass
>   the ErrorSaveContext around.
> 
> * Make necessary modifications to handle the cases where the
>   processing is aborted partway through various functions that take
>   an ErrorSaveContext when a soft error occurs.
> 
> Note that the above changes are only intended to suppress errors in
> the functions in jsonfuncs.c, but not those in any external functions
> that the functions in jsonfuncs.c in turn call, such as those from
> arrayfuncs.c.  It is assumed that the various populate_* functions
> validate the data before passing those to external functions.
> 
> Discussion: https://postgr.es/m/CA+HiwqE4XTdfb1nW=[email-masked]

The code here is getting substantially more verbose / less readable.  I wonder
if there's something more general that could be improved to make this less
painful?

I'd not at all be surprised if this caused a measurable slowdown.

> ---
>  src/backend/utils/adt/jsonfuncs.c | 310 +++++++++++++++++++++++-------
>  1 file changed, 236 insertions(+), 74 deletions(-)

>  /* functions supporting jsonb_delete, jsonb_set and jsonb_concat */
>  static JsonbValue *IteratorConcat(JsonbIterator **it1, JsonbIterator **it2,
> @@ -2484,12 +2491,12 @@ populate_array_report_expected_array(PopulateArrayContext *ctx, int ndim)
>  	if (ndim <= 0)
>  	{
>  		if (ctx->colname)
> -			ereport(ERROR,
> +			errsave(ctx->escontext,
>  					(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
>  					 errmsg("expected JSON array"),
>  					 errhint("See the value of key \"%s\".", ctx->colname)));
>  		else
> -			ereport(ERROR,
> +			errsave(ctx->escontext,
>  					(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
>  					 errmsg("expected JSON array")));
>  	}
> @@ -2506,13 +2513,13 @@ populate_array_report_expected_array(PopulateArrayContext *ctx, int ndim)
>  			appendStringInfo(&indices, "[%d]", ctx->sizes[i]);
>  
>  		if (ctx->colname)
> -			ereport(ERROR,
> +			errsave(ctx->escontext,
>  					(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
>  					 errmsg("expected JSON array"),
>  					 errhint("See the array element %s of key \"%s\".",
>  							 indices.data, ctx->colname)));
>  		else
> -			ereport(ERROR,
> +			errsave(ctx->escontext,
>  					(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
>  					 errmsg("expected JSON array"),
>  					 errhint("See the array element %s.",
> @@ -2520,8 +2527,13 @@ populate_array_report_expected_array(PopulateArrayContext *ctx, int ndim)
>  	}
>  }

It seems mildly errorprone to use errsave() but not have any returns in the
code after the errsave()s - it seems plausible that somebody later would come
and add more code expecting to not reach the later code.

> +/*
> + * Validate and set ndims for populating an array with some
> + * populate_array_*() function.
> + *
> + * Returns false if the input (ndims) is erratic.

I don't think "erratic" is the right word, "erroneous" maybe?

> From 35cf1759f67a1c8ca7691aa87727a9f2c404b7c2 Mon Sep 17 00:00:00 2001
> From: Amit Langote <[email-masked]>
> Date: Tue, 5 Dec 2023 14:33:25 +0900
> Subject: [PATCH v30 3/5] SQL/JSON query functions
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> This introduces the SQL/JSON functions for querying JSON data using
> jsonpath expressions. The functions are:
> 
> JSON_EXISTS()
> JSON_QUERY()
> JSON_VALUE()
> 
> JSON_EXISTS() tests if the jsonpath expression applied to the jsonb
> value yields any values.
> 
> JSON_VALUE() must return a single value, and an error occurs if it
> tries to return multiple values.
> 
> JSON_QUERY() must return a json object or array, and there are
> various WRAPPER options for handling scalar or multi-value results.
> Both these functions have options for handling EMPTY and ERROR
> conditions.
> 
> All of these functions only operate on jsonb. The workaround for now
> is to cast the argument to jsonb.
> 
> Author: Nikita Glukhov <[email-masked]>
> Author: Teodor Sigaev <[email-masked]>
> Author: Oleg Bartunov <[email-masked]>
> Author: Alexander Korotkov <[email-masked]>
> Author: Andrew Dunstan <[email-masked]>
> Author: Amit Langote <[email-masked]>
> Author: Peter Eisentraut <[email-masked]>
> Author: jian he <[email-masked]>
> 
> Reviewers have included (in no particular order) Andres Freund, Alexander
> Korotkov, Pavel Stehule, Andrew Alsup, Erik Rijkers, Zihong Yu,
> Himanshu Upadhyaya, Daniel Gustafsson, Justin Pryzby, Álvaro Herrera,
> jian he, Anton A. Melnikov, Nikita Malakhov, Peter Eisentraut
> 
> Discussion: https://postgr.es/m/[email-masked]
> Discussion: https://postgr.es/m/[email-masked]
> Discussion: https://postgr.es/m/abd9b83b-aa66-f230-3d6d-734817f0995d%40postgresql.org
> Discussion: https://postgr.es/m/CA+HiwqE4XTdfb1nW=[email-masked]
> ---
>  doc/src/sgml/func.sgml                      |  151 +++
>  src/backend/catalog/sql_features.txt        |   12 +-
>  src/backend/executor/execExpr.c             |  363 +++++++
>  src/backend/executor/execExprInterp.c       |  365 ++++++-
>  src/backend/jit/llvm/llvmjit.c              |    2 +
>  src/backend/jit/llvm/llvmjit_expr.c         |  140 +++
>  src/backend/jit/llvm/llvmjit_types.c        |    4 +
>  src/backend/nodes/makefuncs.c               |   18 +
>  src/backend/nodes/nodeFuncs.c               |  238 ++++-
>  src/backend/optimizer/path/costsize.c       |    3 +-
>  src/backend/optimizer/util/clauses.c        |   19 +
>  src/backend/parser/gram.y                   |  178 +++-
>  src/backend/parser/parse_expr.c             |  621 ++++++++++-
>  src/backend/parser/parse_target.c           |   15 +
>  src/backend/utils/adt/formatting.c          |   44 +
>  src/backend/utils/adt/jsonb.c               |   31 +
>  src/backend/utils/adt/jsonfuncs.c           |   52 +-
>  src/backend/utils/adt/jsonpath.c            |  255 +++++
>  src/backend/utils/adt/jsonpath_exec.c       |  391 ++++++-
>  src/backend/utils/adt/ruleutils.c           |  136 +++
>  src/include/executor/execExpr.h             |  133 +++
>  src/include/fmgr.h                          |    1 +
>  src/include/jit/llvmjit.h                   |    1 +
>  src/include/nodes/makefuncs.h               |    2 +
>  src/include/nodes/parsenodes.h              |   47 +
>  src/include/nodes/primnodes.h               |  130 +++
>  src/include/parser/kwlist.h                 |   11 +
>  src/include/utils/formatting.h              |    1 +
>  src/include/utils/jsonb.h                   |    1 +
>  src/include/utils/jsonfuncs.h               |    5 +
>  src/include/utils/jsonpath.h                |   27 +
>  src/interfaces/ecpg/preproc/ecpg.trailer    |   28 +
>  src/test/regress/expected/json_sqljson.out  |   18 +
>  src/test/regress/expected/jsonb_sqljson.out | 1032 +++++++++++++++++++
>  src/test/regress/parallel_schedule          |    2 +-
>  src/test/regress/sql/json_sqljson.sql       |   11 +
>  src/test/regress/sql/jsonb_sqljson.sql      |  337 ++++++
>  src/tools/pgindent/typedefs.list            |   18 +
>  38 files changed, 4767 insertions(+), 76 deletions(-)

I think it'd be worth trying to break this into smaller bits - it's not easy
to review this at once.

> +/*
> + * Information about the state of JsonPath* evaluation.
> + */
> +typedef struct JsonExprPostEvalState
> +{
> +	/* Did JsonPath* evaluation cause an error? */
> +	NullableDatum	error;
> +
> +	/* Is the result of JsonPath* evaluation empty? */
> +	NullableDatum	empty;
> +
> +	/*
> +	 * ExecEvalJsonExprPath() will set this to the address of the step to
> +	 * use to coerce the result of JsonPath* evaluation to the RETURNING type.
> +	 * Also see the description of possible step addresses that this could be
> +	 * set to in the definition of JsonExprState.
> +	 */
> +#define FIELDNO_JSONEXPRPOSTEVALSTATE_JUMP_EVAL_COERCION	2
> +	int			jump_eval_coercion;
> +} JsonExprPostEvalState;
> +
> +/* State for evaluating a JsonExpr, too big to inline */
> +typedef struct JsonExprState
> +{
> +	/* original expression node */
> +	JsonExpr   *jsexpr;
> +
> +	/* value/isnull for formatted_expr */
> +	NullableDatum formatted_expr;
> +
> +	/* value/isnull for pathspec */
> +	NullableDatum pathspec;
> +
> +	/* JsonPathVariable entries for passing_values */
> +	List	   *args;
> +
> +	/*
> +	 * Per-row result status info populated by ExecEvalJsonExprPath()
> +	 * and ExecEvalJsonCoercionFinish().
> +	 */
> +	JsonExprPostEvalState post_eval;
> +
> +	/*
> +	 * Address of the step that implements the non-ERROR variant of ON ERROR
> +	 * and ON EMPTY behaviors, to be jumped to when ExecEvalJsonExprPath()
> +	 * returns false on encountering an error during JsonPath* evaluation
> +	 * (ON ERROR) or on finding that no matching JSON item was returned (ON
> +	 * EMPTY).  The same steps are also performed on encountering an error
> +	 * when coercing JsonPath* result to the RETURNING type.
> +	 */
> +	int			jump_error;
> +
> +	/*
> +	 * Addresses of steps to perform the coercion of the JsonPath* result value
> +	 * to the RETURNING type.  Each address points to either 1) a special
> +	 * EEOP_JSONEXPR_COERCION step that handles coercion using the RETURNING
> +	 * type's input function or by using json_via_populate(), or 2) an
> +	 * expression such as CoerceViaIO.  It may be -1 if no coercion is
> +	 * necessary.
> +	 *
> +	 * jump_eval_result_coercion points to the step to evaluate the coercion
> +	 * given in JsonExpr.result_coercion.
> +	 */
> +	int			jump_eval_result_coercion;
> +
> +	/* eval_item_coercion_jumps is an array of num_item_coercions elements
> +	 * each containing a step address to evaluate the coercion from a value of
> +	 * the given JsonItemType to the RETURNING type, or -1 if no coercion is
> +	 * necessary.  item_coercion_via_expr is an array of boolean flags of the
> +	 * same length that indicates whether each valid step address in the
> +	 * eval_item_coercion_jumps array points to an expression or a
> +	 * EEOP_JSONEXPR_COERCION step.  ExecEvalJsonExprPath() will cause an
> +	 * error if it's the latter, because that mode of coercion is not
> +	 * supported for all JsonItemTypes.
> +	 */
> +	int			num_item_coercions;
> +	int		   *eval_item_coercion_jumps;
> +	bool	   *item_coercion_via_expr;
> +
> +	/*
> +	 * For passing when initializing a EEOP_IOCOERCE_SAFE step for any
> +	 * CoerceViaIO nodes in the expression that must be evaluated in an
> +	 * error-safe manner.
> +	 */
> +	ErrorSaveContext escontext;
> +} JsonExprState;
> +
> +/*
> + * State for coercing a value to the target type specified in 'coercion' using
> + * either json_populate_type() or by calling the type's input function.
> + */
> +typedef struct JsonCoercionState
> +{
> +	/* original expression node */
> +	JsonCoercion   *coercion;
> +
> +	/* Input function info for the target type. */
> +	struct
> +	{
> +		FmgrInfo   *finfo;
> +		Oid			typioparam;
> +	}			input;
> +
> +	/* Cache for json_populate_type() */
> +	void	   *cache;
> +
> +	/*
> +	 * For soft-error handling in json_populate_type() or
> +	 * in InputFunctionCallSafe().
> +	 */
> +	ErrorSaveContext *escontext;
> +} JsonCoercionState;

Does all of this stuff need to live in this header? Some of it seems like it
doesn't need to be in a header at all, and other bits seem like they belong
somewhere more json specific?

> +/*
> + * JsonItemType
> + *		Represents type codes to identify a JsonCoercion node to use when
> + *		coercing a given SQL/JSON items to the output SQL type
> + *
> + * The comment next to each item type mentions the JsonbValue.jbvType of the
> + * source JsonbValue value to be coerced using the expression in the
> + * JsonCoercion node.
> + *
> + * Also, see InitJsonItemCoercions() and ExecPrepareJsonItemCoercion().
> + */
> +typedef enum JsonItemType
> +{
> +	JsonItemTypeNull = 0,		/* jbvNull */
> +	JsonItemTypeString = 1,		/* jbvString */
> +	JsonItemTypeNumeric = 2,	/* jbvNumeric */
> +	JsonItemTypeBoolean = 3,	/* jbvBool */
> +	JsonItemTypeDate = 4,		/* jbvDatetime: DATEOID */
> +	JsonItemTypeTime = 5,		/* jbvDatetime: TIMEOID */
> +	JsonItemTypeTimetz = 6,		/* jbvDatetime: TIMETZOID */
> +	JsonItemTypeTimestamp = 7,	/* jbvDatetime: TIMESTAMPOID */
> +	JsonItemTypeTimestamptz = 8,	/* jbvDatetime: TIMESTAMPTZOID */
> +	JsonItemTypeComposite = 9,	/* jbvArray, jbvObject, jbvBinary */
> +	JsonItemTypeInvalid = 10,
> +} JsonItemType;

Why do we need manually assigned values here?

> +/*
> + * JsonCoercion -
> + *		coercion from SQL/JSON item types to SQL types
> + */
> +typedef struct JsonCoercion
> +{
> +	NodeTag		type;
> +
> +	Oid			targettype;
> +	int32		targettypmod;
> +	bool		via_populate;	/* coerce result using json_populate_type()? */
> +	bool		via_io;			/* coerce result using type input function? */
> +	Oid			collation;		/* collation for coercion via I/O or populate */
> +} JsonCoercion;
> +
> +typedef struct JsonItemCoercion
> +{
> +	NodeTag		type;
> +
> +	JsonItemType item_type;
> +	Node	   *coercion;
> +} JsonItemCoercion;

What's the difference between an "ItemCoercion" and a "Coercion"?

> +/*
> + * JsonBehavior -
> + * 		representation of a given JSON behavior

My editor warns about space-before-tab here.

> + */
> +typedef struct JsonBehavior
> +{
> +	NodeTag		type;

> +	JsonBehaviorType btype;		/* behavior type */
> +	Node	   *expr;			/* behavior expression */

These comment don't seem helpful. I think there's need for comments here, but
restating the field name in different words isn't helpful. What's needed is an
explanation of how things interact, perhaps also why that's the appropriate
representation.

> +	JsonCoercion *coercion;		/* to coerce behavior expression when there is
> +								 * no cast to the target type */
> +	int			location;		/* token location, or -1 if unknown */

> +} JsonBehavior;
> +
> +/*
> + * JsonExpr -
> + *		transformed representation of JSON_VALUE(), JSON_QUERY(), JSON_EXISTS()
> + */
> +typedef struct JsonExpr
> +{
> +	Expr		xpr;
> +
> +	JsonExprOp	op;				/* json function ID */
> +	Node	   *formatted_expr; /* formatted context item expression */
> +	Node	   *result_coercion; /* resulting coercion to RETURNING type */
> +	JsonFormat *format;			/* context item format (JSON/JSONB) */
> +	Node	   *path_spec;		/* JSON path specification expression */
> +	List	   *passing_names;	/* PASSING argument names */
> +	List	   *passing_values; /* PASSING argument values */
> +	JsonReturning *returning;	/* RETURNING clause type/format info */
> +	JsonBehavior *on_empty;		/* ON EMPTY behavior */
> +	JsonBehavior *on_error;		/* ON ERROR behavior */
> +	List	   *item_coercions; /* coercions for JSON_VALUE */
> +	JsonWrapper wrapper;		/* WRAPPER for JSON_QUERY */
> +	bool		omit_quotes;	/* KEEP/OMIT QUOTES for JSON_QUERY */
> +	int			location;		/* token location, or -1 if unknown */
> +} JsonExpr;

These comments seem even worse.

> +static void ExecInitJsonExpr(JsonExpr *jexpr, ExprState *state,
> +							 Datum *resv, bool *resnull,
> +							 ExprEvalStep *scratch);
> +static int ExecInitJsonExprCoercion(ExprState *state, Node *coercion,
> +						 ErrorSaveContext *escontext,
> +						 Datum *resv, bool *resnull);
>  
>  
>  /*
> @@ -2416,6 +2423,36 @@ ExecInitExprRec(Expr *node, ExprState *state,
>  				break;
>  			}
>  
> +		case T_JsonExpr:
> +			{
> +				JsonExpr   *jexpr = castNode(JsonExpr, node);
> +
> +				ExecInitJsonExpr(jexpr, state, resv, resnull, &scratch);
> +				break;
> +			}
> +
> +		case T_JsonCoercion:
> +			{
> +				JsonCoercion	*coercion = castNode(JsonCoercion, node);
> +				JsonCoercionState *jcstate = palloc0(sizeof(JsonCoercionState));
> +				Oid			typinput;
> +				FmgrInfo   *finfo;
> +
> +				getTypeInputInfo(coercion->targettype, &typinput,
> +								 &jcstate->input.typioparam);
> +				finfo = palloc0(sizeof(FmgrInfo));
> +				fmgr_info(typinput, finfo);
> +				jcstate->input.finfo = finfo;
> +
> +				jcstate->coercion = coercion;
> +				jcstate->escontext = state->escontext;
> +
> +				scratch.opcode = EEOP_JSONEXPR_COERCION;
> +				scratch.d.jsonexpr_coercion.jcstate = jcstate;
> +				ExprEvalPushStep(state, &scratch);
> +				break;
> +			}

It's confusing that we have ExecInitJsonExprCoercion, but aren't using that
here, but then use it later, in ExecInitJsonExpr().

>  		case T_NullTest:
>  			{
>  				NullTest   *ntest = (NullTest *) node;
> @@ -4184,3 +4221,329 @@ ExecBuildParamSetEqual(TupleDesc desc,
>  
>  	return state;
>  }
> +
> +/*
> + * Push steps to evaluate a JsonExpr and its various subsidiary expressions.
> + */
> +static void
> +ExecInitJsonExpr(JsonExpr *jexpr, ExprState *state,
> +				 Datum *resv, bool *resnull,
> +				 ExprEvalStep *scratch)
> +{
> +	JsonExprState *jsestate = palloc0(sizeof(JsonExprState));
> +	JsonExprPostEvalState *post_eval = &jsestate->post_eval;
> +	ListCell   *argexprlc;
> +	ListCell   *argnamelc;
> +	List	   *jumps_if_skip = NIL;
> +	List	   *jumps_to_coerce_finish = NIL;
> +	List	   *jumps_to_end = NIL;
> +	ListCell   *lc;
> +	ExprEvalStep *as;
> +
> +	jsestate->jsexpr = jexpr;
> +
> +	/*
> +	 * Evaluate formatted_expr storing the result into
> +	 * jsestate->formatted_expr.
> +	 */
> +	ExecInitExprRec((Expr *) jexpr->formatted_expr, state,
> +					&jsestate->formatted_expr.value,
> +					&jsestate->formatted_expr.isnull);
> +
> +	/* Steps to jump to end if formatted_expr evaluates to NULL */
> +	scratch->opcode = EEOP_JUMP_IF_NULL;
> +	scratch->resnull = &jsestate->formatted_expr.isnull;
> +	scratch->d.jump.jumpdone = -1;	/* set below */
> +	jumps_if_skip = lappend_int(jumps_if_skip, state->steps_len);
> +	ExprEvalPushStep(state, scratch);
> +
> +	/*
> +	 * Evaluate pathspec expression storing the result into
> +	 * jsestate->pathspec.
> +	 */
> +	ExecInitExprRec((Expr *) jexpr->path_spec, state,
> +					&jsestate->pathspec.value,
> +					&jsestate->pathspec.isnull);
> +
> +	/* Steps to JUMP to end if pathspec evaluates to NULL */
> +	scratch->opcode = EEOP_JUMP_IF_NULL;
> +	scratch->resnull = &jsestate->pathspec.isnull;
> +	scratch->d.jump.jumpdone = -1;	/* set below */
> +	jumps_if_skip = lappend_int(jumps_if_skip, state->steps_len);
> +	ExprEvalPushStep(state, scratch);
> +
> +	/* Steps to compute PASSING args. */
> +	jsestate->args = NIL;
> +	forboth(argexprlc, jexpr->passing_values,
> +			argnamelc, jexpr->passing_names)
> +	{
> +		Expr	   *argexpr = (Expr *) lfirst(argexprlc);
> +		String	   *argname = lfirst_node(String, argnamelc);
> +		JsonPathVariable *var = palloc(sizeof(*var));
> +
> +		var->name = argname->sval;
> +		var->typid = exprType((Node *) argexpr);
> +		var->typmod = exprTypmod((Node *) argexpr);
> +
> +		ExecInitExprRec((Expr *) argexpr, state, &var->value, &var->isnull);
> +
> +		jsestate->args = lappend(jsestate->args, var);
> +	}
> +
> +	/* Step for JsonPath* evaluation; see ExecEvalJsonExprPath(). */
> +	scratch->opcode = EEOP_JSONEXPR_PATH;
> +	scratch->resvalue = resv;
> +	scratch->resnull = resnull;
> +	scratch->d.jsonexpr.jsestate = jsestate;
> +	ExprEvalPushStep(state, scratch);
> +
> +	/*
> +	 * Step to jump to end when there's neither an error when evaluating
> +	 * JsonPath* nor any need to coerce the result because it's already
> +	 * of the specified type.
> +	 */
> +	scratch->opcode = EEOP_JUMP;
> +	scratch->d.jump.jumpdone = -1;	/* set below */
> +	jumps_to_end = lappend_int(jumps_to_end, state->steps_len);
> +	ExprEvalPushStep(state, scratch);
> +
> +	/*
> +	 * Steps to coerce the result value computed by EEOP_JSONEXPR_PATH.
> +	 * To handle coercion errors softly, use the following ErrorSaveContext
> +	 * when initializing the coercion expressions, including any JsonCoercion
> +	 * nodes.
> +	 */
> +	jsestate->escontext.type = T_ErrorSaveContext;
> +	if (jexpr->result_coercion || jexpr->omit_quotes)
> +	{
> +		jsestate->jump_eval_result_coercion =
> +			ExecInitJsonExprCoercion(state, jexpr->result_coercion,
> +									 jexpr->on_error->btype != JSON_BEHAVIOR_ERROR ?
> +									 &jsestate->escontext : NULL,
> +									 resv, resnull);
> +	}
> +	else
> +		jsestate->jump_eval_result_coercion = -1;
> +
> +	/* Steps for coercing JsonItemType values returned by JsonPathValue(). */
> +	if (jexpr->item_coercions)
> +	{
> +		/*
> +		 * Jump to COERCION_FINISH to skip over the following steps if
> +		 * result_coercion is present.
> +		 */
> +		if (jsestate->jump_eval_result_coercion >= 0)
> +		{
> +			scratch->opcode = EEOP_JUMP;
> +			scratch->d.jump.jumpdone = -1;	/* set below */
> +			jumps_to_coerce_finish = lappend_int(jumps_to_coerce_finish,
> +												 state->steps_len);
> +			ExprEvalPushStep(state, scratch);
> +		}
> +
> +		/*
> +		 * Here we create the steps for each JsonItemType type's coercion
> +		 * expression and also store a flag whether the expression is
> +		 * a JsonCoercion node.  ExecPrepareJsonItemCoercion() called by
> +		 * ExecEvalJsonExprPath() will map a given JsonbValue returned by
> +		 * JsonPathValue() to its  JsonItemType's expression's step address
> +		 * and the flag by indexing the following arrays with JsonItemType
> +		 * enum value.
> +		 */
> +		jsestate->num_item_coercions = list_length(jexpr->item_coercions);
> +		jsestate->eval_item_coercion_jumps = (int *)
> +			palloc(jsestate->num_item_coercions * sizeof(int));
> +		jsestate->item_coercion_via_expr = (bool *)
> +			palloc0(jsestate->num_item_coercions * sizeof(bool));
> +		foreach(lc, jexpr->item_coercions)
> +		{
> +			JsonItemCoercion *item_coercion = lfirst(lc);
> +			Node *coercion = item_coercion->coercion;
> +
> +			jsestate->item_coercion_via_expr[item_coercion->item_type] =
> +				(coercion != NULL && !IsA(coercion, JsonCoercion));
> +			jsestate->eval_item_coercion_jumps[item_coercion->item_type] =
> +				ExecInitJsonExprCoercion(state, coercion,
> +										 jexpr->on_error->btype != JSON_BEHAVIOR_ERROR ?
> +										 &jsestate->escontext : NULL,
> +										 resv, resnull);
> +
> +			/* Emit JUMP step to skip past other coercions' steps. */
> +			scratch->opcode = EEOP_JUMP;
> +			scratch->d.jump.jumpdone = -1;	/* set below */
> +			jumps_to_coerce_finish = lappend_int(jumps_to_coerce_finish,
> +												 state->steps_len);
> +			ExprEvalPushStep(state, scratch);
> +		}
> +	}
> +
> +	/*
> +	 * Add step to reset the ErrorSaveContext and set error flag if the
> +	 * coercion steps encountered an error but was not thrown because of the
> +	 * ON ERROR behavior.
> +	 */
> +	if (jexpr->result_coercion || jexpr->item_coercions)
> +	{
> +		foreach(lc, jumps_to_coerce_finish)
> +		{
> +			as = &state->steps[lfirst_int(lc)];
> +			as->d.jump.jumpdone = state->steps_len;
> +		}
> +
> +		scratch->opcode = EEOP_JSONEXPR_COERCION_FINISH;
> +		scratch->d.jsonexpr.jsestate = jsestate;
> +		ExprEvalPushStep(state, scratch);
> +	}
> +
> +	/*
> +	 * Step to handle ON ERROR behaviors.  This handles both the errors
> +	 * that occur during EEOP_JSONEXPR_PATH evaluation and subsequent coercion
> +	 * evaluation.
> +	 */
> +	jsestate->jump_error = -1;
> +	if (jexpr->on_error &&
> +		jexpr->on_error->btype != JSON_BEHAVIOR_ERROR)
> +	{
> +		jsestate->jump_error = state->steps_len;
> +		scratch->opcode = EEOP_JUMP_IF_NOT_TRUE;
> +
> +		/*
> +		 * post_eval.error is set by ExecEvalJsonExprPath() and
> +		 * ExecEvalJsonCoercionFinish().
> +		 */
> +		scratch->resvalue = &post_eval->error.value;
> +		scratch->resnull = &post_eval->error.isnull;
> +
> +		scratch->d.jump.jumpdone = -1;	/* set below */
> +		ExprEvalPushStep(state, scratch);
> +
> +		/* Steps to evaluate the ON ERROR expression */
> +		ExecInitExprRec((Expr *) jexpr->on_error->expr,
> +						state, resv, resnull);
> +
> +		/* Steps to coerce the ON ERROR expression if needed */
> +		if (jexpr->on_error->coercion)
> +			ExecInitExprRec((Expr *) jexpr->on_error->coercion, state,
> +							 resv, resnull);
> +
> +		jumps_to_end = lappend_int(jumps_to_end, state->steps_len);
> +		scratch->opcode = EEOP_JUMP;
> +		scratch->d.jump.jumpdone = -1;
> +		ExprEvalPushStep(state, scratch);
> +	}
> +
> +	/* Step to handle ON EMPTY behaviors. */
> +	if (jexpr->on_empty != NULL &&
> +		jexpr->on_empty->btype != JSON_BEHAVIOR_ERROR)
> +	{
> +		/*
> +		 * Make the ON ERROR behavior JUMP to here after checking the error
> +		 * and if it's not present then make EEOP_JSONEXPR_PATH directly
> +		 * jump here.
> +		 */
> +		if (jsestate->jump_error >= 0)
> +		{
> +			as = &state->steps[jsestate->jump_error];
> +			as->d.jump.jumpdone = state->steps_len;
> +		}
> +		else
> +			jsestate->jump_error = state->steps_len;
> +
> +		scratch->opcode = EEOP_JUMP_IF_NOT_TRUE;
> +		scratch->resvalue = &post_eval->empty.value;
> +		scratch->resnull = &post_eval->empty.isnull;
> +		scratch->d.jump.jumpdone = -1;	/* set below */
> +		jumps_to_end = lappend_int(jumps_to_end, state->steps_len);
> +		ExprEvalPushStep(state, scratch);
> +
> +		/* Steps to evaluate the ON EMPTY expression */
> +		ExecInitExprRec((Expr *) jexpr->on_empty->expr,
> +						state, resv, resnull);
> +
> +		/* Steps to coerce the ON EMPTY expression if needed */
> +		if (jexpr->on_empty->coercion)
> +			ExecInitExprRec((Expr *) jexpr->on_empty->coercion, state,
> +							 resv, resnull);
> +
> +		scratch->opcode = EEOP_JUMP;
> +		scratch->d.jump.jumpdone = -1;	/* set below */
> +		jumps_to_end = lappend_int(jumps_to_end, state->steps_len);
> +		ExprEvalPushStep(state, scratch);
> +	}
> +	/* Make EEOP_JSONEXPR_PATH jump to end if no ON EMPTY clause present. */
> +	else if (jsestate->jump_error >= 0)
> +		jumps_to_end = lappend_int(jumps_to_end, jsestate->jump_error);
> +
> +	/*
> +	 * If neither ON ERROR nor ON EMPTY jumps present, then add one to go
> +	 * to end.
> +	 */
> +	if (jsestate->jump_error < 0)
> +	{
> +		scratch->opcode = EEOP_JUMP;
> +		scratch->d.jump.jumpdone = -1;	/* set below */
> +		jumps_to_end = lappend_int(jumps_to_end, state->steps_len);
> +		ExprEvalPushStep(state, scratch);
> +	}
> +
> +	/* Return NULL when either formatted_expr or pathspec is NULL. */
> +	foreach(lc, jumps_if_skip)
> +	{
> +		as = &state->steps[lfirst_int(lc)];
> +		as->d.jump.jumpdone = state->steps_len;
> +	}
> +	scratch->opcode = EEOP_CONST;
> +	scratch->resvalue = resv;
> +	scratch->resnull = resnull;
> +	scratch->d.constval.value = (Datum) 0;
> +	scratch->d.constval.isnull = true;
> +	ExprEvalPushStep(state, scratch);
> +
> +	/* Jump to coerce the NULL using result_coercion is present. */
> +	if (jsestate->jump_eval_result_coercion >= 0)
> +	{
> +		scratch->opcode = EEOP_JUMP;
> +		scratch->d.jump.jumpdone = jsestate->jump_eval_result_coercion;
> +		ExprEvalPushStep(state, scratch);
> +	}
> +
> +	foreach(lc, jumps_to_end)
> +	{
> +		as = &state->steps[lfirst_int(lc)];
> +		as->d.jump.jumpdone = state->steps_len;
> +	}
> +}
> +
> +/* Initialize one JsonCoercion for execution. */
> +static int
> +ExecInitJsonExprCoercion(ExprState *state, Node *coercion,
> +						 ErrorSaveContext *escontext,
> +						 Datum *resv, bool *resnull)
> +{
> +	int			jump_eval_coercion;
> +	Datum	   *save_innermost_caseval;
> +	bool	   *save_innermost_casenull;
> +	ErrorSaveContext *save_escontext;
> +
> +	if (coercion == NULL)
> +		return -1;
> +
> +	jump_eval_coercion = state->steps_len;
> +
> +	/* Push step(s) to compute cstate->coercion. */
> +	save_innermost_caseval = state->innermost_caseval;
> +	save_innermost_casenull = state->innermost_casenull;
> +	save_escontext = state->escontext;
> +
> +	state->innermost_caseval = resv;
> +	state->innermost_casenull = resnull;
> +	state->escontext = escontext;
> +
> +	ExecInitExprRec((Expr *) coercion, state, resv, resnull);
> +
> +	state->innermost_caseval = save_innermost_caseval;
> +	state->innermost_casenull = save_innermost_casenull;
> +	state->escontext = save_escontext;
> +
> +	return jump_eval_coercion;
> +}
> diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
> index d5db96444c..a18662cbf9 100644
> --- a/src/backend/executor/execExprInterp.c
> +++ b/src/backend/executor/execExprInterp.c
> @@ -73,8 +73,8 @@
>  #include "utils/datum.h"
>  #include "utils/expandedrecord.h"
>  #include "utils/json.h"
> -#include "utils/jsonb.h"
>  #include "utils/jsonfuncs.h"
> +#include "utils/jsonpath.h"
>  #include "utils/lsyscache.h"
>  #include "utils/memutils.h"
>  #include "utils/timestamp.h"
> @@ -181,6 +181,10 @@ static pg_attribute_always_inline void ExecAggPlainTransByRef(AggState *aggstate
>  															  AggStatePerGroup pergroup,
>  															  ExprContext *aggcontext,
>  															  int setno);
> +static void ExecPrepareJsonItemCoercion(JsonbValue *item, JsonExprState *jsestate,
> +							bool throw_error,
> +							int *jump_eval_item_coercion,
> +							Datum *resvalue, bool *resnull);
>  
>  /*
>   * ScalarArrayOpExprHashEntry
> @@ -482,6 +486,9 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
>  		&&CASE_EEOP_XMLEXPR,
>  		&&CASE_EEOP_JSON_CONSTRUCTOR,
>  		&&CASE_EEOP_IS_JSON,
> +		&&CASE_EEOP_JSONEXPR_PATH,
> +		&&CASE_EEOP_JSONEXPR_COERCION,
> +		&&CASE_EEOP_JSONEXPR_COERCION_FINISH,
>  		&&CASE_EEOP_AGGREF,
>  		&&CASE_EEOP_GROUPING_FUNC,
>  		&&CASE_EEOP_WINDOW_FUNC,
> @@ -1551,6 +1558,35 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
>  			EEO_NEXT();
>  		}
>  
> +		EEO_CASE(EEOP_JSONEXPR_PATH)
> +		{
> +			JsonExprState *jsestate = op->d.jsonexpr.jsestate;
> +
> +			/* too complex for an inline implementation */
> +			if (!ExecEvalJsonExprPath(state, op, econtext))
> +				EEO_JUMP(jsestate->jump_error);
> +			else if (jsestate->post_eval.jump_eval_coercion >= 0)
> +				EEO_JUMP(jsestate->post_eval.jump_eval_coercion);
> +
> +			EEO_NEXT();
> +		}

Why do we need post_eval.jump_eval_coercion? Seems like that could more
cleanly be implemented by just emitting a constant JUMP step?  Oh, I see -
you're changing post_eval.jump_eval_coercion at runtime.  This seems like a
BAD idea.  I strongly suggest that instead of modifying the field, you instead
return the target jump step as a return value from ExecEvalJsonExprPath or
such.

> +/*
> + * Performs JsonPath{Exists|Query|Value}() for given context item and JSON
> + * path.
> + *
> + * Result is set in *op->resvalue and *op->resnull.
> + *
> + * On return, JsonExprPostEvalState is populated with the following details:
> + *	- jump_eval_coercion: step address of coercion to apply to the result
> + *	- error.value: true if an error occurred during JsonPath evaluation
> + *	- empty.value: true if JsonPath{Query|Value}() found no matching item
> + *
> + * No return if the ON ERROR/EMPTY behavior is ERROR.
> + */
> +bool
> +ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op,
> +					 ExprContext *econtext)
> +{
> +	JsonExprState *jsestate = op->d.jsonexpr.jsestate;
> +	JsonExprPostEvalState *post_eval = &jsestate->post_eval;
> +	JsonExpr   *jexpr = jsestate->jsexpr;
> +	Datum		item;
> +	JsonPath   *path;
> +	bool		throw_error = (jexpr->on_error->btype == JSON_BEHAVIOR_ERROR);

What's the deal with the parentheses here and in similar places below? There's
no danger of ambiguity without, no?

> +		case JSON_VALUE_OP:
> +			{
> +				JsonbValue *jbv = JsonPathValue(item, path, &empty,
> +												!throw_error ? &error : NULL,
> +												jsestate->args);
> +
> +				/* Might get overridden by an item coercion below. */
> +				post_eval->jump_eval_coercion = jsestate->jump_eval_result_coercion;
> +				if (jbv == NULL)
> +				{
> +					/* Will be coerced with result_coercion. */
> +					*op->resvalue = (Datum) 0;
> +					*op->resnull = true;
> +				}
> +				else if (!error && !empty)
> +				{
> +					/*
> +					 * If the requested output type is json(b), use
> +					 * result_coercion to do the coercion.
> +					 */
> +					if (jexpr->returning->typid == JSONOID ||
> +						jexpr->returning->typid == JSONBOID)
> +					{
> +						*op->resvalue = JsonbPGetDatum(JsonbValueToJsonb(jbv));
> +						*op->resnull = false;
> +					}
> +					else
> +					{
> +						/*
> +						 * Else, use one of the item_coercions.
> +						 *
> +						 * Error out if no cast expression exists.
> +						 */
> +						ExecPrepareJsonItemCoercion(jbv, jsestate, throw_error,
> +													&post_eval->jump_eval_coercion,
> +													op->resvalue, op->resnull);

> +	if (empty)
> +	{
> +		if (jexpr->on_empty)
> +		{
> +			if (jexpr->on_empty->btype == JSON_BEHAVIOR_ERROR)
> +				ereport(ERROR,
> +						(errcode(ERRCODE_NO_SQL_JSON_ITEM),
> +						 errmsg("no SQL/JSON item")));

No need for the parens around ereport() arguments anymore. Same in a few other places.

> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index d631ac89a9..4f92d000ec 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -650,11 +650,18 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
>  				json_returning_clause_opt
>  				json_name_and_value
>  				json_aggregate_func
> +				json_argument
> +				json_behavior
>  %type <list>	json_name_and_value_list
>  				json_value_expr_list
>  				json_array_aggregate_order_by_clause_opt
> +				json_arguments
> +				json_behavior_clause_opt
> +				json_passing_clause_opt
>  %type <ival>	json_encoding_clause_opt
>  				json_predicate_type_constraint
> +				json_quotes_clause_opt
> +				json_wrapper_behavior
>  %type <boolean>	json_key_uniqueness_constraint_opt
>  				json_object_constructor_null_clause_opt
>  				json_array_constructor_null_clause_opt
> @@ -695,7 +702,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
>  	CACHE CALL CALLED CASCADE CASCADED CASE CAST CATALOG_P CHAIN CHAR_P
>  	CHARACTER CHARACTERISTICS CHECK CHECKPOINT CLASS CLOSE
>  	CLUSTER COALESCE COLLATE COLLATION COLUMN COLUMNS COMMENT COMMENTS COMMIT
> -	COMMITTED COMPRESSION CONCURRENTLY CONFIGURATION CONFLICT
> +	COMMITTED COMPRESSION CONCURRENTLY CONDITIONAL CONFIGURATION CONFLICT
>  	CONNECTION CONSTRAINT CONSTRAINTS CONTENT_P CONTINUE_P CONVERSION_P COPY
>  	COST CREATE CROSS CSV CUBE CURRENT_P
>  	CURRENT_CATALOG CURRENT_DATE CURRENT_ROLE CURRENT_SCHEMA
> @@ -706,8 +713,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
>  	DETACH DICTIONARY DISABLE_P DISCARD DISTINCT DO DOCUMENT_P DOMAIN_P
>  	DOUBLE_P DROP
>  
> -	EACH ELSE ENABLE_P ENCODING ENCRYPTED END_P ENUM_P ESCAPE EVENT EXCEPT
> -	EXCLUDE EXCLUDING EXCLUSIVE EXECUTE EXISTS EXPLAIN EXPRESSION
> +	EACH ELSE EMPTY_P ENABLE_P ENCODING ENCRYPTED END_P ENUM_P ERROR_P ESCAPE
> +	EVENT EXCEPT EXCLUDE EXCLUDING EXCLUSIVE EXECUTE EXISTS EXPLAIN EXPRESSION
>  	EXTENSION EXTERNAL EXTRACT
>  
>  	FALSE_P FAMILY FETCH FILTER FINALIZE FIRST_P FLOAT_P FOLLOWING FOR
> @@ -722,10 +729,10 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
>  	INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER
>  	INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION
>  
> -	JOIN JSON JSON_ARRAY JSON_ARRAYAGG JSON_OBJECT JSON_OBJECTAGG
> -	JSON_SCALAR JSON_SERIALIZE
> +	JOIN JSON JSON_ARRAY JSON_ARRAYAGG JSON_EXISTS JSON_OBJECT JSON_OBJECTAGG
> +	JSON_QUERY JSON_SCALAR JSON_SERIALIZE JSON_VALUE
>  
> -	KEY KEYS
> +	KEEP KEY KEYS
>  
>  	LABEL LANGUAGE LARGE_P LAST_P LATERAL_P
>  	LEADING LEAKPROOF LEAST LEFT LEVEL LIKE LIMIT LISTEN LOAD LOCAL
> @@ -739,7 +746,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
>  	NOT NOTHING NOTIFY NOTNULL NOWAIT NULL_P NULLIF
>  	NULLS_P NUMERIC
>  
> -	OBJECT_P OF OFF OFFSET OIDS OLD ON ONLY OPERATOR OPTION OPTIONS OR
> +	OBJECT_P OF OFF OFFSET OIDS OLD OMIT ON ONLY OPERATOR OPTION OPTIONS OR
>  	ORDER ORDINALITY OTHERS OUT_P OUTER_P
>  	OVER OVERLAPS OVERLAY OVERRIDING OWNED OWNER
>  
> @@ -748,7 +755,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
>  	POSITION PRECEDING PRECISION PRESERVE PREPARE PREPARED PRIMARY
>  	PRIOR PRIVILEGES PROCEDURAL PROCEDURE PROCEDURES PROGRAM PUBLICATION
>  
> -	QUOTE
> +	QUOTE QUOTES
>  
>  	RANGE READ REAL REASSIGN RECHECK RECURSIVE REF_P REFERENCES REFERENCING
>  	REFRESH REINDEX RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA
> @@ -759,7 +766,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
>  	SEQUENCE SEQUENCES
>  	SERIALIZABLE SERVER SESSION SESSION_USER SET SETS SETOF SHARE SHOW
>  	SIMILAR SIMPLE SKIP SMALLINT SNAPSHOT SOME SQL_P STABLE STANDALONE_P
> -	START STATEMENT STATISTICS STDIN STDOUT STORAGE STORED STRICT_P STRIP_P
> +	START STATEMENT STATISTICS STDIN STDOUT STORAGE STORED STRICT_P STRING_P STRIP_P
>  	SUBSCRIPTION SUBSTRING SUPPORT SYMMETRIC SYSID SYSTEM_P SYSTEM_USER
>  
>  	TABLE TABLES TABLESAMPLE TABLESPACE TEMP TEMPLATE TEMPORARY TEXT_P THEN
> @@ -767,7 +774,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
>  	TREAT TRIGGER TRIM TRUE_P
>  	TRUNCATE TRUSTED TYPE_P TYPES_P
>  
> -	UESCAPE UNBOUNDED UNCOMMITTED UNENCRYPTED UNION UNIQUE UNKNOWN
> +	UESCAPE UNBOUNDED UNCONDITIONAL UNCOMMITTED UNENCRYPTED UNION UNIQUE UNKNOWN
>  	UNLISTEN UNLOGGED UNTIL UPDATE USER USING
>  
>  	VACUUM VALID VALIDATE VALIDATOR VALUE_P VALUES VARCHAR VARIADIC VARYING
> @@ -15768,6 +15775,60 @@ func_expr_common_subexpr:
>  					n->location = @1;
>  					$$ = (Node *) n;
>  				}
> +			| JSON_QUERY '('
> +				json_value_expr ',' a_expr json_passing_clause_opt
> +				json_returning_clause_opt
> +				json_wrapper_behavior
> +				json_quotes_clause_opt
> +				json_behavior_clause_opt
> +			')'
> +				{
> +					JsonFuncExpr *n = makeNode(JsonFuncExpr);
> +
> +					n->op = JSON_QUERY_OP;
> +					n->context_item = (JsonValueExpr *) $3;
> +					n->pathspec = $5;
> +					n->passing = $6;
> +					n->output = (JsonOutput *) $7;
> +					n->wrapper = $8;
> +					n->quotes = $9;
> +					n->behavior = $10;
> +					n->location = @1;
> +					$$ = (Node *) n;
> +				}
> +			| JSON_EXISTS '('
> +				json_value_expr ',' a_expr json_passing_clause_opt
> +				json_behavior_clause_opt
> +			')'
> +				{
> +					JsonFuncExpr *n = makeNode(JsonFuncExpr);
> +
> +					n->op = JSON_EXISTS_OP;
> +					n->context_item = (JsonValueExpr *) $3;
> +					n->pathspec = $5;
> +					n->passing = $6;
> +					n->output = NULL;
> +					n->behavior = $7;
> +					n->location = @1;
> +					$$ = (Node *) n;
> +				}
> +			| JSON_VALUE '('
> +				json_value_expr ',' a_expr json_passing_clause_opt
> +				json_returning_clause_opt
> +				json_behavior_clause_opt
> +			')'
> +				{
> +					JsonFuncExpr *n = makeNode(JsonFuncExpr);
> +
> +					n->op = JSON_VALUE_OP;
> +					n->context_item = (JsonValueExpr *) $3;
> +					n->pathspec = $5;
> +					n->passing = $6;
> +					n->output = (JsonOutput *) $7;
> +					n->behavior = $8;
> +					n->location = @1;
> +					$$ = (Node *) n;
> +				}
>  			;
>  
>  
> @@ -16494,6 +16555,27 @@ opt_asymmetric: ASYMMETRIC
>  		;
>  
>  /* SQL/JSON support */
> +json_passing_clause_opt:
> +			PASSING json_arguments					{ $$ = $2; }
> +			| /*EMPTY*/								{ $$ = NIL; }
> +		;
> +
> +json_arguments:
> +			json_argument							{ $$ = list_make1($1); }
> +			| json_arguments ',' json_argument		{ $$ = lappend($1, $3); }
> +		;
> +
> +json_argument:
> +			json_value_expr AS ColLabel
> +			{
> +				JsonArgument *n = makeNode(JsonArgument);
> +
> +				n->val = (JsonValueExpr *) $1;
> +				n->name = $3;
> +				$$ = (Node *) n;
> +			}
> +		;
> +
>  json_value_expr:
>  			a_expr json_format_clause_opt
>  			{
> @@ -16519,6 +16601,27 @@ json_encoding_clause_opt:
>  			| /* EMPTY */					{ $$ = JS_ENC_DEFAULT; }
>  		;
>  
> +/* ARRAY is a noise word */
> +json_wrapper_behavior:
> +			  WITHOUT WRAPPER					{ $$ = JSW_NONE; }
> +			| WITHOUT ARRAY	WRAPPER				{ $$ = JSW_NONE; }
> +			| WITH WRAPPER						{ $$ = JSW_UNCONDITIONAL; }
> +			| WITH ARRAY WRAPPER				{ $$ = JSW_UNCONDITIONAL; }
> +			| WITH CONDITIONAL ARRAY WRAPPER	{ $$ = JSW_CONDITIONAL; }
> +			| WITH UNCONDITIONAL ARRAY WRAPPER	{ $$ = JSW_UNCONDITIONAL; }
> +			| WITH CONDITIONAL WRAPPER			{ $$ = JSW_CONDITIONAL; }
> +			| WITH UNCONDITIONAL WRAPPER		{ $$ = JSW_UNCONDITIONAL; }
> +			| /* empty */						{ $$ = JSW_NONE; }
> +		;
> +
> +json_quotes_clause_opt:
> +			KEEP QUOTES ON SCALAR STRING_P		{ $$ = JS_QUOTES_KEEP; }
> +			| KEEP QUOTES						{ $$ = JS_QUOTES_KEEP; }
> +			| OMIT QUOTES ON SCALAR STRING_P	{ $$ = JS_QUOTES_OMIT; }
> +			| OMIT QUOTES						{ $$ = JS_QUOTES_OMIT; }
> +			| /* EMPTY */						{ $$ = JS_QUOTES_UNSPEC; }
> +		;
> +
>  json_returning_clause_opt:
>  			RETURNING Typename json_format_clause_opt
>  				{
> @@ -16532,6 +16635,39 @@ json_returning_clause_opt:
>  			| /* EMPTY */							{ $$ = NULL; }
>  		;
>  
> +json_behavior:
> +			DEFAULT a_expr
> +				{ $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_DEFAULT, $2, NULL, @1); }
> +			| ERROR_P
> +				{ $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_ERROR, NULL, NULL, @1); }
> +			| NULL_P
> +				{ $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_NULL, NULL, NULL, @1); }
> +			| TRUE_P
> +				{ $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_TRUE, NULL, NULL, @1); }
> +			| FALSE_P
> +				{ $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_FALSE, NULL, NULL, @1); }
> +			| UNKNOWN
> +				{ $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_UNKNOWN, NULL, NULL, @1); }
> +			| EMPTY_P ARRAY
> +				{ $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL, NULL, @1); }
> +			| EMPTY_P OBJECT_P
> +				{ $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_EMPTY_OBJECT, NULL, NULL, @1); }
> +			/* non-standard, for Oracle compatibility only */
> +			| EMPTY_P
> +				{ $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL, NULL, @1); }
> +		;

Seems like this would look better if you made json_behavior return just the
enum values and had one makeJsonBehavior() at the place referencing it?

> +json_behavior_clause_opt:
> +			json_behavior ON EMPTY_P
> +				{ $$ = list_make2($1, NULL); }
> +			| json_behavior ON ERROR_P
> +				{ $$ = list_make2(NULL, $1); }
> +			| json_behavior ON EMPTY_P json_behavior ON ERROR_P
> +				{ $$ = list_make2($1, $4); }
> +			| /* EMPTY */
> +				{ $$ = list_make2(NULL, NULL); }
> +		;

This seems like an odd representation - why represent the behavior as a two
element list where one needs to know what is stored at which list offset?

Greetings,

Andres Freund

---

From: Amit Langote <[email-masked]>
Date: Thu, 14 Dec 2023 17:04:46 +0900

Thanks for the review.

> Why do we need this both in ExprState *and* in ExprEvalStep?

In the current design, ExprState.escontext is only set when
initializing sub-expressions that should have their errors handled
softly and is supposed to be NULL at the runtime.   So, the design
expects the expressions to save the ErrorSaveContext pointer into
their struct in ExecEvalStep or somewhere else (input function's
FunctionCallInfo in CoerceViaIO's case).

> if there's something more general that could be improved to make this less

Hmm, I can't think of anything short of a rewrite of the code under
populate_record_field() so that any error-producing code is well
isolated or adding a variant/wrapper with soft-error handling
capabilities.  I'll give this some more thought, though I'm happy to
hear ideas.

> I'd not at all be surprised if this caused a measurable slowdown.   Patches 0004, 0005, and 0006 are new.

I don't notice a significant slowdown.  The benchmark I used is the
time to run the following query:

select json_populate_record(row(1,1), '{"f1":1, "f2":1}') from
generate_series(1, 1000000)

Here are the times:

Unpatched:
Time: 1262.011 ms (00:01.262)
Time: 1202.354 ms (00:01.202)
Time: 1187.708 ms (00:01.188)
Time: 1171.752 ms (00:01.172)
Time: 1174.249 ms (00:01.174)

Patched:
Time: 1233.927 ms (00:01.234)
Time: 1185.381 ms (00:01.185)
Time: 1202.245 ms (00:01.202)
Time: 1164.994 ms (00:01.165)
Time: 1179.009 ms (00:01.179)

perf shows that a significant amount of time is spent is json_lex()
dwarfing the time spent in dispatching code that is being changed
here.

> and add more code expecting to not reach the later code.

Having returns in the code blocks containing errsave() sounds prudent, so done.

> I don't think "erratic" is the right word, "erroneous" maybe?

"erroneous" sounds better.

> I think it'd be worth trying to break this into smaller bits - it's not easy

ISTM that the only piece that can be broken out at this point is the
additions under src/backend/utils/adt.  I'm not entirely sure if it'd
be a good idea to commit the various bits on their own, that is,
without tests which cannot be added without the rest of the
parser/executor additions for JsonFuncExpr, JsonExpr, and the
supporting child nodes.

I've extracted those bits as separate patches even if only for the
ease of review.

> somewhere more json specific?

I've gotten rid of JsonCoercionState, moving the fields directly into
ExprEvalStep.d.jsonexpr_coercion.

Regarding JsonExprState and JsonExprPostEvalState, maybe they're
better put in execnodes.h to be near other expression state nodes like
WindowFuncExprState, so have moved them there.  I'm not sure of a
json-specific place for this.  All of the information contained in
those structs is populated and used by execInterpExpr.c, so
execnodes.h seems appropriate to me.

> Why do we need manually assigned values here?

Not really necessary here.  I think I simply copied the style from
some other json-related enum where assigning values seems necessary.

> What's the difference between an "ItemCoercion" and a "Coercion"?

ItemCoercion is used to store the coercion expression used at runtime
to convert the value of given JsonItemType to the target type
specified in the JsonExpr.returning.  It can either be a cast
expression node found by the parser or a JsonCoercion node.

I'll update the comments.

> My editor warns about space-before-tab here.

Fixed.

> These comments seem even worse.

OK, I've rewritten the comments about JsonBehavior and JsonExpr.

> here, but then use it later, in ExecInitJsonExpr().

I had moved this code out of ExecInitJsonExprCoercion() into
ExecInitExprRec() to make the JsonCoercion node look like a first
class citizen of execExpr.c, but maybe that's not such a good idea
after all.  I've moved it back to make it just another implementation
detail of JsonExpr.

> return the target jump step as a return value from ExecEvalJsonExprPath or

OK, done that way.

> no danger of ambiguity without, no?

Yes, this looks like a remnant of an old version of this condition.

> No need for the parens around ereport() arguments anymore. Same in a few other places.

All fixed.

> enum values and had one makeJsonBehavior() at the place referencing it?

Yes, changed like that.

> element list where one needs to know what is stored at which list offset?

A previous version had a JsonBehaviorClause containing 2 JsonBehavior
nodes, but Peter didn't like it, so we have this.  Like Peter, I
prefer to use the List instead of a whole new parser node, but maybe
the damage would be less if we make the List be local to gram.y.  I've
done that by adding two JsonBehavior nodes to JsonFuncExpr itself
which are assigned appropriate values from the List in gram.y itself.

Updated patches attached.

---

From: Amit Langote <[email-masked]>
Date: Wed, 20 Dec 2023 17:36:25 +0900

> capabilities.  I'll give this some more thought, though I'm happy to

I looked at this and wasn't able to come up with alternative takes
that are better in terms of the verbosity/readability.  I'd still want
to hear if someone well-versed in the json(b) code has any advice.

I also looked at some commits touching src/backend/utils/adt/json*
files to add soft error handling and I can't help but notice that
those commits look not very different from this.  For example, commits
c60c9bad, 50428a30 contain changes like:

@@ -454,7 +474,11 @@ parse_array_element(JsonLexContext *lex,
JsonSemAction *sem)
        return result;

    if (aend != NULL)
-       (*aend) (sem->semstate, isnull);
+   {
+       result = (*aend) (sem->semstate, isnull);
+       if (result != JSON_SUCCESS)
+           return result;
+   }

Attached updated patches addressing jian he's comments, some minor
fixes, and commit message updates.

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 18 Jan 2024 12:46:24 +0100

I've been eyeballing the coverage report generated after applying all
patches (but I only checked the code added by the 0008 patch).  AFAICS
the coverage is pretty good.  Some uncovered paths:

commands/explain.c (Hmm, I think this is a preexisting bug actually)

    3893          18 :         case T_TableFuncScan:
    3894          18 :             Assert(rte->rtekind == RTE_TABLEFUNC);
    3895          18 :             if (rte->tablefunc)
    3896           0 :                 if (rte->tablefunc->functype == TFT_XMLTABLE)
    3897           0 :                     objectname = "xmltable";
    3898             :                 else            /* Must be TFT_JSON_TABLE */
    3899           0 :                     objectname = "json_table";
    3900             :             else
    3901          18 :                 objectname = NULL;
    3902          18 :             objecttag = "Table Function Name";
    3903          18 :             break;

parser/gram.y:

   16940             : json_table_plan_cross:
   16941             :             json_table_plan_primary CROSS json_table_plan_primary
   16942          39 :                 { $$ = makeJsonTableJoinedPlan(JSTPJ_CROSS, $1, $3, @1); }
   16943             :             | json_table_plan_cross CROSS json_table_plan_primary
   16944           0 :                 { $$ = makeJsonTableJoinedPlan(JSTPJ_CROSS, $1, $3, @1); }
Not really sure how critical this one is TBH.

utils/adt/jsonpath_exec.c:

    3492             : /* Recursively reset scan and its child nodes */
    3493             : static void
    3494         120 : JsonTableRescanRecursive(JsonTablePlanState * state)
    3495             : {
    3496         120 :     if (state->type == JSON_TABLE_JOIN_STATE)
    3497             :     {
    3498           0 :         JsonTableJoinState *join = (JsonTableJoinState *) state;
    3499             : 
    3500           0 :         JsonTableRescanRecursive(join->left);
    3501           0 :         JsonTableRescanRecursive(join->right);
    3502           0 :         join->advanceRight = false;
    3503             :     }

I think this one had better be covered.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"The saddest aspect of life right now is that science gathers knowledge faster
 than society gathers wisdom."  (Isaac Asimov)

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 18 Jan 2024 18:11:18 +0100

>     3902          18 :             objecttag = "Table Function Name";

Indeed -- the problem seems to be that add_rte_to_flat_rtable is
creating a new RTE and zaps the ->tablefunc pointer for it.  So when
EXPLAIN goes to examine the struct, there's a NULL pointer there and
nothing is printed.

One simple fix is to change add_rte_to_flat_rtable so that it doesn't
zero out the tablefunc pointer, but this is straight against what that
function is trying to do, namely to remove substructure.  Which means
that we need to preserve the name somewhere else.  I added a new member
to RangeTblEntry for this, which perhaps is a little ugly.  So here's
the patch for that.  (I also added an alias to one XMLTABLE invocation
under EXPLAIN, to show what it looks like when an alias is specified.
Otherwise they're always shown as "XMLTABLE" "xmltable" which is a bit
dumb).

Another possible way out is to decide that we don't want the
"objectname" to be reported here.  I admit it's perhaps redundant.  In
this case we'd just remove lines 3896-3899 shown above and let it be
NULL.

Thoughts?

---

From: Amit Langote <[email-masked]>
Date: Fri, 19 Jan 2024 12:09:49 +0900

> EXPLAIN goes to examine the struct, there's a NULL pointer there and

Ah yes.

> function is trying to do, namely to remove substructure.

Yes.

>  Which means
> the patch for that.
>  (I also added an alias to one XMLTABLE invocation

Thanks for the patch.  Seems alright to me.

> this case we'd just remove lines 3896-3899 shown above and let it be

Showing the function's name spelled out in the query (XMLTABLE /
JSON_TABLE) seems fine to me, even though maybe a bit redundant, yes.

---

From: jian he <[email-masked]>
Date: Fri, 19 Jan 2024 18:46:09 +0800

play with domain types.
in ExecEvalJsonCoercion, seems func json_populate_type cannot cope
with domain type.

tests:
drop domain test;
create domain test as int[] check ( array_length(value,1) =2 and
(value[1] = 1 or value[2] = 2));
SELECT * from JSON_QUERY(jsonb'{"rec": "{1,2,3}"}', '$.rec' returning
test omit quotes);
SELECT * from JSON_QUERY(jsonb'{"rec": "{1,11}"}', '$.rec' returning
test keep quotes);
SELECT * from JSON_QUERY(jsonb'{"rec": "{2,11}"}', '$.rec' returning
test omit quotes error on error);
SELECT * from JSON_QUERY(jsonb'{"rec": "{2,2}"}', '$.rec' returning
test keep quotes error on error);

SELECT * from JSON_QUERY(jsonb'{"rec": [1,2,3]}', '$.rec' returning
test omit quotes );
SELECT * from JSON_QUERY(jsonb'{"rec": [1,2,3]}', '$.rec' returning
test omit quotes null on error);
SELECT * from JSON_QUERY(jsonb'{"rec": [1,2,3]}', '$.rec' returning
test null on error);
SELECT * from JSON_QUERY(jsonb'{"rec": [1,11]}', '$.rec' returning
test omit quotes);
SELECT * from JSON_QUERY(jsonb'{"rec": [2,2]}', '$.rec' returning test
omit quotes);

Many domain related tests seem not right.
like the following, i think it should just return null.
+SELECT JSON_QUERY(jsonb '{"a": 1}', '$.b' RETURNING sqljsonb_int_not_null);
+ERROR:  domain sqljsonb_int_not_null does not allow null values

--another example
SELECT JSON_QUERY(jsonb '{"a": 1}', '$.b' RETURNING
sqljsonb_int_not_null null on error);

Maybe in node JsonCoercion, we don't need both via_io and
via_populate, but we can have one bool to indicate either call
InputFunctionCallSafe or json_populate_type in ExecEvalJsonCoercion.

---

From: jian he <[email-masked]>
Date: Mon, 22 Jan 2024 08:00:00 +0800

based on v35.
Now I only applied from 0001 to 0007.
For {DEFAULT expression  ON EMPTY}  | {DEFAULT expression ON ERROR}
restrict DEFAULT expression be either Const node or FuncExpr node.
so these 3 SQL/JSON functions can be used in the btree expression index.

I made some big changes on the doc. (see attachment)
list (json_query, json_exists, json_value) as a new <section2> may be
a good idea.

follow these two links, we can see the difference.
only apply v35, 0001 to 0007: https://v35-functions-json-html.vercel.app
apply v35, 0001 to 0007 plus my changes:
https://html-starter-seven-pied.vercel.app

minor issues:
+        Note that if the <replaceable>path_expression</replaceable>
+        is <literal>strict</literal>, an error is generated if it yields no
+        items, provided the specified <literal>ON ERROR</literal> behavior is
+        <literal>ERROR</literal>.

how about something like this:
+        Note that if the <replaceable>path_expression</replaceable>
+        is <literal>strict</literal> and <literal>ON ERROR</literal>
behavior is specified
+        <literal>ERROR</literal>, an error is generated if it yields no
+        items

+  <note>
+   <para>
+    SQL/JSON path expression can currently only accept values of the
+    <type>jsonb</type> type, so it might be necessary to cast the
+    <replaceable>context_item</replaceable> argument of these functions to
+    <type>jsonb</type>.
+   </para>
+  </note>
here should it be "SQL/JSON query functions"?

---

From: Peter Smith <[email-masked]>
Date: Mon, 22 Jan 2024 16:55:07 +1100

2024-01 Commitfest.

Hi, This patch has a CF status of "Needs Review" [1], but it seems
there were CFbot test failures last time it was run [2]. Please have a
look and post an updated version if necessary.

======
[1] https://commitfest.postgresql.org/46/4377/
[2] https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/4377

Kind Regards,
Peter Smith.

---

From: jian he <[email-masked]>
Date: Mon, 22 Jan 2024 14:14:07 +0800

I found two main issues regarding cocece SQL/JSON function output to
other data types.
* returning typmod influence the returning result of JSON_VALUE | JSON_QUERY.
* JSON_VALUE | JSON_QUERY handles returning type domains allowing null
and not allowing null inconsistencies.

in ExecInitJsonExprCoercion, there is IsA(coercion,JsonCoercion) or
not difference.
for the returning of (JSON_VALUE | JSON_QUERY),
"coercion" is a JsonCoercion or not is set in coerceJsonFuncExprOutput.

this influence returning type with typmod is not -1.
if set "coercion" as JsonCoercion Node then it may call the
InputFunctionCallSafe to do the coercion.
If not, it may call ExecInitFunc related code which is wrapped in
ExecEvalCoerceViaIOSafe.

for example:
SELECT JSON_QUERY(jsonb '[1,2]', '$' RETURNING char(3));
will ExecInitFunc, will init function bpchar(character, integer,
boolean). it will set the third argument to true.
so it will initiate related instructions like: `select
bpchar('[,2]',7,true); ` which in the end will make the result be
`[,2`
However, InputFunctionCallSafe cannot handle that.
simple demo:
create table t(a char(3));
--fail.
INSERT INTO t values ('test');
--ok
select 'test'::char(3);

however current ExecEvalCoerceViaIOSafe cannot handle omit quotes.

even if I made the changes, still not bullet-proof.
for example:
create domain char3_domain_not_null as char(3) NOT NULL;
create domain hello as text NOT NULL check (value = 'hello');
create domain int42 as int check (value = 42);
CREATE TYPE comp_domain_with_typmod AS (a char3_domain_not_null, b int42);

SELECT JSON_VALUE(jsonb'{"rec": "(abcd,42)"}', '$.rec' returning
comp_domain_with_typmod);
will return NULL

however
SELECT JSON_VALUE(jsonb'{"rec": "abcd"}', '$.rec' returning
char3_domain_not_null);
will return `abc`.

I made the modification, you can see the difference.
attached is test_coerce.sql is the test file.
test_coerce_only_v35.out  is the test output of only applying v35 0001
to 0007 plus my previous changes[0].
test_coerce_v35_plus_change.out is  the test output of applying to v35
0001 to 0007 plus changes (attachment) and previous changes[0].

[0] https://www.postgresql.org/message-id/CACJufxHo1VVk_0th3AsFxqdMgjaUDz6s0F7%2Bj9rYA3d%3DURw97A%40mail.gmail.com

---

From: Amit Langote <[email-masked]>
Date: Mon, 22 Jan 2024 23:27:58 +0900

Hi,

> sqljsonb_int_not_null null on error);

Hmm, yes, I've thought the same thing, but the patch since it has
existed appears to have made an exception for the case when the
RETURNING type is a domain for some reason; I couldn't find any
mention of why in the old discussions.  I suspect it might be because
a domain's constraints should always be enforced, irrespective of what
the SQL/JSON's ON ERROR says.

Though, I'm inclined to classify the domain constraint failure errors
into the same class as any other error as far as the ON ERROR clause
is concerned, so have adjusted the code to do so.

Please check if the attached looks fine.

> InputFunctionCallSafe or json_populate_type in ExecEvalJsonCoercion.

I'm not sure if there's a way to set such a bool statically, because
the decision between calling input function or json_populate_type()
must be made at run-time based on whether the input jsonb datum is a
scalar or not.  That said, I think we should ideally be able to always
use json_populate_type(), but it can't handle OMIT QUOTES for scalars
and I haven't been able to refactor it to do so

> so these 3 SQL/JSON functions can be used in the btree expression index.

I'm not really excited about adding these restrictions into the
transformJsonFuncExpr() path.  Index or any other code that wants to
put restrictions already have those in place, no need to add them
here.  Moreover, by adding these restrictions, we might end up
preventing users from doing useful things with this like specify
column references.  If there are semantic issues with allowing that,
we should discuss them.

> https://html-starter-seven-pied.vercel.app

Thanks for your patch.  I've adapted some of your proposed changes.

> +        <literal>ERROR</literal>, an error is generated if it yields no

Sure.

> here should it be "SQL/JSON query functions"?

"path expressions" is not wrong but I agree that "query functions"
might be better, so changed.  I've also mentioned that the restriction
arises from the fact that SQL/JSON path langage expects the input
document to be passed as jsonb.

> [0] https://www.postgresql.org/message-id/CACJufxHo1VVk_0th3AsFxqdMgjaUDz6s0F7%2Bj9rYA3d%3DURw97A%40mail.gmail.com

I'll think about this tomorrow.

In the meantime, here are the updated/reorganized patches with the
following changes:

* I started having second thoughts about introducing
json_populate_type(), jspIsMutable, and JsonbUnquote() in commits
separate from the commit introducing the SQL/JSON query functions
patch where they are needed, so I moved them back into that patch.  So
there are 2 fewer patches -- 0005, 0006 squashed into 0007.

* Boke the test file jsonb_sqljson into 2 files named
sqljson_queryfuncs and sqljson_jsontable.  Also, the test files under
ECPG to sql_jsontable

* Some cosmetic improvements in the JSON_TABLE() patch

I'll push 0001-0004 tomorrow, barring objections.

---

From: jian he <[email-masked]>
Date: Mon, 22 Jan 2024 23:46:15 +0800

> column references.  If there are semantic issues with allowing that,

after applying v36.
The following index creation and query operation works. I am not 100%
sure about these cases.
just want confirmation, sorry for bothering you....

drop table t;
create table t(a jsonb, b  int);
insert into t select '{"hello":11}',1;
insert into t select '{"hello":12}',2;
CREATE INDEX t_idx2 ON t (JSON_query(a, '$.hello1' RETURNING int
default b + random() on error));
CREATE INDEX t_idx3 ON t (JSON_query(a, '$.hello1' RETURNING int
default random()::int on error));
SELECT JSON_query(a, '$.hello1'  RETURNING int default ret_setint() on
error) from t;
SELECT JSON_query(a, '$.hello1'  RETURNING int default sum(b) over()
on error) from t;
SELECT JSON_query(a, '$.hello1'  RETURNING int default sum(b) on
error) from t group by a;

but the following cases will fail related to index and default expression.
create table zz(a int, b int);
CREATE INDEX zz_idx1 ON zz ( (b + random()::int));
create table ssss(a int, b int default ret_setint());
create table ssss(a int, b int default sum(b) over());

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 22 Jan 2024 17:19:14 +0100

On 2024-Jan-18, Alvaro Herrera wrote:

> Indeed 

I was completely wrong about this, and in order to gain coverage the
only thing we needed was to add an EXPLAIN that uses the JSON format.
I did that just now.  I think your addition here works just fine.

---

From: jian he <[email-masked]>
Date: Tue, 23 Jan 2024 16:51:55 +0800

> SELECT JSON_query(a, '$.hello1'  RETURNING int default ret_setint() on

I forgot to attach ret_setint defition.

create or replace function ret_setint() returns setof integer as
$$
begin
    -- perform pg_sleep(0.1);
    return query execute 'select 1 union all select 1';
end;
$$
language plpgsql IMMUTABLE;

-----------------------------------------
In the function transformJsonExprCommon, we have
`JsonExpr   *jsexpr = makeNode(JsonExpr);`
then the following 2 assignments are not necessary.

/* Both set in the caller. */
jsexpr->result_coercion = NULL;
jsexpr->omit_quotes = false;

So I removed it.

JSON_VALUE OMIT QUOTES by default, so I set it accordingly.
I also changed coerceJsonFuncExprOutput accordingly

---

From: Amit Langote <[email-masked]>
Date: Tue, 23 Jan 2024 22:46:06 +0900

> I did that just now.  I think your addition here works just fine.

I think we'd still need your RangeTblFunc.tablefunc_name in order for
the new code (with JSON_TABLE) to be able to set objectname to either
"XMLTABLE" or "JSON_TABLE", no?

As you pointed out, rte->tablefunc is always NULL in
ExplainTargetRel() due to setrefs.c setting it to NULL, so the
JSON_TABLE additions to explain.c in my patch as they were won't work.
I've included your patch in the attached set and adjusted the
JSON_TABLE patch to set tablefunc_name in the parser.

I had intended to push 0001-0004 today, but held off to add a
SQL-callable testing function for the changes in 0002.  On that note,
I'm now not so sure about committing jsonpath_exec.c functions
JsonPathExists/Query/Value() from their SQL/JSON counterparts, so
inclined to squash that one into the SQL/JSON query functions patch
from a testability standpoint.

I haven't looked at Jian He's comments yet.

---

From: Amit Langote <[email-masked]>
Date: Wed, 24 Jan 2024 22:11:54 +0900

> from a testability standpoint.

Pushed 0001-0003 for now.

Rebased patches attached.  I merged 0004 into the query functions
patch after all.

> I haven't looked at Jian He's comments yet.

See below...

> just want confirmation, sorry for bothering you....

No worries; I really appreciate your testing and suggestions.

> create table ssss(a int, b int default sum(b) over());

I think your suggestion to add restrictions on what is allowed for
DEFAULT makes sense.  Also, immutability shouldn't be checked in
transformJsonBehavior(), but in contain_mutable_functions() as done in
the attached.  Added some tests too.

I still need to take a look at your other report regarding typmod but
I'm out of energy today.

---

From: Amit Langote <[email-masked]>
Date: Thu, 25 Jan 2024 18:09:42 +0900

> I still need to take a look at your other report regarding typmod but

The attached updated patch should address one of the concerns --
JSON_QUERY() should now work appropriately with RETURNING type with
typmod whether or  OMIT QUOTES is specified.

But I wasn't able to address the problems with RETURNING
record_type_with_typmod, that is, the following example you shared
upthread:

create domain char3_domain_not_null as char(3) NOT NULL;
create domain hello as text not null check (value = 'hello');
create domain int42 as int check (value = 42);
create type comp_domain_with_typmod AS (a char3_domain_not_null, b int42);
select json_value(jsonb'{"rec": "(abcd,42)"}', '$.rec' returning
comp_domain_with_typmod);
 json_value
------------

(1 row)

select json_value(jsonb'{"rec": "(abcd,42)"}', '$.rec' returning
comp_domain_with_typmod error on error);
ERROR:  value too long for type character(3)

select json_value(jsonb'{"rec": "abcd"}', '$.rec' returning
char3_domain_not_null error on error);
 json_value
------------
 abc
(1 row)

The problem with returning comp_domain_with_typmod from json_value()
seems to be that it's using a text-to-record CoerceViaIO expression
picked from JsonExpr.item_coercions, which behaves differently than
the expression tree that the following uses:

select ('abcd', 42)::comp_domain_with_typmod;
   row
----------
 (abc,42)
(1 row)

I don't see a good way to make RETURNING record_type_with_typmod to
work cleanly, so I am inclined to either simply disallow the feature
or live with the limitation.

---

From: jian he <[email-masked]>
Date: Thu, 25 Jan 2024 17:20:00 +0800

On 9.16.4. JSON_TABLE
`
name type FORMAT JSON [ENCODING UTF8] [ PATH json_path_specification ]
Inserts a composite SQL/JSON item into the output row
`
i am not sure "Inserts a composite SQL/JSON item into the output row"
I think it means, for any type's typecategory is TYPCATEGORY_STRING,
if FORMAT JSON is specified explicitly, the output value (text type)
will be legal
json type representation.

I also did a minor refactor on JSON_VALUE_OP, jsexpr->omit_quotes.

---

From: Amit Langote <[email-masked]>
Date: Thu, 25 Jan 2024 20:54:25 +0900

> comp_domain_with_typmod);

Oh, it hadn't occurred to me to check what trying to coerce a "string"
containing the record literal would do:

select '(''abcd'', 42)'::comp_domain_with_typmod;
ERROR:  value too long for type character(3)
LINE 1: select '(''abcd'', 42)'::comp_domain_with_typmod;

which is the same thing as what the JSON_QUERY() and JSON_VALUE() are
running into.  So, it might be fair to think that the error is not a
limitation of the SQL/JSON patch but an underlying behavior that it
has to accept as is.

---

From: jian he <[email-masked]>
Date: Thu, 25 Jan 2024 22:39:30 +0800

> limitation of the SQL/JSON patch but an underlying behavior that it

Hi, I reconciled with these cases.
What bugs me now is the first query of the following 4 cases (for comparison).
SELECT JSON_QUERY(jsonb '[1,2]', '$' RETURNING char(3) omit quotes);
SELECT JSON_QUERY(jsonb '[1,2]', '$' RETURNING char(3) keep quotes);
SELECT JSON_QUERY(jsonb '[1,2]', '$' RETURNING text omit quotes);
SELECT JSON_QUERY(jsonb '[1,2]', '$' RETURNING text keep quotes);

I did some minor refactoring on the function coerceJsonFuncExprOutput.
it will make the following queries return null instead of error. NULL
is the return of json_value.

    SELECT JSON_QUERY(jsonb '"123"', '$' RETURNING int2);
    SELECT JSON_QUERY(jsonb '"123"', '$' RETURNING int4);
    SELECT JSON_QUERY(jsonb '"123"', '$' RETURNING int8);
    SELECT JSON_QUERY(jsonb '"123"', '$' RETURNING bool);
    SELECT JSON_QUERY(jsonb '"123"', '$' RETURNING numeric);
    SELECT JSON_QUERY(jsonb '"123"', '$' RETURNING real);
    SELECT JSON_QUERY(jsonb '"123"', '$' RETURNING float8);

---

From: jian he <[email-masked]>
Date: Wed, 31 Jan 2024 22:51:52 +0800

Hi.
minor issues.
I am wondering do we need add `pg_node_attr(query_jumble_ignore)`
to some of our created structs in src/include/nodes/parsenodes.h in
v39-0001-Add-SQL-JSON-query-functions.patch

diff --git a/src/backend/parser/parse_jsontable.c
b/src/backend/parser/parse_jsontable.c
new file mode 100644
index 0000000000..25b8204dc6
--- /dev/null
+++ b/src/backend/parser/parse_jsontable.c
@@ -0,0 +1,718 @@
+/*-------------------------------------------------------------------------
+ *
+ * parse_jsontable.c
+ *  parsing of JSON_TABLE
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ *  src/backend/parser/parse_jsontable.c
+ *
+ *-------------------------------------------------------------------------
+ */
2022 should change to 2024.