SQL/JSON revisited

On Wed, Dec 28, 2022 at 4:28 PM Amit Langote <amitlangote09@gmail.com> wrote:

Hi,

Rebased the SQL/JSON patches over the latest HEAD. I've decided to
keep the same division of code into individual commits as that
mentioned in the revert commit 2f2b18bd3f, squashing fixup commits in
that list into the appropriate feature commits.

The main difference from the patches as they were committed into v15
is that JsonExpr evaluation no longer needs to use sub-transactions,
thanks to the work done recently to handle type errors softly. I've
made the new code pass an ErrorSaveContext into the type-conversion
related functions as needed and also added an ExecEvalExprSafe() to
evaluate sub-expressions of JsonExpr that might contain expressions
that call type-conversion functions, such as CoerceViaIO contained in
JsonCoercion nodes. ExecExprEvalSafe() is based on one of the patches
that Nikita Glukhov had submitted in a previous discussion about
redesigning SQL/JSON expression evaluation [1]. Though, I think that
new interface will become unnecessary after I have finished rebasing
my patches to remove subsidiary ExprStates of JsonExprState that we
had also discussed back in [2].

Adding this to January CF.

Done.

https://commitfest.postgresql.org/41/4086/

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

Hi,

The Postgres Pro documentation team prepared another SQL/JSON
documentation patch (attached), to apply on top of
v1-0009-Documentation-for-SQL-JSON-features.patch.
The new patch:
- Fixes minor typos
- Does some rewording agreed with Nikita Glukhov
- Updates Docbook markup to make tags consistent across SQL/JSON
documentation and across func.sgml, and in particular, consistent with
the XMLTABLE function, which resembles SQL/JSON functions pretty much.

--
Elena Indrupskaya
Lead Technical Writer
Postgres Professional http://www.postgrespro.com

On 2023-01-10 Tu 07:51, Elena Indrupskaya wrote:

Hi,

The Postgres Pro documentation team prepared another SQL/JSON
documentation patch (attached), to apply on top of
v1-0009-Documentation-for-SQL-JSON-features.patch.
The new patch:
- Fixes minor typos
- Does some rewording agreed with Nikita Glukhov
- Updates Docbook markup to make tags consistent across SQL/JSON
documentation and across func.sgml, and in particular, consistent with
the XMLTABLE function, which resembles SQL/JSON functions pretty much.

That's nice, but please don't post incremental patches like this. It
upsets the cfbot. (I wish there were a way to tell the cfbot to ignore
patches)

Also, I'm fairly certain that a good many of your changes are not
according to project style. The rule as I understand it is that
<parameter> is used for things that are parameters and <replaceable> is
only used for things that are not parameters. (I'm not sure where that's
documented other than the comment on commit 47046763c3, but it's what I
attempted to do with the earlier doc tidy up.)

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Tags in the patch follow the markup of the XMLTABLE function:

<function>XMLTABLE</function> (
    <optional> <literal>XMLNAMESPACES</literal> (
<replaceable>namespace_uri</replaceable> <literal>AS</literal>
<replaceable>namespace_name</replaceable> <optional>, ...</optional> ),
</optional>
    <replaceable>row_expression</replaceable>
<literal>PASSING</literal> <optional><literal>BY</literal>
{<literal>REF</literal>|<literal>VALUE</literal>}</optional>
<replaceable>document_expression</replaceable>
<optional><literal>BY</literal>
{<literal>REF</literal>|<literal>VALUE</literal>}</optional>
    <literal>COLUMNS</literal> <replaceable>name</replaceable> {
<replaceable>type</replaceable> <optional><literal>PATH</literal>
<replaceable>column_expression</replaceable></optional>
<optional><literal>DEFAULT</literal>
<replaceable>default_expression</replaceable></optional>
<optional><literal>NOT NULL</literal> | <literal>NULL</literal></optional>
                  | <literal>FOR ORDINALITY</literal> }
            <optional>, ...</optional>
) <returnvalue>setof record</returnvalue>

In the above, as well as in the signatures of SQL/JSON functions, there
are no exact parameter names; otherwise, they should have been followed
by the <type> tag, which is not the case. There are no parameter names
in the functions' code either. Therefore, <replaceable> tags seem more
appropriate, according to the comment to commit 47046763c3.

