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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Marko Tiikkaja-4
The following bug has been logged on the website:

Bug reference:      14849
Logged by:          Marko Tiikkaja
Email address:      [hidden email]
PostgreSQL version: 10.0
Operating system:   Linux
Description:        

Hi,

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?


--
Sent via pgsql-bugs mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier
On Wed, Oct 11, 2017 at 11:00 AM,  <[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Reply | Threaded
Open this post in threaded view
|

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

Andrew Dunstan-8


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

> On Wed, Oct 11, 2017 at 11:00 AM,  <[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier
On Thu, Oct 12, 2017 at 9:38 PM, Andrew Dunstan
<[hidden email]> 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


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

json_variadic_v1.patch (34K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dmitry Dolgov
> On 13 October 2017 at 06:29, Michael Paquier <[hidden email]> 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.
```
Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier
On Fri, Oct 13, 2017 at 9:46 PM, Dmitry Dolgov <[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier
On Fri, Oct 13, 2017 at 10:15 PM, Michael Paquier
<[hidden email]> wrote:
> On Fri, Oct 13, 2017 at 9:46 PM, Dmitry Dolgov <[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Reply | Threaded
Open this post in threaded view
|

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

Dmitry Dolgov
> On 13 October 2017 at 15:48, Michael Paquier <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier
On Fri, Oct 13, 2017 at 10:56 PM, Dmitry Dolgov <[hidden email]> wrote:

>> On 13 October 2017 at 15:48, Michael Paquier <[hidden email]>
>> 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


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

json_variadic_v2.patch (36K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dmitry Dolgov
> On 14 October 2017 at 12:46, Michael Paquier <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier
On Mon, Oct 16, 2017 at 4:41 AM, Dmitry Dolgov <[hidden email]> wrote:

>> On 14 October 2017 at 12:46, Michael Paquier <[hidden email]>
>> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Reply | Threaded
Open this post in threaded view
|

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

Andrew Dunstan-8


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

> On Mon, Oct 16, 2017 at 4:41 AM, Dmitry Dolgov <[hidden email]> wrote:
>>> On 14 October 2017 at 12:46, Michael Paquier <[hidden email]>
>>> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Reply | Threaded
Open this post in threaded view
|

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

Dmitry Dolgov
In reply to this post by Michael Paquier
> On 16 October 2017 at 02:08, Michael Paquier <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

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

Andrew Dunstan-8


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

> > On 16 October 2017 at 02:08, Michael Paquier
> <[hidden email] <mailto:[hidden email]>> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier
On Thu, Oct 19, 2017 at 2:54 AM, Andrew Dunstan
<[hidden email]> 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


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

json_variadic_v3.patch (27K) Download Attachment
Previous Thread Next Thread