Re: jsonpath

On 1/18/19 6:58 PM, Alexander Korotkov wrote:
> Nikita, thank you!
>
> I noticed another thing. 3-arguments version of functions
> jsonpath_exists(), jsonpath_predicate(), jsonpath_query(),
> jsonpath_query_wrapped() are:
>
> 1) Not documented
> 2) Not used in operator definitions
> 3) Not used in subsequent patches
>
> So, it looks like we can just remove then. But I think we're very
> likely can commit jsonpath patch to PostgreSQL 12, but I'm not sure
> about other patches. So, having those functions exposed to user can
> be extremely useful until we have separate nodes for SQL/JSON. So, I
> vote to document these functions.
>

That seems a bit strange. If those functions are meant to be used by
other patches (which ones?) then why should not we make them part of
those future patches?

But it seems more like those functions are actually meant to be used by
users in the first place, in cases when we need to provide a third
parameter (which operators can't do). In that case yes - we should have
them documented properly, but also tested. Which is not the case
currently, because the regression tests only use the operators.

Two more comments:

1) I'm wondering why we even need separate functions for the different
numbers of arguments at the C level, as both simply call to the same
function anyway with a PG_NARGS() condition in it. Can't we ditch those
wrappers and reference that target function directly?

2) I once again ran into the jsonb->json business, which essentially
means that when you do this:

select json '{"a": { "b" : 10}}' @? '$.a.b';

it ends up calling jsonb_jsonpath_exists(), which then does this:

Jsonb *jb = PG_GETARG_JSONB_P(0);

I and am not sure why/how it seems to work, but I find it confusing
because the stack still looks like this:

#0 jsonb_jsonpath_exists (fcinfo=0x162f800) at jsonpath_exec.c:2857
#1 0x000000000096d721 in json_jsonpath_exists2 (fcinfo=0x162f800) at
jsonpath_exec.c:2882
#2 0x00000000006c790a in ExecInterpExpr (state=0x162f300,
econtext=0x162ee18, isnull=0x7ffcea4c3857) at execExprInterp.c:648
...

and of course, the fcinfo->flinfo still points to the json-specific
function, which say the first parameter type is 'json'.

(gdb) print *fcinfo->flinfo
$23 = {fn_addr = 0x96d709 <json_jsonpath_exists2>, fn_oid = 6043,
fn_nargs = 2, fn_strict = true, fn_retset = false, fn_stats = 2 '\002',
fn_extra = 0x0, fn_mcxt = 0x162e990, fn_expr = 0x15db378}

test=# select proname, prosrc, proargtypes from pg_proc where oid = 6043;
proname | prosrc | proargtypes
-----------------+-----------------------+-------------
jsonpath_exists | json_jsonpath_exists2 | 114 6050
(1 row)

test=# select typname from pg_type where oid = 114;
typname
---------
json
(1 row)

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services