Re: remaining sql/json patches

On Wed, Mar 13, 2024 at 5:47 AM Alvaro Herrera <alvherre@alvh.no-ip.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