On Wed, Mar 13, 2024 at 5:47 AM Alvaro Herrera <[email protected]> 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
