Re: remaining sql/json patches

On Wed, Mar 13, 2024 at 5:47 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> About 0002:
>
> I think we should just drop it. Look at the changes it produces in the
> plans for aliases XMLTABLE:
>
> > @@ -1556,7 +1556,7 @@ SELECT f.* FROM xmldata, LATERAL xmltable('/ROWS/ROW[COUNTRY_NAME="Japan" or COU
> > Output: f."COUNTRY_NAME", f."REGION_ID"
> > -> Seq Scan on public.xmldata
> > Output: xmldata.data
> > - -> Table Function Scan on "xmltable" f
> > + -> Table Function Scan on "XMLTABLE" f
> > Output: f."COUNTRY_NAME", f."REGION_ID"
> > Table Function Call: XMLTABLE(('/ROWS/ROW[COUNTRY_NAME="Japan" or COUNTRY_NAME="India"]'::text) PASSING (xmldata.data) COLUMNS "COUNTRY_NAME" text, "REGION_ID" integer)
> > Filter: (f."COUNTRY_NAME" = 'Japan'::text)
>
> Here in text-format EXPLAIN, we already have the alias next to the
> "xmltable" moniker, when an alias is present. This matches the
> query itself as well as the labels used in the "Output:" display.
> If an alias is not present, then this says just 'Table Function Scan on "xmltable"'
> and the rest of the plans refers to this as "xmltable", so it's also
> fine.
>
> > @@ -1591,7 +1591,7 @@ SELECT f.* FROM xmldata, LATERAL xmltable('/ROWS/ROW[COUNTRY_NAME="Japan" or COU
> > "Parent Relationship": "Inner", +
> > "Parallel Aware": false, +
> > "Async Capable": false, +
> > - "Table Function Name": "xmltable", +
> > + "Table Function Name": "XMLTABLE", +
> > "Alias": "f", +
> > "Output": ["f.\"COUNTRY_NAME\"", "f.\"REGION_ID\""], +
> > "Table Function Call": "XMLTABLE(('/ROWS/ROW[COUNTRY_NAME=\"Japan\" or COUNTRY_NAME=\"India\"]'::text) PASSING (xmldata.data) COLUMNS \"COUNTRY_NAME\" text, \"REGION_ID\" integer)",+
>
> This is the JSON-format explain. Notice that the "Alias" member already
> shows the alias "f", so the only thing this change is doing is
> uppercasing the "xmltable" to "XMLTABLE". We're not really achieving
> anything here.
>
> I think the only salvageable piece from this, **if anything**, is making
> the "xmltable" literal string into uppercase. That might bring a little
> clarity to the fact that this is a keyword and not a user-introduced
> name.
>
>
> In your 0003 I think this would only have relevance in this query,
>
> +-- JSON_TABLE() with alias
> +EXPLAIN (COSTS OFF, VERBOSE)
> +SELECT * FROM
> + JSON_TABLE(
> + jsonb 'null', 'lax $[*]' PASSING 1 + 2 AS a, json '"foo"' AS "b c"
> + COLUMNS (
> + id FOR ORDINALITY,
> + "int" int PATH '$',
> + "text" text PATH '$'
> + )) json_table_func;
> + QUERY PLAN
>
> +--------------------------------------------------------------------------------------------------------------------------------------------------------------
> ----------------------------------------------------------
> + Table Function Scan on "JSON_TABLE" json_table_func
> + Output: id, "int", text
> + Table Function Call: JSON_TABLE('null'::jsonb, '$[*]' AS json_table_path_0 PASSING 3 AS a, '"foo"'::jsonb AS "b c" COLUMNS (id FOR ORDINALITY, "int" integer PATH '$', text text PATH '$') PLAN (json_table_path_0))
> +(3 rows)
>
> and I'm curious to see what this would output if this was to be run
> without the 0002 patch. If I understand things correctly, the alias
> would be displayed anyway, meaning 0002 doesn't get us anything.

Patch 0002 came about because old versions of json_table patch were
changing ExplainTargetRel() incorrectly to use rte->tablefunc to get
the function type to set objectname, but rte->tablefunc is NULL
because add_rte_to_flat_rtable() zaps it. You pointed that out in
[1].

However, we can get the TableFunc to get the function type from the
Plan node instead of the RTE, as follows:

- Assert(rte->rtekind == RTE_TABLEFUNC);
- objectname = "xmltable";
- objecttag = "Table Function Name";
+ {
+ TableFunc *tablefunc = ((TableFuncScan *) plan)->tablefunc;
+
+ Assert(rte->rtekind == RTE_TABLEFUNC);
+ switch (tablefunc->functype)
+ {
+ case TFT_XMLTABLE:
+ objectname = "xmltable";
+ break;
+ case TFT_JSON_TABLE:
+ objectname = "json_table";
+ break;
+ default:
+ elog(ERROR, "invalid TableFunc type %d",
+ (int) tablefunc->functype);
+ }
+ objecttag = "Table Function Name";
+ }

So that gets us what we need here.

Given that, 0002 does seem like an overkill and unnecessary, so I'll drop it.

> Please do add a test with EXPLAIN (FORMAT JSON) in 0003.

OK, will do.

--
Thanks, Amit Langote

[1] https://www.postgresql.org/message-id/202401181711.qxjxpnl3ohnw%40alvherre.pgsql