BUG #14849: jsonb_build_object doesn't like VARIADIC calls very much

On Wed, Oct 11, 2017 at 11:00 AM, <marko@joh.to> wrote:

This query fails with an unreasonable error message:

=# SELECT jsonb_build_object(VARIADIC '{a,b}'::text[]);
ERROR: invalid number of arguments: object must be matched key value
pairs

jsonb_object(text[]) can be used instead in this case, so perhaps
jsonb_build_object() could simply point to that one if a variadic call like
this is made?

It looks like a good idea to do so for this code. It seems that nobody
has actually bothered testing those functions in more fancy ways than
the documentation shows... And I think that this is not the only
problem.

I looked as well at jsonb_build_array(), which also uses VARIADIC ANY,
being surprised by that:
=# SELECT jsonb_build_array(variadic '{a,b}'::text[]);
jsonb_build_array
-------------------
[["a", "b"]]
(1 row)
But it seems to me that when a variadic call is used, then ["a", "b"]
is the correct result, no?

The json_* equivalent functions are reacting similarly than the jsonb_* ones.

It is actually possible to make the difference between a variadic and
a non-variadic call by looking at funcvariadic in
fcinfo->flinfo->fn_expr. So my suggestion of a fix would be the
following:
- refactor jsonb_object with an _internal routine that gets called as
well for variadic calls of jsonb_build_object. Something equivalent
needs to be done for the json functions.
- for json[b]_build_array, let's check if the input is a variadic
call, then fill in an intermediate structure with all the array
values, which is used with the existing processing.

More regression tests are needed as well. Andrew, you worked on most
of those items, including 7e354ab9, what is your opinion on the
matter?
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On 10/12/2017 12:42 AM, Michael Paquier wrote:

On Wed, Oct 11, 2017 at 11:00 AM, <marko@joh.to> wrote:

This query fails with an unreasonable error message:

=# SELECT jsonb_build_object(VARIADIC '{a,b}'::text[]);
ERROR: invalid number of arguments: object must be matched key value
pairs

jsonb_object(text[]) can be used instead in this case, so perhaps
jsonb_build_object() could simply point to that one if a variadic call like
this is made?

It looks like a good idea to do so for this code. It seems that nobody
has actually bothered testing those functions in more fancy ways than
the documentation shows... And I think that this is not the only
problem.

I looked as well at jsonb_build_array(), which also uses VARIADIC ANY,
being surprised by that:
=# SELECT jsonb_build_array(variadic '{a,b}'::text[]);
jsonb_build_array
-------------------
[["a", "b"]]
(1 row)
But it seems to me that when a variadic call is used, then ["a", "b"]
is the correct result, no?

The json_* equivalent functions are reacting similarly than the jsonb_* ones.

It is actually possible to make the difference between a variadic and
a non-variadic call by looking at funcvariadic in
fcinfo->flinfo->fn_expr. So my suggestion of a fix would be the
following:
- refactor jsonb_object with an _internal routine that gets called as
well for variadic calls of jsonb_build_object. Something equivalent
needs to be done for the json functions.
- for json[b]_build_array, let's check if the input is a variadic
call, then fill in an intermediate structure with all the array
values, which is used with the existing processing.

More regression tests are needed as well. Andrew, you worked on most
of those items, including 7e354ab9, what is your opinion on the
matter?

If the case of an explicit VARIADIC parameter doesn't work the same way
then certainly it's a bug which needs to be fixed, and for which I
apologise. If you have time to produce a patch I will review it. Your
plan sounds good.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Thu, Oct 12, 2017 at 9:38 PM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

If the case of an explicit VARIADIC parameter doesn't work the same way
then certainly it's a bug which needs to be fixed, and for which I
apologise. If you have time to produce a patch I will review it. Your
plan sounds good.

Okay, thanks for your input.

Looking at fmgr.c, I found about get_fn_expr_variadic which is already
doing all the legwork to detect if a call is variadic or not. Also,
after looking at the code I think that using directly
jsonb_object(text[]) is incorrect. If the caller provides for example
int[] as input data, I think that we ought to allow the result to be
casted as a JSON integer. So I have implemented a patch that fills in
intermediate state data when dong a variadic call and feeds that to
the JSONB constructor.

An interesting side-effect of this patch is that:
=# SELECT jsonb_build_object(VARIADIC '{{1,4},{2,5},{3,6}}'::int[][]);
jsonb_build_object
--------------------------
{"1": 4, "2": 5, "3": 6}
(1 row)
This makes actually things more consistent with json_object(), which
generates the same result:
=# SELECT jsonb_object('{{1,4},{2,5},{3,6}}'::text[]);
jsonb_object
--------------------------------
{"1": "4", "2": "5", "3": "6"}
(1 row)
But jsonb_build_object complains for non-variadic calls:
=# SELECT jsonb_build_object('{1,2}'::int[],'{3,4}'::int[]);
ERROR: 22023: key value must be scalar, not array, composite, or json
LOCATION: datum_to_jsonb, jsonb.c:725
So I tend to think that the patch attached is correct, and that we
ought to not complain back to the user if NDIMS > 1 when doing
variadic calls.