Sorry for upsetting your bot. :(

--
Elena Indrupskaya
Lead Technical Writer
Postgres Professional http://www.postgrespro.com

On Wed, Jan 11, 2023 at 2:02 PM Elena Indrupskaya <
e.indrupskaya@postgrespro.ru> wrote:

Sorry for upsetting your bot. :(

What I do in these cases is save the incremental patch as a .txt file --
that way people can read it, but the cf bot doesn't try to launch a CI run.
And if I forget that detail, well it's not a big deal, it happens sometimes.

--
John Naylor
EDB: http://www.enterprisedb.com

On Wed, Dec 28, 2022 at 4:28 PM Amit Langote <amitlangote09@gmail.com> wrote:

Hi,

Rebased the SQL/JSON patches over the latest HEAD. I've decided to
keep the same division of code into individual commits as that
mentioned in the revert commit 2f2b18bd3f, squashing fixup commits in
that list into the appropriate feature commits.

The main difference from the patches as they were committed into v15
is that JsonExpr evaluation no longer needs to use sub-transactions,
thanks to the work done recently to handle type errors softly. I've
made the new code pass an ErrorSaveContext into the type-conversion
related functions as needed and also added an ExecEvalExprSafe() to
evaluate sub-expressions of JsonExpr that might contain expressions
that call type-conversion functions, such as CoerceViaIO contained in
JsonCoercion nodes. ExecExprEvalSafe() is based on one of the patches
that Nikita Glukhov had submitted in a previous discussion about
redesigning SQL/JSON expression evaluation [1]. Though, I think that
new interface will become unnecessary after I have finished rebasing
my patches to remove subsidiary ExprStates of JsonExprState that we
had also discussed back in [2].

And I've just finished doing that. In the attached updated 0004,
which adds the JsonExpr node, its evaluation code is now broken into
ExprEvalSteps to handle the subsidiary JsonCoercion and JsonBehavior
expression nodes that previously used ExprState for recursive
evaluation. Andres didn't like the latter as previously discussed at
[1]: /messages/by-id/20220616233130.rparivafipt6doj3@alap3.anarazel.de

I've also attached the patch that Elena has proposed as the patch
0011. I haven't managed to review it yet, though once I do, I'll
merge it into the main documentation patch 0009. Thanks Elena.

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

[1]: /messages/by-id/20220616233130.rparivafipt6doj3@alap3.anarazel.de

On Tue, 17 Jan 2023 at 19:01, Amit Langote <amitlangote09@gmail.com> wrote:

On Wed, Dec 28, 2022 at 4:28 PM Amit Langote <amitlangote09@gmail.com> wrote:

Hi,

Rebased the SQL/JSON patches over the latest HEAD. I've decided to
keep the same division of code into individual commits as that
mentioned in the revert commit 2f2b18bd3f, squashing fixup commits in
that list into the appropriate feature commits.

The main difference from the patches as they were committed into v15
is that JsonExpr evaluation no longer needs to use sub-transactions,
thanks to the work done recently to handle type errors softly. I've
made the new code pass an ErrorSaveContext into the type-conversion
related functions as needed and also added an ExecEvalExprSafe() to
evaluate sub-expressions of JsonExpr that might contain expressions
that call type-conversion functions, such as CoerceViaIO contained in
JsonCoercion nodes. ExecExprEvalSafe() is based on one of the patches
that Nikita Glukhov had submitted in a previous discussion about
redesigning SQL/JSON expression evaluation [1]. Though, I think that
new interface will become unnecessary after I have finished rebasing
my patches to remove subsidiary ExprStates of JsonExprState that we
had also discussed back in [2].

And I've just finished doing that. In the attached updated 0004,
which adds the JsonExpr node, its evaluation code is now broken into
ExprEvalSteps to handle the subsidiary JsonCoercion and JsonBehavior
expression nodes that previously used ExprState for recursive
evaluation. Andres didn't like the latter as previously discussed at
[1].

I've also attached the patch that Elena has proposed as the patch
0011. I haven't managed to review it yet, though once I do, I'll
merge it into the main documentation patch 0009. Thanks Elena.

The patch does not apply on top of HEAD as in [1]http://cfbot.cputube.org/patch_41_4086.log, please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
37e267335068059ac9bd4ec5d06b493afb4b73e8 ===
=== applying patch ./v2-0001-Common-SQL-JSON-clauses.patch
....
can't find file to patch at input line 717
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/src/backend/utils/misc/queryjumble.c
b/src/backend/utils/misc/queryjumble.c
|index 328995a7dc..2361845a62 100644
|--- a/src/backend/utils/misc/queryjumble.c
|+++ b/src/backend/utils/misc/queryjumble.c
--------------------------
No file to patch. Skipping patch.
1 out of 1 hunk ignored

[1]: http://cfbot.cputube.org/patch_41_4086.log

Regards,
Vignesh

On Fri, Jan 27, 2023 at 11:27 PM vignesh C <vignesh21@gmail.com> wrote:

On Tue, 17 Jan 2023 at 19:01, Amit Langote <amitlangote09@gmail.com> wrote:

And I've just finished doing that. In the attached updated 0004,
which adds the JsonExpr node, its evaluation code is now broken into
ExprEvalSteps to handle the subsidiary JsonCoercion and JsonBehavior
expression nodes that previously used ExprState for recursive
evaluation. Andres didn't like the latter as previously discussed at
[1].

I've also attached the patch that Elena has proposed as the patch
0011. I haven't managed to review it yet, though once I do, I'll
merge it into the main documentation patch 0009. Thanks Elena.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:

Thanks for the heads up. Here's a rebased version.

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

Hi,

On Mon, Jan 30, 2023 at 3:39 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Jan 27, 2023 at 11:27 PM vignesh C <vignesh21@gmail.com> wrote:

On Tue, 17 Jan 2023 at 19:01, Amit Langote <amitlangote09@gmail.com> wrote:

And I've just finished doing that. In the attached updated 0004,
which adds the JsonExpr node, its evaluation code is now broken into
ExprEvalSteps to handle the subsidiary JsonCoercion and JsonBehavior
expression nodes that previously used ExprState for recursive
evaluation. Andres didn't like the latter as previously discussed at
[1].

I've also attached the patch that Elena has proposed as the patch
0011. I haven't managed to review it yet, though once I do, I'll
merge it into the main documentation patch 0009. Thanks Elena.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:

Thanks for the heads up. Here's a rebased version.

Rebased again over queryjumble overhaul.

I decided to squash what was "[PATCH v3 01/11] Common SQL/JSON
clauses" into "[PATCH v3 02/11] SQL/JSON constructors", because I
noticed "useless productions" warnings against its gram.y additions
when building just 0001.

I also looked at squashing "[PATCH v3 11/11] Proposed reworking of
SQL/JSON documentaion" into "[PATCH v3 09/11] Documentation for
SQL/JSON features", but didn't, again, because I am still not sure
which one of <parameter> and <replaceable> is correct for the SQL/JSON
function constructs. Maybe it's the latter looking at the markup for
some text on [1]https://www.postgresql.org/docs/15/functions-json.html, such as exists ( path_expression ) → boolean, but
Andrew sounded doubtful about that upthread.

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

[1]: https://www.postgresql.org/docs/15/functions-json.html

Hi Amit and Andrew,

Regarding not squashing [PATCH v3 11/11] Proposed reworking of
SQL/JSON documentaion, here is exactly what Tom Lane wrote in the comment to commit 47046763c3:

Use <parameter>
consistently for things that are in fact names of parameters (including
OUT parameters), reserving <replaceable> for things that aren't.

Following this, <parameter> tags should be replaced with <replaceable> because
the SQL/JSON functions' code does not explicitly specify those tagged variables
as function parameters. Doesn't it convince you to look at the patch again? Thank you.

Op 20-02-2023 om 08:35 schreef Amit Langote:

Rebased again over queryjumble overhaul.

Hi,

But the following statement is a problem. It does not crash but it goes
off, half-freezing the machine, and only comes back after fanatic
Ctrl-C'ing.

select json_query(jsonb '[3,4]', '$[*]' returning bigint[] empty object
on error);

Can you have a look?

Thanks,

Erik Rijkers

PS
Log doesn't really have anything interesting:

2023-02-20 14:57:06.073 CET 1336 LOG: server process (PID 1493) was
terminated by signal 9: Killed
2023-02-20 14:57:06.073 CET 1336 DETAIL: Failed process was running:
select json_query(jsonb '[3,4]', '$[*]' returning bigint[] empty object
on error);
2023-02-20 14:57:06.359 CET 1336 LOG: terminating any other active
server processes
2023-02-20 14:57:06.667 CET 1336 LOG: all server processes terminated;
reinitializing
2023-02-20 14:57:11.870 CET 1556 LOG: database system was interrupted;
last known up at 2023-02-20 14:44:43 CET

Hi,

On 2023-02-20 16:35:52 +0900, Amit Langote wrote:

Subject: [PATCH v4 03/10] SQL/JSON query functions
+/*
+ * Evaluate a JSON error/empty behavior result.
+ */
+static Datum
+ExecEvalJsonBehavior(JsonBehavior *behavior, bool *is_null)
+{
+	*is_null = false;
+
+	switch (behavior->btype)
+	{
+		case JSON_BEHAVIOR_EMPTY_ARRAY:
+			return JsonbPGetDatum(JsonbMakeEmptyArray());
+
+		case JSON_BEHAVIOR_EMPTY_OBJECT:
+			return JsonbPGetDatum(JsonbMakeEmptyObject());
+
+		case JSON_BEHAVIOR_TRUE:
+			return BoolGetDatum(true);
+
+		case JSON_BEHAVIOR_FALSE:
+			return BoolGetDatum(false);
+
+		case JSON_BEHAVIOR_NULL:
+		case JSON_BEHAVIOR_UNKNOWN:
+			*is_null = true;
+			return (Datum) 0;
+
+		case JSON_BEHAVIOR_DEFAULT:
+			/* Always handled in the caller. */
+			Assert(false);
+			return (Datum) 0;
+
+		default:
+			elog(ERROR, "unrecognized SQL/JSON behavior %d", behavior->btype);
+			return (Datum) 0;
+	}
+}

Does this actually need to be evaluated at expression eavluation time?
Couldn't we just emit the proper constants in execExpr.c?

+/* ----------------------------------------------------------------
+ *		ExecEvalJson
+ * ----------------------------------------------------------------
+ */
+void
+ExecEvalJson(ExprState *state, ExprEvalStep *op, ExprContext *econtext)

Pointless comment.

+{
+	JsonExprState *jsestate = op->d.jsonexpr.jsestate;
+	JsonExprPreEvalState *pre_eval = &jsestate->pre_eval;
+	JsonExprPostEvalState *post_eval = &jsestate->post_eval;
+	JsonExpr   *jexpr = jsestate->jsexpr;
+	Datum		item;
+	Datum		res = (Datum) 0;
+	JsonPath   *path;
+	bool		throwErrors = jexpr->on_error->btype == JSON_BEHAVIOR_ERROR;
+
+	*op->resnull = true;		/* until we get a result */
+	*op->resvalue = (Datum) 0;
+
+	item = pre_eval->formatted_expr.value;
+	path = DatumGetJsonPathP(pre_eval->pathspec.value);
+
+	/* Reset JsonExprPostEvalState for this evaluation. */
+	memset(post_eval, 0, sizeof(*post_eval));
+
+	res = ExecEvalJsonExpr(op, econtext, path, item, op->resnull,
+						   !throwErrors ? &post_eval->error : NULL);
+
+	*op->resvalue = res;
+}

I really don't like having both ExecEvalJson() and ExecEvalJsonExpr(). There's
really no way to know what which version does, just based on the name.

--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y

This stuff adds quite a bit of complexity to the parser. Do we realy need like
a dozen new rules here?

+json_behavior_empty_array:
+			EMPTY_P ARRAY	{ $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
+			/* non-standard, for Oracle compatibility only */
+			| EMPTY_P		{ $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
+		;

Do we really want to add random oracle compat crud here?

+/*
+ * Evaluate a JSON path variable caching computed value.
+ */
+int
+EvalJsonPathVar(void *cxt, char *varName, int varNameLen,
+				JsonbValue *val, JsonbValue *baseObject)

Missing static?

+{
+	JsonPathVariableEvalContext *var = NULL;
+	List	   *vars = cxt;
+	ListCell   *lc;
+	int			id = 1;
+
+	if (!varName)
+		return list_length(vars);
+
+	foreach(lc, vars)
+	{
+		var = lfirst(lc);
+
+		if (!strncmp(var->name, varName, varNameLen))
+			break;
+
+		var = NULL;
+		id++;
+	}
+
+	if (!var)
+		return -1;
+
+	/*
+	 * When belonging to a JsonExpr, path variables are computed with the
+	 * JsonExpr's ExprState (var->estate is NULL), so don't need to be computed
+	 * here.  In some other cases, such as when the path variables belonging
+	 * to a JsonTable instead, those variables must be evaluated on their own,
+	 * without the enclosing JsonExpr itself needing to be evaluated, so must
+	 * be handled here.
+	 */
+	if (var->estate && !var->evaluated)
+	{
+		Assert(var->econtext != NULL);
+		var->value = ExecEvalExpr(var->estate, var->econtext, &var->isnull);
+		var->evaluated = true;

Uh, so this continues to do recursive expression evaluation, as
ExecEvalJsonExpr()->JsonPathQuery()->executeJsonPath(EvalJsonPathVar)

I'm getting grumpy here. This is wrong, has been pointed out many times. The
only thing that changes is that the point of recursion is moved around.

+
+/*
+ * ExecEvalExprSafe
+ *
+ * Like ExecEvalExpr(), though this allows the caller to pass an
+ * ErrorSaveContext to declare its intenion to catch any errors that occur when
+ * executing the expression, such as when calling type input functions that may
+ * be present in it.
+ */
+static inline Datum
+ExecEvalExprSafe(ExprState *state,
+				 ExprContext *econtext,
+				 bool *isNull,
+				 Node *escontext,
+				 bool *error)

Afaict there's no caller of this?

+/*
+ * ExecInitExprWithCaseValue
+ *
+ * This is the same as ExecInitExpr, except the caller passes the Datum and
+ * bool pointers that it would like the ExprState.innermost_caseval
+ * and ExprState.innermost_casenull, respectively, to be set to.  That way,
+ * it can pass an input value to evaluate the expression via a CaseTestExpr.
+ */
+ExprState *
+ExecInitExprWithCaseValue(Expr *node, PlanState *parent,
+						  Datum *caseval, bool *casenull)
+{
+	ExprState  *state;
+	ExprEvalStep scratch = {0};
+
+	/* Special case: NULL expression produces a NULL ExprState pointer */
+	if (node == NULL)
+		return NULL;
+
+	/* Initialize ExprState with empty step list */
+	state = makeNode(ExprState);
+	state->expr = node;
+	state->parent = parent;
+	state->ext_params = NULL;
+	state->innermost_caseval = caseval;
+	state->innermost_casenull = casenull;
+
+	/* Insert EEOP_*_FETCHSOME steps as needed */
+	ExecInitExprSlots(state, (Node *) node);
+
+	/* Compile the expression proper */
+	ExecInitExprRec(node, state, &state->resvalue, &state->resnull);
+
+	/* Finally, append a DONE step */
+	scratch.opcode = EEOP_DONE;
+	ExprEvalPushStep(state, &scratch);
+
+	ExecReadyExpr(state);
+
+	return state;

+struct JsonTableJoinState
+{
+	union
+	{
+		struct
+		{
+			JsonTableJoinState *left;
+			JsonTableJoinState *right;
+			bool		advanceRight;
+		}			join;
+		JsonTableScanState scan;
+	}			u;
+	bool		is_join;
+};

A join state that unions the join member with a scan, and has a is_join field?

+/*
+ * JsonTableInitOpaque
+ *		Fill in TableFuncScanState->opaque for JsonTable processor
+ */
+static void
+JsonTableInitOpaque(TableFuncScanState *state, int natts)
+{
+	JsonTableContext *cxt;
+	PlanState  *ps = &state->ss.ps;
+	TableFuncScan *tfs = castNode(TableFuncScan, ps->plan);
+	TableFunc  *tf = tfs->tablefunc;
+	JsonExpr   *ci = castNode(JsonExpr, tf->docexpr);
+	JsonTableParent *root = castNode(JsonTableParent, tf->plan);
+	List	   *args = NIL;
+	ListCell   *lc;
+	int			i;
+
+	cxt = palloc0(sizeof(JsonTableContext));
+	cxt->magic = JSON_TABLE_CONTEXT_MAGIC;
+
+	if (ci->passing_values)
+	{
+		ListCell   *exprlc;
+		ListCell   *namelc;
+
+		forboth(exprlc, ci->passing_values,
+				namelc, ci->passing_names)
+		{
+			Expr	   *expr = (Expr *) lfirst(exprlc);
+			String	   *name = lfirst_node(String, namelc);
+			JsonPathVariableEvalContext *var = palloc(sizeof(*var));
+
+			var->name = pstrdup(name->sval);
+			var->typid = exprType((Node *) expr);
+			var->typmod = exprTypmod((Node *) expr);
+			var->estate = ExecInitExpr(expr, ps);
+			var->econtext = ps->ps_ExprContext;
+			var->mcxt = CurrentMemoryContext;
+			var->evaluated = false;
+			var->value = (Datum) 0;
+			var->isnull = true;
+
+			args = lappend(args, var);
+		}
+	}
+
+	cxt->colexprs = palloc(sizeof(*cxt->colexprs) *
+						   list_length(tf->colvalexprs));
+
+	JsonTableInitScanState(cxt, &cxt->root, root, NULL, args,
+						   CurrentMemoryContext);
+
+	i = 0;
+
+	foreach(lc, tf->colvalexprs)
+	{
+		Expr	   *expr = lfirst(lc);
+
+		cxt->colexprs[i].expr =
+			ExecInitExprWithCaseValue(expr, ps,
+									  &cxt->colexprs[i].scan->current,
+									  &cxt->colexprs[i].scan->currentIsNull);
+
+		i++;
+	}
+
+	state->opaque = cxt;
+}

Evaluating N expressions for a json table isn't a good approach, both memory
and CPU efficiency wise.

Why don't you just emit the proper expression directly, insted of the
CaseTestExpr stuff, that you then separately evaluate?

Greetings,

Andres Freund

On Mon, Feb 20, 2023 at 11:41 PM Erik Rijkers <er@xs4all.nl> wrote:

Op 20-02-2023 om 08:35 schreef Amit Langote:

Rebased again over queryjumble overhaul.

But the following statement is a problem. It does not crash but it goes
off, half-freezing the machine, and only comes back after fanatic
Ctrl-C'ing.

select json_query(jsonb '[3,4]', '$[*]' returning bigint[] empty object
on error);

Can you have a look?

Thanks for the test case. It caused ExecInterpExpr() to enter an
infinite loop, which I've fixed in the attached updated version. I've
also merged Elena's documentation changes; I can see that
<replaceable> is more correct.

Now looking at Andres' comments, though, posting a version containing
a fix for the above case so Erik may continue the testing in the
meantime.

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

On Tue, Feb 21, 2023 at 12:09 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Feb 20, 2023 at 11:41 PM Erik Rijkers <er@xs4all.nl> wrote:

Op 20-02-2023 om 08:35 schreef Amit Langote:

Rebased again over queryjumble overhaul.

But the following statement is a problem. It does not crash but it goes
off, half-freezing the machine, and only comes back after fanatic
Ctrl-C'ing.

select json_query(jsonb '[3,4]', '$[*]' returning bigint[] empty object
on error);

Can you have a look?

Thanks for the test case. It caused ExecInterpExpr() to enter an
infinite loop, which I've fixed in the attached updated version. I've
also merged Elena's documentation changes; I can see that
<replaceable> is more correct.

Oops, I hadn't actually done the latter. Will do when posting the next version.

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

Thanks a lot for taking a look at this and sorry about the delay in response.

On Tue, Feb 21, 2023 at 2:25 AM Andres Freund <andres@anarazel.de> wrote:

On 2023-02-20 16:35:52 +0900, Amit Langote wrote:

Subject: [PATCH v4 03/10] SQL/JSON query functions
+/*
+ * Evaluate a JSON error/empty behavior result.
+ */
+static Datum
+ExecEvalJsonBehavior(JsonBehavior *behavior, bool *is_null)
+{
+     *is_null = false;
+
+     switch (behavior->btype)
+     {
+             case JSON_BEHAVIOR_EMPTY_ARRAY:
+                     return JsonbPGetDatum(JsonbMakeEmptyArray());
+
+             case JSON_BEHAVIOR_EMPTY_OBJECT:
+                     return JsonbPGetDatum(JsonbMakeEmptyObject());
+
+             case JSON_BEHAVIOR_TRUE:
+                     return BoolGetDatum(true);
+
+             case JSON_BEHAVIOR_FALSE:
+                     return BoolGetDatum(false);
+
+             case JSON_BEHAVIOR_NULL:
+             case JSON_BEHAVIOR_UNKNOWN:
+                     *is_null = true;
+                     return (Datum) 0;
+
+             case JSON_BEHAVIOR_DEFAULT:
+                     /* Always handled in the caller. */
+                     Assert(false);
+                     return (Datum) 0;
+
+             default:
+                     elog(ERROR, "unrecognized SQL/JSON behavior %d", behavior->btype);
+                     return (Datum) 0;
+     }
+}

Does this actually need to be evaluated at expression eavluation time?
Couldn't we just emit the proper constants in execExpr.c?

Yes, done that way in the updated patch.

+/* ----------------------------------------------------------------
+ *           ExecEvalJson
+ * ----------------------------------------------------------------
+ */
+void
+ExecEvalJson(ExprState *state, ExprEvalStep *op, ExprContext *econtext)

Pointless comment.

Removed this function altogether in favor of merging the body with
ExecEvalJsonExpr(), which does have a more sensible comment

+{
+     JsonExprState *jsestate = op->d.jsonexpr.jsestate;
+     JsonExprPreEvalState *pre_eval = &jsestate->pre_eval;
+     JsonExprPostEvalState *post_eval = &jsestate->post_eval;
+     JsonExpr   *jexpr = jsestate->jsexpr;
+     Datum           item;
+     Datum           res = (Datum) 0;
+     JsonPath   *path;
+     bool            throwErrors = jexpr->on_error->btype == JSON_BEHAVIOR_ERROR;
+
+     *op->resnull = true;            /* until we get a result */
+     *op->resvalue = (Datum) 0;
+
+     item = pre_eval->formatted_expr.value;
+     path = DatumGetJsonPathP(pre_eval->pathspec.value);
+
+     /* Reset JsonExprPostEvalState for this evaluation. */
+     memset(post_eval, 0, sizeof(*post_eval));
+
+     res = ExecEvalJsonExpr(op, econtext, path, item, op->resnull,
+                                                !throwErrors ? &post_eval->error : NULL);
+
+     *op->resvalue = res;
+}

I really don't like having both ExecEvalJson() and ExecEvalJsonExpr(). There's
really no way to know what which version does, just based on the name.

Yes, having two functions is no longer necessary.

--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y

This stuff adds quite a bit of complexity to the parser. Do we realy need like
a dozen new rules here?

+json_behavior_empty_array:
+                     EMPTY_P ARRAY   { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
+                     /* non-standard, for Oracle compatibility only */
+                     | EMPTY_P               { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
+             ;

Do we really want to add random oracle compat crud here?

Hmm, sorry, but I haven't familiarized myself with the grammar side of
things as much as I perhaps should have, so I am not sure whether a
more simplified grammar would suffice for offering a
standard-compliant functionality.

Maybe we could take out the oracle-compatibility bit, but I'd
appreciate it if someone who has been involved with SQL/JSON from the
beginning can comment on the above 2 points.

+/*
+ * Evaluate a JSON path variable caching computed value.
+ */
+int
+EvalJsonPathVar(void *cxt, char *varName, int varNameLen,
+                             JsonbValue *val, JsonbValue *baseObject)

Missing static?

Fixed.

+{
+     JsonPathVariableEvalContext *var = NULL;
+     List       *vars = cxt;
+     ListCell   *lc;
+     int                     id = 1;
+
+     if (!varName)
+             return list_length(vars);
+
+     foreach(lc, vars)
+     {
+             var = lfirst(lc);
+
+             if (!strncmp(var->name, varName, varNameLen))
+                     break;
+
+             var = NULL;
+             id++;
+     }
+
+     if (!var)
+             return -1;
+
+     /*
+      * When belonging to a JsonExpr, path variables are computed with the
+      * JsonExpr's ExprState (var->estate is NULL), so don't need to be computed
+      * here.  In some other cases, such as when the path variables belonging
+      * to a JsonTable instead, those variables must be evaluated on their own,
+      * without the enclosing JsonExpr itself needing to be evaluated, so must
+      * be handled here.
+      */
+     if (var->estate && !var->evaluated)
+     {
+             Assert(var->econtext != NULL);
+             var->value = ExecEvalExpr(var->estate, var->econtext, &var->isnull);
+             var->evaluated = true;

Uh, so this continues to do recursive expression evaluation, as
ExecEvalJsonExpr()->JsonPathQuery()->executeJsonPath(EvalJsonPathVar)

I'm getting grumpy here. This is wrong, has been pointed out many times. The
only thing that changes is that the point of recursion is moved around.

Actually, these JSON path vars, along with other sub-expressions of
JsonExpr, *are* computed non-recursively as ExprEvalSteps of the
JsonExprState, at least in the cases where the vars are to be computed
as part of evaluating the JsonExpr itself. So, the code path you've
shown above perhaps as a hypothetical doesn't really exist, though
there *is* an instance where these path vars are computed *outside*
the context of evaluating the parent JsonExpr, such as in
JsonTableResetContextItem(). Maybe there's a cleaner way of doing
that though...

+
+/*
+ * ExecEvalExprSafe
+ *
+ * Like ExecEvalExpr(), though this allows the caller to pass an
+ * ErrorSaveContext to declare its intenion to catch any errors that occur when
+ * executing the expression, such as when calling type input functions that may
+ * be present in it.
+ */
+static inline Datum
+ExecEvalExprSafe(ExprState *state,
+                              ExprContext *econtext,
+                              bool *isNull,
+                              Node *escontext,
+                              bool *error)

Afaict there's no caller of this?

Oops, removed. This was used in a previous version of the patch that
still had nested ExprStates inside JsonExprState.

+/*
+ * ExecInitExprWithCaseValue
+ *
+ * This is the same as ExecInitExpr, except the caller passes the Datum and
+ * bool pointers that it would like the ExprState.innermost_caseval
+ * and ExprState.innermost_casenull, respectively, to be set to.  That way,
+ * it can pass an input value to evaluate the expression via a CaseTestExpr.
+ */
+ExprState *
+ExecInitExprWithCaseValue(Expr *node, PlanState *parent,
+                                               Datum *caseval, bool *casenull)
+{
+     ExprState  *state;
+     ExprEvalStep scratch = {0};
+
+     /* Special case: NULL expression produces a NULL ExprState pointer */
+     if (node == NULL)
+             return NULL;
+
+     /* Initialize ExprState with empty step list */
+     state = makeNode(ExprState);
+     state->expr = node;
+     state->parent = parent;
+     state->ext_params = NULL;
+     state->innermost_caseval = caseval;
+     state->innermost_casenull = casenull;
+
+     /* Insert EEOP_*_FETCHSOME steps as needed */
+     ExecInitExprSlots(state, (Node *) node);
+
+     /* Compile the expression proper */
+     ExecInitExprRec(node, state, &state->resvalue, &state->resnull);
+
+     /* Finally, append a DONE step */
+     scratch.opcode = EEOP_DONE;
+     ExprEvalPushStep(state, &scratch);
+
+     ExecReadyExpr(state);
+
+     return state;

+struct JsonTableJoinState
+{
+     union
+     {
+             struct
+             {
+                     JsonTableJoinState *left;
+                     JsonTableJoinState *right;
+                     bool            advanceRight;
+             }                       join;
+             JsonTableScanState scan;
+     }                       u;
+     bool            is_join;
+};

A join state that unions the join member with a scan, and has a is_join field?

Yeah, I agree that's not the best form for what it is. I've replaced
that with the following:

+/* Structures for JSON_TABLE execution  */
+
+typedef enum JsonTablePlanStateType
+{
+   JSON_TABLE_SCAN_STATE = 0,
+   JSON_TABLE_JOIN_STATE
+} JsonTablePlanStateType;
+
+typedef struct JsonTablePlanState
+{
+   JsonTablePlanStateType  type;
+
+   struct JsonTablePlanState *parent;
+   struct JsonTablePlanState *nested;
+} JsonTablePlanState;
+
+typedef struct JsonTableScanState
+{
+   JsonTablePlanState  plan;
+
+   MemoryContext mcxt;
+   JsonPath   *path;
+   List       *args;
+   JsonValueList found;
+   JsonValueListIterator iter;
+   Datum       current;
+   int         ordinal;
+   bool        currentIsNull;
+   bool        outerJoin;
+   bool        errorOnError;
+   bool        advanceNested;
+   bool        reset;
+} JsonTableScanState;
+
+typedef struct JsonTableJoinState
+{
+   JsonTablePlanState  plan;
+
+   JsonTablePlanState *left;
+   JsonTablePlanState *right;
+   bool        advanceRight;
+} JsonTableJoinState;

I considered using NodeTag but decided not to, because this stuff is
local to jsonpath_exec.c.

+ * JsonTableInitOpaque
+ *           Fill in TableFuncScanState->opaque for JsonTable processor
+ */
+static void
+JsonTableInitOpaque(TableFuncScanState *state, int natts)
+{
+     JsonTableContext *cxt;
+     PlanState  *ps = &state->ss.ps;
+     TableFuncScan *tfs = castNode(TableFuncScan, ps->plan);
+     TableFunc  *tf = tfs->tablefunc;
+     JsonExpr   *ci = castNode(JsonExpr, tf->docexpr);
+     JsonTableParent *root = castNode(JsonTableParent, tf->plan);
+     List       *args = NIL;
+     ListCell   *lc;
+     int                     i;
+
+     cxt = palloc0(sizeof(JsonTableContext));
+     cxt->magic = JSON_TABLE_CONTEXT_MAGIC;
+
+     if (ci->passing_values)
+     {
+             ListCell   *exprlc;
+             ListCell   *namelc;
+
+             forboth(exprlc, ci->passing_values,
+                             namelc, ci->passing_names)
+             {
+                     Expr       *expr = (Expr *) lfirst(exprlc);
+                     String     *name = lfirst_node(String, namelc);
+                     JsonPathVariableEvalContext *var = palloc(sizeof(*var));
+
+                     var->name = pstrdup(name->sval);
+                     var->typid = exprType((Node *) expr);
+                     var->typmod = exprTypmod((Node *) expr);
+                     var->estate = ExecInitExpr(expr, ps);
+                     var->econtext = ps->ps_ExprContext;
+                     var->mcxt = CurrentMemoryContext;
+                     var->evaluated = false;
+                     var->value = (Datum) 0;
+                     var->isnull = true;
+
+                     args = lappend(args, var);
+             }
+     }
+
+     cxt->colexprs = palloc(sizeof(*cxt->colexprs) *
+                                                list_length(tf->colvalexprs));
+
+     JsonTableInitScanState(cxt, &cxt->root, root, NULL, args,
+                                                CurrentMemoryContext);
+
+     i = 0;
+
+     foreach(lc, tf->colvalexprs)
+     {
+             Expr       *expr = lfirst(lc);
+
+             cxt->colexprs[i].expr =
+                     ExecInitExprWithCaseValue(expr, ps,
+                                                                       &cxt->colexprs[i].scan->current,
+                                                                       &cxt->colexprs[i].scan->currentIsNull);
+
+             i++;
+     }
+
+     state->opaque = cxt;
+}

Why don't you just emit the proper expression directly, insted of the
CaseTestExpr stuff, that you then separately evaluate?

I suppose you mean emitting the expression that supplies the value
given by scan->current and scan->currentIsNull into the same ExprState
that holds the steps for a given colvalexpr. If so, I don't really
see a way of doing that given the current model of JSON_TABLE
execution. The former is computed as part of
TableFuncRoutine.FetchRow(scan), which sets scan.current (and
currentIsNull) and the letter is computer as part of
TableFuncRoutine.GetValue(scan, colnum).

Evaluating N expressions for a json table isn't a good approach, both memory
and CPU efficiency wise.

Are you referring to JsonTableInitOpaque() initializing all these
sub-expressions of JsonTableParent, especially colvalexprs, using N
*independent* ExprStates? That could perhaps be made to work by
making JsonTableParent be an expression recognized by execExpr.c, so
that a single ExprState can store the steps for all its
sub-expressions, much like JsonExpr is. I'll give that a try, though
I wonder if the semantics of making this work in a single
ExecEvalExpr() call will mismatch that of the current way, because
different sub-expressions are currently evaluated under different APIs
of TableFuncRoutine.

In the meantime, I'm attaching a version of the patchset with a few
things fixed as mentioned above.

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

On Mon, Feb 27, 2023 at 4:45 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Tue, Feb 21, 2023 at 2:25 AM Andres Freund <andres@anarazel.de> wrote:

Evaluating N expressions for a json table isn't a good approach, both memory
and CPU efficiency wise.

Are you referring to JsonTableInitOpaque() initializing all these
sub-expressions of JsonTableParent, especially colvalexprs, using N
*independent* ExprStates? That could perhaps be made to work by
making JsonTableParent be an expression recognized by execExpr.c, so
that a single ExprState can store the steps for all its
sub-expressions, much like JsonExpr is. I'll give that a try, though
I wonder if the semantics of making this work in a single
ExecEvalExpr() call will mismatch that of the current way, because
different sub-expressions are currently evaluated under different APIs
of TableFuncRoutine.

I was looking at this and realized that using N ExprStates for various
subsidiary expressions is not something specific to JSON_TABLE
implementation. I mean we already have bunch of ExprStates being
created in ExecInitTableFuncScan():

scanstate->ns_uris =
ExecInitExprList(tf->ns_uris, (PlanState *) scanstate);
scanstate->docexpr =
ExecInitExpr((Expr *) tf->docexpr, (PlanState *) scanstate);
scanstate->rowexpr =
ExecInitExpr((Expr *) tf->rowexpr, (PlanState *) scanstate);
scanstate->colexprs =
ExecInitExprList(tf->colexprs, (PlanState *) scanstate);
scanstate->coldefexprs =
ExecInitExprList(tf->coldefexprs, (PlanState *) scanstate);

Or maybe you're worried about jsonpath_exec.c using so many ExprStates
*privately* to put into TableFuncScanState.opaque?

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

On Mon, Feb 27, 2023 at 4:45 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Tue, Feb 21, 2023 at 2:25 AM Andres Freund <andres@anarazel.de> wrote:

Uh, so this continues to do recursive expression evaluation, as
ExecEvalJsonExpr()->JsonPathQuery()->executeJsonPath(EvalJsonPathVar)

I'm getting grumpy here. This is wrong, has been pointed out many times. The
only thing that changes is that the point of recursion is moved around.

Actually, these JSON path vars, along with other sub-expressions of
JsonExpr, *are* computed non-recursively as ExprEvalSteps of the
JsonExprState, at least in the cases where the vars are to be computed
as part of evaluating the JsonExpr itself. So, the code path you've
shown above perhaps as a hypothetical doesn't really exist, though
there *is* an instance where these path vars are computed *outside*
the context of evaluating the parent JsonExpr, such as in
JsonTableResetContextItem(). Maybe there's a cleaner way of doing
that though...

+ * JsonTableInitOpaque
+ *           Fill in TableFuncScanState->opaque for JsonTable processor
+ */
+static void
+JsonTableInitOpaque(TableFuncScanState *state, int natts)
+{
+     JsonTableContext *cxt;
+     PlanState  *ps = &state->ss.ps;
+     TableFuncScan *tfs = castNode(TableFuncScan, ps->plan);
+     TableFunc  *tf = tfs->tablefunc;
+     JsonExpr   *ci = castNode(JsonExpr, tf->docexpr);
+     JsonTableParent *root = castNode(JsonTableParent, tf->plan);
+     List       *args = NIL;
+     ListCell   *lc;
+     int                     i;
+
+     cxt = palloc0(sizeof(JsonTableContext));
+     cxt->magic = JSON_TABLE_CONTEXT_MAGIC;
+
+     if (ci->passing_values)
+     {
+             ListCell   *exprlc;
+             ListCell   *namelc;
+
+             forboth(exprlc, ci->passing_values,
+                             namelc, ci->passing_names)
+             {
+                     Expr       *expr = (Expr *) lfirst(exprlc);
+                     String     *name = lfirst_node(String, namelc);
+                     JsonPathVariableEvalContext *var = palloc(sizeof(*var));
+
+                     var->name = pstrdup(name->sval);
+                     var->typid = exprType((Node *) expr);
+                     var->typmod = exprTypmod((Node *) expr);
+                     var->estate = ExecInitExpr(expr, ps);
+                     var->econtext = ps->ps_ExprContext;
+                     var->mcxt = CurrentMemoryContext;
+                     var->evaluated = false;
+                     var->value = (Datum) 0;
+                     var->isnull = true;
+
+                     args = lappend(args, var);
+             }
+     }
+
+     cxt->colexprs = palloc(sizeof(*cxt->colexprs) *
+                                                list_length(tf->colvalexprs));
+
+     JsonTableInitScanState(cxt, &cxt->root, root, NULL, args,
+                                                CurrentMemoryContext);
+
+     i = 0;
+
+     foreach(lc, tf->colvalexprs)
+     {
+             Expr       *expr = lfirst(lc);
+
+             cxt->colexprs[i].expr =
+                     ExecInitExprWithCaseValue(expr, ps,
+                                                                       &cxt->colexprs[i].scan->current,
+                                                                       &cxt->colexprs[i].scan->currentIsNull);
+
+             i++;
+     }
+
+     state->opaque = cxt;
+}

Why don't you just emit the proper expression directly, insted of the
CaseTestExpr stuff, that you then separately evaluate?

I suppose you mean emitting the expression that supplies the value
given by scan->current and scan->currentIsNull into the same ExprState
that holds the steps for a given colvalexpr. If so, I don't really
see a way of doing that given the current model of JSON_TABLE
execution. The former is computed as part of
TableFuncRoutine.FetchRow(scan), which sets scan.current (and
currentIsNull) and the letter is computer as part of
TableFuncRoutine.GetValue(scan, colnum).

I looked around for another way to pass the value of evaluating one
expression (JsonTableParent.path) as input to the evaluation of
another (an expression in TableFunc.colvalexprs). The only thing that
came to mind is to use PARAM_EXEC parameters instead of CaseTestExpr
placeholders, though I'm not sure whether that is simpler or whether
that would really make things better?

Evaluating N expressions for a json table isn't a good approach, both memory
and CPU efficiency wise.

Are you referring to JsonTableInitOpaque() initializing all these
sub-expressions of JsonTableParent, especially colvalexprs, using N
*independent* ExprStates? That could perhaps be made to work by
making JsonTableParent be an expression recognized by execExpr.c, so
that a single ExprState can store the steps for all its
sub-expressions, much like JsonExpr is. I'll give that a try, though
I wonder if the semantics of making this work in a single
ExecEvalExpr() call will mismatch that of the current way, because
different sub-expressions are currently evaluated under different APIs
of TableFuncRoutine.

Hmm, the idea to turn JSON_TABLE into a single expression turned out
to be a non-starter after all, because, unlike JsonExpr, it can
produce multiple values. So there must be an ExprState for computing
each column of its output rows. As I mentioned in my other reply,
TableFuncScanState has a list of ExprStates anyway for
TableFunc.colexprs. What we could do is move the ExprStates of
TableFunc.colvalexprs into TableFuncScanState instead of making that
part of the JSON_TABLE opaque state, as I've done in the attached
updated patch.

I also found a way to not require ExecInitExprWithCaseValue() for the
initialization of those expressions by moving the responsibility of
passing the value of CaseTestExpr placeholder contained in those
expressions to the time of evaluating the expressions rather than
initialization time.

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

On Tue, Feb 28, 2023 at 8:36 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Feb 27, 2023 at 4:45 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Tue, Feb 21, 2023 at 2:25 AM Andres Freund <andres@anarazel.de> wrote:

Evaluating N expressions for a json table isn't a good approach, both memory
and CPU efficiency wise.

Are you referring to JsonTableInitOpaque() initializing all these
sub-expressions of JsonTableParent, especially colvalexprs, using N
*independent* ExprStates? That could perhaps be made to work by
making JsonTableParent be an expression recognized by execExpr.c, so
that a single ExprState can store the steps for all its
sub-expressions, much like JsonExpr is. I'll give that a try, though
I wonder if the semantics of making this work in a single
ExecEvalExpr() call will mismatch that of the current way, because
different sub-expressions are currently evaluated under different APIs
of TableFuncRoutine.

Hmm, the idea to turn JSON_TABLE into a single expression turned out
to be a non-starter after all, because, unlike JsonExpr, it can
produce multiple values. So there must be an ExprState for computing
each column of its output rows. As I mentioned in my other reply,
TableFuncScanState has a list of ExprStates anyway for
TableFunc.colexprs. What we could do is move the ExprStates of
TableFunc.colvalexprs into TableFuncScanState instead of making that
part of the JSON_TABLE opaque state, as I've done in the attached
updated patch.

Here's another version in which I've also moved the ExprStates of
PASSING args into TableFuncScanState instead of keeping them in
JSON_TABLE opaque state. That means all the subsidiary ExprStates of
TableFuncScanState are now initialized only once during
ExecInitTableFuncScan(). Previously, JSON_TABLE related ones would be
initialized on every call of JsonTableInitOpaque().

I've also done some cosmetic changes such as renaming the
JsonTableContext to JsonTableParseContext in parse_jsontable.c and to
JsonTableExecContext in jsonpath_exec.c.

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

On 2023-03-05 Su 22:18, Amit Langote wrote:

On Tue, Feb 28, 2023 at 8:36 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Feb 27, 2023 at 4:45 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Tue, Feb 21, 2023 at 2:25 AM Andres Freund <andres@anarazel.de> wrote:

Evaluating N expressions for a json table isn't a good approach, both memory
and CPU efficiency wise.

Are you referring to JsonTableInitOpaque() initializing all these
sub-expressions of JsonTableParent, especially colvalexprs, using N
*independent* ExprStates? That could perhaps be made to work by
making JsonTableParent be an expression recognized by execExpr.c, so
that a single ExprState can store the steps for all its
sub-expressions, much like JsonExpr is. I'll give that a try, though
I wonder if the semantics of making this work in a single
ExecEvalExpr() call will mismatch that of the current way, because
different sub-expressions are currently evaluated under different APIs
of TableFuncRoutine.

Hmm, the idea to turn JSON_TABLE into a single expression turned out
to be a non-starter after all, because, unlike JsonExpr, it can
produce multiple values. So there must be an ExprState for computing
each column of its output rows. As I mentioned in my other reply,
TableFuncScanState has a list of ExprStates anyway for
TableFunc.colexprs. What we could do is move the ExprStates of
TableFunc.colvalexprs into TableFuncScanState instead of making that
part of the JSON_TABLE opaque state, as I've done in the attached
updated patch.

Here's another version in which I've also moved the ExprStates of
PASSING args into TableFuncScanState instead of keeping them in
JSON_TABLE opaque state. That means all the subsidiary ExprStates of
TableFuncScanState are now initialized only once during
ExecInitTableFuncScan(). Previously, JSON_TABLE related ones would be
initialized on every call of JsonTableInitOpaque().

I've also done some cosmetic changes such as renaming the
JsonTableContext to JsonTableParseContext in parse_jsontable.c and to
JsonTableExecContext in jsonpath_exec.c.

Hi, I have just spent some time going through the first five patches
(i.e. those that precede the JSONTABLE patches) and Andres's comments in

</messages/by-id/20230220172456.q3oshnvfk3wyhm5l@awork3.anarazel.de&gt;

AFAICT there are only two possible matters of concern that remain, both
regarding the grammar.

First is this general complaint:

This stuff adds quite a bit of complexity to the parser. Do we realy need like
a dozen new rules here?

I mentioned that more than a year ago, I think, without anybody taking
the matter up, so I didn't pursue it. I guess I should have.

There are probably some fairly easy opportunities to reduce the number
of non-terminals introduced here (e.g. I think json_aggregate_func could
possibly be expanded in place without introducing
json_object_aggregate_constructor and json_array_aggregate_constructor).
I'm going to make an attempt at that, at least to pick some low hanging
fruit. But in the end I think we are going to be left with a significant
expansion of the grammar rules, more or less forced on us by the way the
SQL Standards Committee rather profligately invents new ways of
contorting the grammar.

Second is this complaint:

+json_behavior_empty_array:
+			EMPTY_P ARRAY	{ $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
+			/* non-standard, for Oracle compatibility only */
+			| EMPTY_P		{ $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
+		;
Do we really want to add random oracle compat crud here?

I think this case is pretty harmless, and it literally involves one line
of code, so I'm inclined to leave it.

These both seem like things not worth holding up progress for, and I
think it would be good to get these patches committed as soon as
possible. My intention is to commit them (after some grammar
adjustments) plus their documentation in the next few days. That would
leave the JSONTABLE patches still to go. They are substantial, but a far
more manageable chunk of work for some committer (not me) once we get
this foundational piece in.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com