Re: remaining sql/json patches

Hi.

+/*
+ * 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.
also seems the  `if (cxt->empty)` part never called.

+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.

+/*
+ * 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.

+/* 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.

+ 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.

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?