Re: remaining sql/json patches

On Tue, Jan 23, 2024 at 1:19 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> On 2024-Jan-18, Alvaro Herrera wrote:
> > > commands/explain.c (Hmm, I think this is a preexisting bug actually)
> > >
> > > 3893 18 : case T_TableFuncScan:
> > > 3894 18 : Assert(rte->rtekind == RTE_TABLEFUNC);
> > > 3895 18 : if (rte->tablefunc)
> > > 3896 0 : if (rte->tablefunc->functype == TFT_XMLTABLE)
> > > 3897 0 : objectname = "xmltable";
> > > 3898 : else /* Must be TFT_JSON_TABLE */
> > > 3899 0 : objectname = "json_table";
> > > 3900 : else
> > > 3901 18 : objectname = NULL;
> > > 3902 18 : objecttag = "Table Function Name";
> > > 3903 18 : break;
> >
> > Indeed
>
> I was completely wrong about this, and in order to gain coverage the
> only thing we needed was to add an EXPLAIN that uses the JSON format.
> I did that just now. I think your addition here works just fine.

I think we'd still need your RangeTblFunc.tablefunc_name in order for
the new code (with JSON_TABLE) to be able to set objectname to either
"XMLTABLE" or "JSON_TABLE", no?

As you pointed out, rte->tablefunc is always NULL in
ExplainTargetRel() due to setrefs.c setting it to NULL, so the
JSON_TABLE additions to explain.c in my patch as they were won't work.
I've included your patch in the attached set and adjusted the
JSON_TABLE patch to set tablefunc_name in the parser.

I had intended to push 0001-0004 today, but held off to add a
SQL-callable testing function for the changes in 0002. On that note,
I'm now not so sure about committing jsonpath_exec.c functions
JsonPathExists/Query/Value() from their SQL/JSON counterparts, so
inclined to squash that one into the SQL/JSON query functions patch
from a testability standpoint.

I haven't looked at Jian He's comments yet.

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