Re: remaining sql/json patches

Hi,

On Fri, Dec 22, 2023 at 10:01 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> Hi

Thanks for the reviews.

> v33-0007-SQL-JSON-query-functions.patch, commit message:
> This introduces the SQL/JSON functions for querying JSON data using
> jsonpath expressions. The functions are:
>
> should it be "These functions are"

Rewrote that sentence to say "introduces the following SQL/JSON functions..."

> + <para>
> + Returns true if the SQL/JSON <replaceable>path_expression</replaceable>
> + applied to the <replaceable>context_item</replaceable> using the
> + <replaceable>value</replaceable>s yields any items.
> + The <literal>ON ERROR</literal> clause specifies what is returned if
> + an error occurs; the default is to return <literal>FALSE</literal>.
> + Note that if the <replaceable>path_expression</replaceable>
> + is <literal>strict</literal>, an error is generated if it
> yields no items.
> + </para>
>
> I think the following description is more accurate.
> + Note that if the <replaceable>path_expression</replaceable>
> + is <literal>strict</literal> and the <literal>ON
> ERROR</literal> clause is <literal> ERROR</literal>,
> + an error is generated if it yields no items.
> + </para>

True, fixed.

> +/*
> + * transformJsonTable -
> + * Transform a raw JsonTable into TableFunc.
> + *
> + * Transform the document-generating expression, the row-generating expression,
> + * the column-generating expressions, and the default value expressions.
> + */
> +ParseNamespaceItem *
> +transformJsonTable(ParseState *pstate, JsonTable *jt)
> +{
> + JsonTableParseContext cxt;
> + TableFunc *tf = makeNode(TableFunc);
> + JsonFuncExpr *jfe = makeNode(JsonFuncExpr);
> + JsonExpr *je;
> + JsonTablePlan *plan = jt->plan;
> + char *rootPathName = jt->pathname;
> + char *rootPath;
> + bool is_lateral;
> +
> + if (jt->on_empty)
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("ON EMPTY not allowed in JSON_TABLE"),
> + parser_errposition(pstate,
> + exprLocation((Node *) jt->on_empty))));
>
> This error may be slightly misleading?
> you can add ON EMPTY inside the COLUMNS part, like the following:
> SELECT * FROM (VALUES ('1'), ('"1"')) vals(js) LEFT OUTER JOIN
> JSON_TABLE(vals.js::jsonb, '$' COLUMNS (a int PATH '$' default 1 ON
> empty)) jt ON true;

That check is to catch an ON EMPTY specified *outside* the COLUMN(...)
clause of a JSON_TABLE(...) expression. It was added during a recent
gram.y refactoring, but maybe that wasn't a great idea. It seems
better to disallow the ON EMPTY clause in the grammar itself.

> + <para>
> + Each <literal>NESTED PATH</literal> clause can generate one or more
> + columns. Columns produced by <literal>NESTED PATH</literal>s at the
> + same level are considered to be <firstterm>siblings</firstterm>,
> + while a column produced by a <literal>NESTED PATH</literal> is
> + considered to be a child of the column produced by a
> + <literal>NESTED PATH</literal> or row expression at a higher level.
> + Sibling columns are always joined first. Once they are processed,
> + the resulting rows are joined to the parent row.
> + </para>
> Does changing to the following make sense?
> + considered to be a <firstterm>child</firstterm> of the column produced by a
> + the resulting rows are joined to the <firstterm>parent</firstterm> row.

Terms "child" and "parent" are already introduced in previous
paragraphs, so no need for the <firstterm> tag.

> seems like `format json_representation`, not listed in the
> documentation, but json_representation is "Parameters", do we need
> add a section to explain it?
> even though I think currently we can only do `FORMAT JSON`.

The syntax appears to allow an optional ENCODING UTF8 too, so I've
gotten rid of json_representation and literally listed out what the
syntax says.

> SELECT * FROM JSON_TABLE(jsonb '123', '$' COLUMNS (item int PATH '$'
> empty on empty)) bar;
> ERROR: cannot cast jsonb array to type integer
> The error is the same as the output of the following:
> SELECT * FROM JSON_TABLE(jsonb '123', '$' COLUMNS (item int PATH '$'
> empty array on empty )) bar;
> but these two are different things?

