Re: remaining sql/json patches - Mailing list pgsql-hackers

From Amit Langote
Subject Re: remaining sql/json patches
Date
Msg-id CA+HiwqGN6DUKAVabqEhQ8e=0593emvJ6P4rQ_8x8N84k=mWq1Q@mail.gmail.com
Whole thread Raw
In response to Re: remaining sql/json patches  (Alvaro Herrera <alvherre@alvh.no-ip.org>)
Responses Re: remaining sql/json patches
List pgsql-hackers
On Tue, Jan 23, 2024 at 1:19 AM Alvaro Herrera <alvherre@alvh.no-ip.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

Attachment

pgsql-hackers by date:

Previous
From: Aleksander Alekseev
Date:
Subject: Re: Support TZ format code in to_timestamp()
Next
From: Dave Cramer
Date:
Subject: Re: [PATCH] Add native windows on arm64 support