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

On Tue, Jan 23, 2024 at 10:46 PM Amit Langote <amitlangote09@gmail.com> wrote:
> 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.

Pushed 0001-0003 for now.

Rebased patches attached.  I merged 0004 into the query functions
patch after all.

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

See below...

On Tue, Jan 23, 2024 at 12:46 AM jian he <jian.universality@gmail.com> wrote:
> On Mon, Jan 22, 2024 at 10:28 PM Amit Langote <amitlangote09@gmail.com> wrote:
> >
> > > based on v35.
> > > Now I only applied from 0001 to 0007.
> > > For {DEFAULT expression  ON EMPTY}  | {DEFAULT expression ON ERROR}
> > > restrict DEFAULT expression be either Const node or FuncExpr node.
> > > so these 3 SQL/JSON functions can be used in the btree expression index.
> >
> > I'm not really excited about adding these restrictions into the
> > transformJsonFuncExpr() path.  Index or any other code that wants to
> > put restrictions already have those in place, no need to add them
> > here.  Moreover, by adding these restrictions, we might end up
> > preventing users from doing useful things with this like specify
> > column references.  If there are semantic issues with allowing that,
> > we should discuss them.
> >
>
> after applying v36.
> The following index creation and query operation works. I am not 100%
> sure about these cases.
> just want confirmation, sorry for bothering you....

No worries; I really appreciate your testing and suggestions.

> drop table t;
> create table t(a jsonb, b  int);
> insert into t select '{"hello":11}',1;
> insert into t select '{"hello":12}',2;
> CREATE INDEX t_idx2 ON t (JSON_query(a, '$.hello1' RETURNING int
> default b + random() on error));
> CREATE INDEX t_idx3 ON t (JSON_query(a, '$.hello1' RETURNING int
> default random()::int on error));
> create or replace function ret_setint() returns setof integer as
> $$
> begin
>     -- perform pg_sleep(0.1);
>     return query execute 'select 1 union all select 1';
> end;
> $$
> language plpgsql IMMUTABLE;
> SELECT JSON_query(a, '$.hello1'  RETURNING int default ret_setint() on
> error) from t;
> SELECT JSON_query(a, '$.hello1'  RETURNING int default sum(b) over()
> on error) from t;
> SELECT JSON_query(a, '$.hello1'  RETURNING int default sum(b) on
> error) from t group by a;
>
> but the following cases will fail related to index and default expression.
> create table zz(a int, b int);
> CREATE INDEX zz_idx1 ON zz ( (b + random()::int));
> create table ssss(a int, b int default ret_setint());
> create table ssss(a int, b int default sum(b) over());

I think your suggestion to add restrictions on what is allowed for
DEFAULT makes sense.  Also, immutability shouldn't be checked in
transformJsonBehavior(), but in contain_mutable_functions() as done in
the attached.  Added some tests too.

I still need to take a look at your other report regarding typmod but
I'm out of energy today.

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