EMPTY and EMPTY ARRAY both spell out an array:

json_behavior_type:
...
| EMPTY_P ARRAY { $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
/* non-standard, for Oracle compatibility only */
| EMPTY_P { $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }

> + /* FALLTHROUGH */
> + case JTC_EXISTS:
> + case JTC_FORMATTED:
> + {
> + Node *je;
> + CaseTestExpr *param = makeNode(CaseTestExpr);
> +
> + param->collation = InvalidOid;
> + param->typeId = cxt->contextItemTypid;
> + param->typeMod = -1;
> +
> + if (rawc->wrapper != JSW_NONE &&
> + rawc->quotes != JS_QUOTES_UNSPEC)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("cannot use WITH WRAPPER clause for formatted colunmns"
> + " without also specifying OMIT/KEEP QUOTES"),
> + parser_errposition(pstate, rawc->location)));
>
> typo, should be "formatted columns".

Oops.

> I suspect people will be confused with the meaning of "formatted column".
> maybe we can replace this part:"cannot use WITH WRAPPER clause for
> formatted column"
> to
> "SQL/JSON WITH WRAPPER behavior must not be specified when FORMAT
> clause is used"
>
> SELECT * FROM JSON_TABLE(jsonb '"world"', '$' COLUMNS (item text
> FORMAT JSON PATH '$' with wrapper KEEP QUOTES));
> ERROR: cannot use WITH WRAPPER clause for formatted colunmns without
> also specifying OMIT/KEEP QUOTES
> LINE 1: ...T * FROM JSON_TABLE(jsonb '"world"', '$' COLUMNS (item text ...
> ^
> this error is misleading, since now I am using WITH WRAPPER clause for
> formatted columns and specified KEEP QUOTES.
>
> in parse_expr.c, we have errmsg("SQL/JSON QUOTES behavior must not be
> specified when WITH WRAPPER is used").

It seems to me that we should just remove the above check in
appendJsonTableColumns() and let the check(s) in parse_expr.c take
care of the various allowed/disallowed scenarios for "formatted"
columns. Also see further below...

> +/*
> + * Fetch next row from a cross/union joined scan.
> + *
> + * Returns false at the end of a scan, true otherwise.
> + */
> +static bool
> +JsonTablePlanNextRow(JsonTablePlanState * state)
> +{
> + JsonTableJoinState *join;
> +
> + if (state->type == JSON_TABLE_SCAN_STATE)
> + return JsonTableScanNextRow((JsonTableScanState *) state);
> +
> + join = (JsonTableJoinState *) state;
> + if (join->advanceRight)
> + {
> + /* fetch next inner row */
> + if (JsonTablePlanNextRow(join->right))
> + return true;
> +
> + /* inner rows are exhausted */
> + if (join->cross)
> + join->advanceRight = false; /* next outer row */
> + else
> + return false; /* end of scan */
> + }
> +
> + while (!join->advanceRight)
> + {
> + /* fetch next outer row */
> + bool left = JsonTablePlanNextRow(join->left);
>
> + bool left = JsonTablePlanNextRow(join->left);
> JsonTablePlanNextRow function comment says "Returns false at the end
> of a scan, true otherwise.",
> so bool variable name as "left" seems not so good?

Hmm, maybe, "more" might be more appropriate given the context.

> It might help others understand the whole code by adding some comments on
> struct JsonTableScanState and struct JsonTableJoinState.
> since json_table patch is quite recursive, IMHO.

Agree that the various JsonTable parser/executor comments are lacking.
Working on adding more commentary and improving the notation -- struct
names, etc.

> I did some minor refactoring in parse_expr.c, since some code like
> transformJsonExprCommon is duplicated.

Thanks, I've adopted some of the ideas in your patch.

On Mon, Dec 25, 2023 at 2:03 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> +/*
> + * JsonTableFetchRow
> + * Prepare the next "current" tuple for upcoming GetValue calls.
> + * Returns FALSE if the row-filter expression returned no more rows.
> + */
> +static bool
> +JsonTableFetchRow(TableFuncScanState *state)
> +{
> + JsonTableExecContext *cxt =
> + GetJsonTableExecContext(state, "JsonTableFetchRow");
> +
> + if (cxt->empty)
> + return false;
> +
> + return JsonTableScanNextRow(cxt->root);
> +}
>
> The declaration of struct JsonbTableRoutine, SetRowFilter field is
> null. So I am confused by the above comment.

Yeah, it might be a leftover from copy-pasting the XML code. Reworded
the comment to not mention SetRowFilter.

> also seems the `if (cxt->empty)` part never called.

I don't understand why the context struct has that empty flag too, it
might be a leftover field. Removed.

> +static inline JsonTableExecContext *
> +GetJsonTableExecContext(TableFuncScanState *state, const char *fname)
> +{
> + JsonTableExecContext *result;
> +
> + if (!IsA(state, TableFuncScanState))
> + elog(ERROR, "%s called with invalid TableFuncScanState", fname);
> + result = (JsonTableExecContext *) state->opaque;
> + if (result->magic != JSON_TABLE_EXEC_CONTEXT_MAGIC)
> + elog(ERROR, "%s called with invalid TableFuncScanState", fname);
> +
> + return result;
> +}
> I think Assert(IsA(state, TableFuncScanState)) would be better.

Hmm, better to leave this as-is to be consistent with what the XML
code is doing. Though I also wonder why it's not an Assert in the
first place.

> +/*
> + * JsonTablePlanType -
> + * flags for JSON_TABLE plan node types representation
> + */
> +typedef enum JsonTablePlanType
> +{
> + JSTP_DEFAULT,
> + JSTP_SIMPLE,
> + JSTP_JOINED,
> +} JsonTablePlanType;
> it would be better to add some comments on it. thanks.
>
> JsonTablePlanNextRow is quite recursive! Adding more explanation would
> be helpful, thanks.

Will do.

> +/* Recursively reset scan and its child nodes */
> +static void
> +JsonTableRescanRecursive(JsonTablePlanState * state)
> +{
> + if (state->type == JSON_TABLE_JOIN_STATE)
> + {
> + JsonTableJoinState *join = (JsonTableJoinState *) state;
> +
> + JsonTableRescanRecursive(join->left);
> + JsonTableRescanRecursive(join->right);
> + join->advanceRight = false;
> + }
> + else
> + {
> + JsonTableScanState *scan = (JsonTableScanState *) state;
> +
> + Assert(state->type == JSON_TABLE_SCAN_STATE);
> + JsonTableRescan(scan);
> + if (scan->plan.nested)
> + JsonTableRescanRecursive(scan->plan.nested);
> + }
> +}
>
> From the coverage report, I noticed the first IF branch in
> JsonTableRescanRecursive never called.

Will look into this.

> + foreach(col, columns)
> + {
> + JsonTableColumn *rawc = castNode(JsonTableColumn, lfirst(col));
> + Oid typid;
> + int32 typmod;
> + Node *colexpr;
> +
> + if (rawc->name)
> + {
> + /* make sure column names are unique */
> + ListCell *colname;
> +
> + foreach(colname, tf->colnames)
> + if (!strcmp((const char *) colname, rawc->name))
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("column name \"%s\" is not unique",
> + rawc->name),
> + parser_errposition(pstate, rawc->location)));
>
> this `/* make sure column names are unique */` logic part already
> validated in isJsonTablePathNameDuplicate, so we don't need it?
> actually isJsonTablePathNameDuplicate validates both column name and pathname.

I think you are right. All columns/path names are de-duplicated much
earlier at the beginning of transformJsonTable(), so there's no need
for the above check.

That said, I don't know why column and path names share the namespace
or whether that has any semantic issues. Maybe there aren't, but will
think some more on that.

> select jt.* from jsonb_table_test jtt,
> json_table (jtt.js,'strict $[*]' as p
> columns (n for ordinality,
> nested path 'strict $.b[*]' as pb columns ( c int path '$' ),
> nested path 'strict $.b[*]' as pb columns ( s int path '$' ))
> ) jt;
>
> ERROR: duplicate JSON_TABLE column name: pb
> HINT: JSON_TABLE column names must be distinct from one another.
> the error is not very accurate, since pb is a pathname?

I think this can be improved by passing the information whether it's a
column or path name to the deduplication code. I've reworked that
code to get more useful error info.

On Wed, Jan 3, 2024 at 7:53 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> some more minor issues:
> SELECT * FROM JSON_TABLE(jsonb '{"a":[123,2]}', '$'
> COLUMNS (item int[] PATH '$.a' error on error, foo text path '$'
> error on error)) bar;
> ERROR: JSON path expression in JSON_VALUE should return singleton scalar item
>
> the error message seems not so great, imho.
> since the JSON_TABLE doc entries didn't mention that
> JSON_TABLE actually transformed to json_value, json_query, json_exists.

Hmm, yes, the context whether the JSON_VALUE() is user-specified or
internally generated is not readily available where the error is
reported.

I'm inlinced to document this aspect of JSON_TABLE(), instead of
complicating the executor interfaces in order to make the error
message better.

> JSON_VALUE even though cannot specify KEEP | OMIT QUOTES.
> It might be a good idea to mention the default is to omit quotes in the doc.
> because JSON_TABLE actually transformed to json_value, json_query, json_exists.
> JSON_TABLE can specify quotes behavior freely.

Done.

> (json_query)
> + This function must return a JSON string, so if the path expression
> + returns multiple SQL/JSON items, you must wrap the result using the
> + <literal>WITH WRAPPER</literal> clause. If the wrapper is
> + <literal>UNCONDITIONAL</literal>, an array wrapper will always
> + be applied, even if the returned value is already a single JSON object
> + or an array, but if it is <literal>CONDITIONAL</literal>, it
> will not be
> + applied to a single array or object. <literal>UNCONDITIONAL</literal>
> + is the default. If the result is a scalar string, by default the value
> + returned will have surrounding quotes making it a valid JSON value,
> + which can be made explicit by specifying <literal>KEEP
> QUOTES</literal>.
> + Conversely, quotes can be omitted by specifying <literal>OMIT
> QUOTES</literal>.
> + The returned <replaceable>data_type</replaceable> has the
> same semantics
> + as for constructor functions like <function>json_objectagg</function>;
> + the default returned type is <type>jsonb</type>.
>
> + <para>
> + Returns the result of applying the
> + <replaceable>path_expression</replaceable> to the
> + <replaceable>context_item</replaceable> using the
> + <literal>PASSING</literal> <replaceable>value</replaceable>s. The
> + extracted value must be a single <acronym>SQL/JSON</acronym> scalar
> + item. For results that are objects or arrays, use the
> + <function>json_query</function> function instead.
> + The returned <replaceable>data_type</replaceable> has the
> same semantics
> + as for constructor functions like <function>json_objectagg</function>.
> + The default returned type is <type>text</type>.
> + The <literal>ON ERROR</literal> and <literal>ON EMPTY</literal>
> + clauses have similar semantics as mentioned in the description of
> + <function>json_query</function>.
> + </para>
>
> + The returned <replaceable>data_type</replaceable> has the
> same semantics
> + as for constructor functions like <function>json_objectagg</function>.
>
> IMHO, the above description is not so good, since the function
> json_objectagg is listed in functions-aggregate.html,
> using Ctrl + F in the browser cannot find json_objectagg in functions-json.html.
>
> for json_query, maybe we can rephrase like:
> the RETURNING clause, which specifies the data type returned. It must
> be a type for which there is a cast from text to that type.
> By default, the <type>jsonb</type> type is returned.
>
> json_value:
> the RETURNING clause, which specifies the data type returned. It must
> be a type for which there is a cast from text to that type.
> By default, the <type>text</type> type is returned.

Fixed the description of returned type for both json_query() and
json_value(). For the latter, the cast to the returned type must
exist from each possible JSON scalar type viz. text, boolean, numeric,
and various datetime types.

On Wed, Jan 3, 2024 at 7:50 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> Hi. still based on v33.
> JSON_TABLE:
> I also refactor parse_jsontable.c error reporting, now the error
> message will be consistent with json_query.
> now you can specify wrapper freely as long as you don't specify
> wrapper and quote at the same time.
> overall, json_table behavior is more consistent with json_query and json_value.
> I also added some tests.

Thanks for the patches. I've taken the tests, some of your suggested
code changes, and made some changes of my own. Some of the new tests
give a different error message than what your patch had but I think
what I have is fine.

> +void
> +ExecEvalJsonCoercion(ExprState *state, ExprEvalStep *op,
> + ExprContext *econtext)
> +{
> + JsonCoercion *coercion = op->d.jsonexpr_coercion.coercion;
> + ErrorSaveContext *escontext = op->d.jsonexpr_coercion.escontext;
> + Datum res = *op->resvalue;
> + bool resnull = *op->resnull;
> +
> + if (coercion->via_populate)
> + {
> + void *cache = op->d.jsonexpr_coercion.json_populate_type_cache;
> +
> + *op->resvalue = json_populate_type(res, JSONBOID,
> + coercion->targettype,
> + coercion->targettypmod,
> + &cache,
> + econtext->ecxt_per_query_memory,
> + op->resnull, (Node *) escontext);
> + }
> + else if (coercion->via_io)
> + {
> + FmgrInfo *input_finfo = op->d.jsonexpr_coercion.input_finfo;
> + Oid typioparam = op->d.jsonexpr_coercion.typioparam;
> + char *val_string = resnull ? NULL :
> + JsonbUnquote(DatumGetJsonbP(res));
> +
> + (void) InputFunctionCallSafe(input_finfo, val_string, typioparam,
> + coercion->targettypmod,
> + (Node *) escontext,
> + op->resvalue);
> + }
> via_populate, via_io should be mutually exclusive.
> your patch, in some cases, both (coercion->via_io) and
> (coercion->via_populate) are true.
> (we can use elog find out).
> I refactor coerceJsonFuncExprOutput, so now it will be mutually exclusive.
> I also add asserts to it.

I realized that we don't really need the via_io and via_populate
flags. You can see in the latest patch that the decision of whether
to call json_populate_type() or the RETURNING type's input function is
now deferred to run-time or ExecEvalJsonCoercion(). The new comment
should also make it clear why one or the other is used for a given
source datum passed to ExecEvalJsonCoercion().

> By default, json_query keeps quotes, json_value omit quotes.
> However, json_table will be transformed to json_value or json_query
> based on certain criteria,
> that means we need to explicitly set the JsonExpr->omit_quotes in the
> function transformJsonFuncExpr
> for case JSON_QUERY_OP and JSON_VALUE_OP.
>
> We need to know the QUOTE behavior in the function ExecEvalJsonCoercion.
> Because for ExecEvalJsonCoercion, the coercion datum source can be a
> scalar string item,
> scalar items means RETURNING clause is dependent on QUOTE behavior.
> keep quotes, omit quotes the results are different.
> consider
> JSON_QUERY(jsonb'{"rec": "[1,2]"}', '$.rec' returning int4range omit quotes);
> and
> JSON_QUERY(jsonb'{"rec": "[1,2]"}', '$.rec' returning int4range omit quotes);
>
> to make sure ExecEvalJsonCoercion can distinguish keep and omit quotes,
> I added a bool keep_quotes to struct JsonCoercion.
> (maybe there is a more simple way, so far, that's what I come up with).
> the keep_quotes value will be settled in the function transformJsonFuncExpr.
> After refactoring, in ExecEvalJsonCoercion, keep_quotes is true then
> call JsonbToCString, else call JsonbUnquote.
>
> example:
> SELECT JSON_QUERY(jsonb'{"rec": "{1,2,3}"}', '$.rec' returning int[]
> omit quotes);
> without my changes, return NULL
> with my changes:
> {1,2,3}
>
> JSON_VALUE:
> main changes:
> --- a/src/test/regress/expected/jsonb_sqljson.out
> +++ b/src/test/regress/expected/jsonb_sqljson.out
> @@ -301,7 +301,11 @@ SELECT JSON_VALUE(jsonb '"2017-02-20"', '$'
> RETURNING date) + 9;
> -- Test NULL checks execution in domain types
> CREATE DOMAIN sqljsonb_int_not_null AS int NOT NULL;
> SELECT JSON_VALUE(jsonb 'null', '$' RETURNING sqljsonb_int_not_null);
> -ERROR: domain sqljsonb_int_not_null does not allow null values
> + json_value
> +------------
> +
> +(1 row)
> +
> I think the change is correct, given `SELECT JSON_VALUE(jsonb 'null',
> '$' RETURNING int4range);` returns NULL.
>
> I also attached a test.sql, without_patch.out (apply v33 only),
> with_patch.out (my changes based on v33).
> So you can see the difference after applying the patch, in case, my
> wording is not clear.

To address these points:

* I've taken your idea to make omit/keep_quotes available to
ExecEvalJsonCoercion().

* I've also taken your suggestion to fix parse_jsontable.c such that
WRAPPER/QUOTES combinations specified with JSON_TABLE() columns work
without many arbitrary-looking restrictions.

Please take a look at the attached latest patch and let me know if
anything looks amiss.

On Sat, Jan 6, 2024 at 9:45 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> some tests after applying V33 and my small changes.
> setup:
> create table test_scalar1(js jsonb);
> insert into test_scalar1 select jsonb '{"a":"[12,13]"}' FROM
> generate_series(1,1e5) g;
> create table test_scalar2(js jsonb);
> insert into test_scalar2 select jsonb '{"a":12}' FROM generate_series(1,1e5) g;
> create table test_array1(js jsonb);
> insert into test_array1 select jsonb '{"a":[1,2,3,4,5]}' FROM
> generate_series(1,1e5) g;
> create table test_array2(js jsonb);
> insert into test_array2 select jsonb '{"a": "{1,2,3,4,5}"}' FROM
> generate_series(1,1e5) g;
>
> tests:
> ----------------------------------------return a scalar int4range
> explain(costs off,analyze) SELECT item FROM test_scalar1,
> JSON_TABLE(js, '$.a' COLUMNS (item int4range PATH '$' omit quotes))
> \watch count=5
> 237.753 ms
>
> explain(costs off,analyze) select json_query(js, '$.a' returning
> int4range omit quotes) from test_scalar1 \watch count=5
> 462.379 ms
>
> explain(costs off,analyze) select json_value(js,'$.a' returning
> int4range) from test_scalar1 \watch count=5
> 362.148 ms
>
> explain(costs off,analyze) select (js->>'a')::int4range from
> test_scalar1 \watch count=5
> 301.089 ms
>
> explain(costs off,analyze) select trim(both '"' from
> jsonb_path_query_first(js,'$.a')::text)::int4range from test_scalar1
> \watch count=5
> 643.337 ms
> ---------------------------------
> overall, json_value is faster than json_query. but json_value can not
> deal with arrays in some cases.

I think that may be explained by the fact that JsonPathQuery() has
this step, which JsonPathValue() does not:

if (singleton)
return JsonbPGetDatum(JsonbValueToJsonb(singleton));

I can see JsonbValueToJsonb() in perf profile when running the
benchmark you shared. I don't know if there's any way to make that
better.

> but as you can see, in some cases, json_value and json_query are not
> as fast as our current implementation

Yeah, there *is* some expected overhead to using the new functions;
ExecEvalJsonExprPath() appears in the top 5 frames of perf profile,
for example. The times I see are similar to yours and I don't find
the difference to be very drastic.

postgres=# \o /dev/null
postgres=# explain(costs off,analyze) select (js->>'a') from
test_scalar1 \watch count=3
Time: 21.581 ms
Time: 18.838 ms
Time: 21.589 ms

postgres=# explain(costs off,analyze) select json_query(js,'$.a') from
test_scalar1 \watch count=3
Time: 38.562 ms
Time: 34.251 ms
Time: 32.681 ms

postgres=# explain(costs off,analyze) select json_value(js,'$.a') from
test_scalar1 \watch count=3
Time: 28.595 ms
Time: 23.947 ms
Time: 25.334 ms

postgres=# explain(costs off,analyze) select item from test_scalar1,
json_table(js, '$.a' columns (item int4range path '$')); \watch
count=3
Time: 52.739 ms
Time: 53.996 ms
Time: 50.774 ms

Attached v34 of all of the patches. 0008 may be considered to be WIP
given the points I mentioned above -- need to add a bit more
commentary about JSON_TABLE plan implementation and other
miscellaneous fixes.

As said in my previous email, I'd like to commit 0001-0007 next week.

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