Re: remaining sql/json patches

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

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(dot)universality(at)gmail(dot)com> wrote:
> On Mon, Jan 22, 2024 at 10:28 PM Amit Langote <amitlangote09(at)gmail(dot)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