More regression tests are added for json[b]_build_object and
json[b]_build_array to validate all that. When deconstructing the
variadic array, there are similarities between the json and jsonb code
but data type evaluate particularly for UNKNOWNOID is different
between both, so things are better not refactoring into a common
routine IMO. Each _array and _object code path also has slight
differences, and it felt more natural to me to not refactor json.c and
jsonb.c to hold a common routine.

In short, I have nailed things down with the attached patch. What do
you guys think?
--
Michael

On 13 October 2017 at 06:29, Michael Paquier <michael.paquier@gmail.com>

wrote:

So I have implemented a patch that fills in intermediate state data when

dong

a variadic call and feeds that to the JSONB constructor.

Shouldn't `jsonb_build_object` and `jsonb_build_array` be strict in this
patch?
Otherwise one can get a segfault for these cases:

```
=# select jsonb_build_object(variadic NULL::text[]);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

=# select jsonb_build_array(variadic NULL::text[]);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
```

On Fri, Oct 13, 2017 at 9:46 PM, Dmitry Dolgov <9erthalion6@gmail.com> wrote:

Shouldn't `jsonb_build_object` and `jsonb_build_array` be strict in this
patch?

Oh, right. I thought that those were marked as strict already. Their
type cannot be changed in back-branches, but can be on HEAD. While I
agree that build_object should return NULL in this case, what about
build_array? Should it return [NULL] (empty array), or just NULL? I
would think the latter but this can be debated.
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Fri, Oct 13, 2017 at 10:15 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Fri, Oct 13, 2017 at 9:46 PM, Dmitry Dolgov <9erthalion6@gmail.com> wrote:

Shouldn't `jsonb_build_object` and `jsonb_build_array` be strict in this
patch?

Oh, right. I thought that those were marked as strict already. Their
type cannot be changed in back-branches, but can be on HEAD. While I
agree that build_object should return NULL in this case, what about
build_array? Should it return [NULL] (empty array), or just NULL? I
would think the latter but this can be debated.

I take that back. STRICT means that the result would be NULL if any of
the argument is NULL, so your argument makes no real sense as it is
perfectly valid to have something like json_build_array('2', NULL) or
json_build_object('1', NULL), because there can be NULL values in JSON
strings. What we just need to be careful here is to check if the array
is NULL or not in a variadic call, and just return NULL in this case.
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On 13 October 2017 at 15:48, Michael Paquier <michael.paquier@gmail.com>

wrote:

it is perfectly valid to have something like json_build_array('2', NULL)

or

json_build_object('1', NULL), because there can be NULL values in JSON
strings. What we just need to be careful here is to check if the array
is NULL or not in a variadic call, and just return NULL in this case.

Oh, indeed, I agree.

On Fri, Oct 13, 2017 at 10:56 PM, Dmitry Dolgov <9erthalion6@gmail.com> wrote:

On 13 October 2017 at 15:48, Michael Paquier <michael.paquier@gmail.com>
wrote:

it is perfectly valid to have something like json_build_array('2', NULL)
or
json_build_object('1', NULL), because there can be NULL values in JSON
strings. What we just need to be careful here is to check if the array
is NULL or not in a variadic call, and just return NULL in this case.

Oh, indeed, I agree.

Okay. Attached is an updated patch fixing what you have reported. Now
things behave as I would expect the way they should:
=# select json_build_array(variadic NULL::text[]);
json_build_array
------------------
null
(1 row)
=# select json_build_array(variadic '{}'::text[]);
json_build_array
------------------
[]
(1 row)
=# select json_build_object(variadic '{}'::text[]);
json_build_object
-------------------
{}
(1 row)
=# select json_build_object(variadic NULL::text[]);
json_build_object
-------------------
null
(1 row)

Thanks Dmitry for the review.
--
Michael

On 14 October 2017 at 12:46, Michael Paquier <michael.paquier@gmail.com>

wrote:

Okay. Attached is an updated patch fixing what you have reported.

Thanks for the patch. Everything looks fine, I just have one question -
maybe
it makes sense to put this pattern `if (variadic) {/*...*/}` into a separate
function? Because from what I see it's basically one part of code (I didn't
spot any difference) repeated four times for every function.

On Mon, Oct 16, 2017 at 4:41 AM, Dmitry Dolgov <9erthalion6@gmail.com> wrote:

On 14 October 2017 at 12:46, Michael Paquier <michael.paquier@gmail.com>
wrote:

Okay. Attached is an updated patch fixing what you have reported.

Thanks for the patch. Everything looks fine, I just have one question -
maybe
it makes sense to put this pattern `if (variadic) {/*...*/}` into a separate
function? Because from what I see it's basically one part of code (I didn't
spot any difference) repeated four times for every function.

The json/jsonb calls have one difference though. For jsonb, arguments
with unknown type are enforced to text, which is not the case of json,
and we don't want to change that behavior. Both the json and jsonb
calls also use different utility routines which are proper to json.c
and jsonb.c, so moving all things to jsonfuncs.c is out of the game
(see add_jsonb and add_json). There is a common place for JSON-common
code in jsonapi.c, but jsonapi.h is wanted as light-weight (see its
set of headers), so moving things there is incorrect as well IMO.

One thing that could be done though is to have two routines which
prepare the arguments for the array and object build, one for each
file json.c and jsonb.c. The routine could return the number of
arguments processed, at the exception that if the routine returns -1,
then the result for the object generated should be NULL, which
corresponds to the case where a NULL array is given in a variadic
call.

What do you think?
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On 10/15/2017 08:08 PM, Michael Paquier wrote:

On Mon, Oct 16, 2017 at 4:41 AM, Dmitry Dolgov <9erthalion6@gmail.com> wrote:

On 14 October 2017 at 12:46, Michael Paquier <michael.paquier@gmail.com>
wrote:

Okay. Attached is an updated patch fixing what you have reported.

Thanks for the patch. Everything looks fine, I just have one question -
maybe
it makes sense to put this pattern `if (variadic) {/*...*/}` into a separate
function? Because from what I see it's basically one part of code (I didn't
spot any difference) repeated four times for every function.

The json/jsonb calls have one difference though. For jsonb, arguments
with unknown type are enforced to text, which is not the case of json,
and we don't want to change that behavior. Both the json and jsonb
calls also use different utility routines which are proper to json.c
and jsonb.c, so moving all things to jsonfuncs.c is out of the game
(see add_jsonb and add_json). There is a common place for JSON-common
code in jsonapi.c, but jsonapi.h is wanted as light-weight (see its
set of headers), so moving things there is incorrect as well IMO.

One thing that could be done though is to have two routines which
prepare the arguments for the array and object build, one for each
file json.c and jsonb.c. The routine could return the number of
arguments processed, at the exception that if the routine returns -1,
then the result for the object generated should be NULL, which
corresponds to the case where a NULL array is given in a variadic
call.

What do you think?

That seems a reasonable approach.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On 16 October 2017 at 02:08, Michael Paquier <michael.paquier@gmail.com>

wrote:

Thanks for the patch. Everything looks fine, I just have one question -
maybe it makes sense to put this pattern `if (variadic) {/*...*/}` into a
separate function? Because from what I see it's basically one part of

code

(I didn't spot any difference) repeated four times for every function.

The json/jsonb calls have one difference though. For jsonb, arguments
with unknown type are enforced to text, which is not the case of json,
and we don't want to change that behavior.

Oh, actually what I meant is to put into a separate function only the first
branch of this condition, i.e. when `variadic` is true. But in general I
don't
really have strong opinion about that, so if you think that this repetition
is
not a problem, then fine.

There is a common place for JSON-common code in jsonapi.c, but jsonapi.h

is

wanted as light-weight (see its set of headers), so moving things there is
incorrect as well IMO.

Hm...did you mean `jsonfuncs.c` and `jsonapi.h`? It seems to me, that this
potential function (to handle `variadic` case) would not really pollute it.

On 10/18/2017 11:47 AM, Dmitry Dolgov wrote:

On 16 October 2017 at 02:08, Michael Paquier

<michael.paquier@gmail.com <mailto:michael.paquier@gmail.com>> wrote:

Thanks for the patch. Everything looks fine, I just have one question -
maybe it makes sense to put this pattern `if (variadic) {/*...*/}`

into a

separate function? Because from what I see it's basically one part

of code

(I didn't spot any difference) repeated four times for every function.

The json/jsonb calls have one difference though. For jsonb, arguments
with unknown type are enforced to text, which is not the case of json,
and we don't want to change that behavior.

Oh, actually what I meant is to put into a separate function only the
first
branch of this condition, i.e. when `variadic` is true. But in general
I don't
really have strong opinion about that, so if you think that this
repetition is
not a problem, then fine.

There is a common place for JSON-common code in jsonapi.c, but

jsonapi.h is

wanted as light-weight (see its set of headers), so moving things

there is

incorrect as well IMO.

Hm...did you mean `jsonfuncs.c` and `jsonapi.h`? It seems to me, that this
potential function (to handle `variadic` case) would not really
pollute it.

If we really wanted to we could have it as a parameter to the function
to do the special UNKONOWNOID handling or not.

But I'm not sure it's worth it, especially on the back branches where we
should try to do minimal code disturbance.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Thu, Oct 19, 2017 at 2:54 AM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

On 10/18/2017 11:47 AM, Dmitry Dolgov wrote:

On 16 October 2017 at 02:08, Michael Paquier

Hm...did you mean `jsonfuncs.c` and `jsonapi.h`? It seems to me, that this
potential function (to handle `variadic` case) would not really
pollute it.

I think it is not a good idea to make jsonapi.h depend on anything
higher-level than what is now in that. And adding this routine would
require including fmgr.h. We cannot move everything to jsonfuncs.c as
well per the dependencies to json_add and jsonb_add for each build
routine. Or we could move everything but that would be really
disturbing for back-patching.

If we really wanted to we could have it as a parameter to the function
to do the special UNKNOWNOID handling or not.

If you are fine with more stuff included in jsonapi.h, that's doable
then, but I don't recommend it. As you are the original author of this
code after all, I will rely on your opinion, so shaping it the way you
see suited better will be fine for me. We could also create a new
header like jsoncommon.h which includes more high-level code. Still
that would be unsuited for back-branches.

But I'm not sure it's worth it, especially on the back branches where we
should try to do minimal code disturbance.

Agreed. So for both HEAD and back-branches, my current recommendation
is just to have two functions, one in json.c and jsonb.c, which are in
charge of extracting the argument values, types and NULL-ness flags to
minimize the code duplication. And this also does not cause any huge
disturbing refactoring.

    if (nargs % 2 != 0)
        ereport(ERROR,
                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                errmsg("invalid number of arguments: object must be
matched key value pairs")));
+                errmsg("argument list must have even number of elements"),
+                errhint("The arguments of jsonb_build_object() must
consist of alternating keys and values.")));
I have noticed as well that the error message for jsonb_build_object
is for an uneven number of arguments is inconsistent with its json-ish
cousin. So I reworded things in a more consistent way.

Please see the attached patch which considers all the discussion done,
which shapes the code in the way I see most suited for HEAD and
back-branches.
--
Michael

On 10/18/2017 10:07 PM, Michael Paquier wrote:

Please see the attached patch which considers all the discussion done,
which shapes the code in the way I see most suited for HEAD and
back-branches.

Patch doesn't apply against HEAD.

Also, don't we need more tests?

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Thu, Oct 19, 2017 at 10:13 PM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

On 10/18/2017 10:07 PM, Michael Paquier wrote:

Please see the attached patch which considers all the discussion done,
which shapes the code in the way I see most suited for HEAD and
back-branches.

Patch doesn't apply against HEAD.

Also, don't we need more tests?

Oops. Sorry about that. v3 is incorrect, I messed up the origin commit
for the diffs generated. Here you go as attached.
--
Michael

On 10/19/2017 11:25 AM, Michael Paquier wrote:

On Thu, Oct 19, 2017 at 10:13 PM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

On 10/18/2017 10:07 PM, Michael Paquier wrote:

Please see the attached patch which considers all the discussion done,
which shapes the code in the way I see most suited for HEAD and
back-branches.

Patch doesn't apply against HEAD.

Also, don't we need more tests?

Oops. Sorry about that. v3 is incorrect, I messed up the origin commit
for the diffs generated. Here you go as attached.

Looks good. Do you want to produce patches for the back branches also?
this applies to REL_10 but not earlier. If not I'll work on it this weekend.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Fri, Oct 20, 2017 at 10:49 PM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

On 10/19/2017 11:25 AM, Michael Paquier wrote:

On Thu, Oct 19, 2017 at 10:13 PM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

On 10/18/2017 10:07 PM, Michael Paquier wrote:

Please see the attached patch which considers all the discussion done,
which shapes the code in the way I see most suited for HEAD and
back-branches.

Patch doesn't apply against HEAD.

Also, don't we need more tests?

Oops. Sorry about that. v3 is incorrect, I messed up the origin commit
for the diffs generated. Here you go as attached.

Looks good. Do you want to produce patches for the back branches also?
this applies to REL_10 but not earlier. If not I'll work on it this weekend.

Here you go, down to 9.4 where things apply. The same patch can be
applied for HEAD/10, 9.5/9.6 and 9.4. The 9.4 version just needs to
have the portion for json as the jsonb functions have been added in
9.5, and the json functions are new as of 9.4.
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:

Here you go, down to 9.4 where things apply. The same patch can be
applied for HEAD/10, 9.5/9.6 and 9.4. The 9.4 version just needs to
have the portion for json as the jsonb functions have been added in
9.5, and the json functions are new as of 9.4.

It doesn't seem very nice to have two identical(?) copies of
extract_variadic_args, especially since the same functionality
might be of use elsewhere. I'm almost inclined to say that that
should go in funcapi.c/.h.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs