Thread: [BUGS] BUG #14849: jsonb_build_object doesn't like VARIADIC calls very much

The following bug has been logged on the website:

Bug reference:      14849
Logged by:          Marko Tiikkaja
Email address:      marko@joh.to
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
keyvalue 
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 (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

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

-- 
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 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 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

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

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 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

-- 
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/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

-- 
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/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

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

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

On Sun, Oct 22, 2017 at 12:43 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.

More or less identical, the jsonb sibling enforces arguments of type
unknown to text. I was considering first a common place where to put
this stuff and thought about fmgr.c, but this has no dependency on
FunctionCallInfo. I didn't think about this one, still do you think
that's adapted? extract_variadic_args is quite json-ish with its text
enforcement...
-- 
Michael


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

Michael Paquier <michael.paquier@gmail.com> writes:
> On Sun, Oct 22, 2017 at 12:43 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.

> More or less identical, the jsonb sibling enforces arguments of type
> unknown to text. I was considering first a common place where to put
> this stuff and thought about fmgr.c, but this has no dependency on
> FunctionCallInfo. I didn't think about this one, still do you think
> that's adapted? extract_variadic_args is quite json-ish with its text
> enforcement...

I don't think collecting all the arguments is particularly special ---
format() or concat() for instance could possibly use this.  You might
need an option to say what to do with unknown.
        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

On Sun, Oct 22, 2017 at 1:43 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I don't think collecting all the arguments is particularly special ---
> format() or concat() for instance could possibly use this.  You might
> need an option to say what to do with unknown.

In this case, we could just use a boolean flag to decide if TEXTOID
should be enforced or not.
-- 
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/21/2017 11:43 AM, Tom Lane wrote:
> 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.
>
>             



I guess it's not a problem exposing a new function in funcapi.h in the
stable branches, since this fix needs to be backpatched to 9.4.

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 10/21/2017 07:33 PM, Michael Paquier wrote:
> On Sun, Oct 22, 2017 at 1:43 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't think collecting all the arguments is particularly special ---
>> format() or concat() for instance could possibly use this.  You might
>> need an option to say what to do with unknown.
> In this case, we could just use a boolean flag to decide if TEXTOID
> should be enforced or not.

A generic function is going to look a little more complicated than this,
though. The functions as coded assume that the function has a single
variadic argument. But for it to be useful generically it really needs
to be able to work where there are both fixed and variadic arguments (a
la printf style functions).

I guess a simple way would be to make the caller tell the function where
the variadic arguments start, or how many to skip, something like that.

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 10/22/2017 12:11 PM, Andrew Dunstan wrote:
>
> On 10/21/2017 07:33 PM, Michael Paquier wrote:
>> On Sun, Oct 22, 2017 at 1:43 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I don't think collecting all the arguments is particularly special ---
>>> format() or concat() for instance could possibly use this.  You might
>>> need an option to say what to do with unknown.
>> In this case, we could just use a boolean flag to decide if TEXTOID
>> should be enforced or not.
> A generic function is going to look a little more complicated than this,
> though. The functions as coded assume that the function has a single
> variadic argument. But for it to be useful generically it really needs
> to be able to work where there are both fixed and variadic arguments (a
> la printf style functions).
>
> I guess a simple way would be to make the caller tell the function where
> the variadic arguments start, or how many to skip, something like that.
>


here's a patch that works that way, based on Michael's code.

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 Mon, Oct 23, 2017 at 1:44 AM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:
>
>
> On 10/22/2017 12:11 PM, Andrew Dunstan wrote:
>>
>> On 10/21/2017 07:33 PM, Michael Paquier wrote:
>>> On Sun, Oct 22, 2017 at 1:43 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> I don't think collecting all the arguments is particularly special ---
>>>> format() or concat() for instance could possibly use this.  You might
>>>> need an option to say what to do with unknown.
>>> In this case, we could just use a boolean flag to decide if TEXTOID
>>> should be enforced or not.
>> A generic function is going to look a little more complicated than this,
>> though. The functions as coded assume that the function has a single
>> variadic argument. But for it to be useful generically it really needs
>> to be able to work where there are both fixed and variadic arguments (a
>> la printf style functions).
>>
>> I guess a simple way would be to make the caller tell the function where
>> the variadic arguments start, or how many to skip, something like that.

Sorry for the late reply, I was taking a long flight. Yes, that's
actually the conclusion I am reaching when looking at the code by
adding an argument to mark when the variadic arguments start. The
headers of funcapi.h and funcapi.c need also a cleanup.

> here's a patch that works that way, based on Michael's code.

Patch not attached :)
I still have a patch half-cooked, that I can send if necessary, but
you are on it, right?
-- 
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/22/2017 04:35 PM, Michael Paquier wrote:
> On Mon, Oct 23, 2017 at 1:44 AM, Andrew Dunstan
> <andrew.dunstan@2ndquadrant.com> wrote:
>
>> here's a patch that works that way, based on Michael's code.
> Patch not attached :)
> I still have a patch half-cooked, that I can send if necessary, but
> you are on it, right?


Sorry. Here it is.

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> Sorry. Here it is.

This comment is neither correct nor intelligible:
/* important for value, key cannot being NULL */

I'd say just drop it.

The checks for "could not determine data type" errors seem
rather duplicative, too.

<compulsive-neatnik-ism>
The extern declaration seems to have been dropped in a rather
random place in the .h file.
</compulsive-neatnik-ism>

Looks good otherwise.
        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

On Mon, Oct 23, 2017 at 6:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> This comment is neither correct nor intelligible:
>
>         /* important for value, key cannot being NULL */
>
> I'd say just drop it.

Yep.

> The checks for "could not determine data type" errors seem
> rather duplicative, too.

Yep.

> <compulsive-neatnik-ism>
> The extern declaration seems to have been dropped in a rather
> random place in the .h file.
> </compulsive-neatnik-ism>

funcapi.h/c are nicely documented. I think that more is needed.

> Looks good otherwise.

My set of diffs for funcapi.h are actually that: * funcapi.h *   Definitions for functions which return composite type
and/orsets
 
+ *   or work on VARIADIC inputs.
[...]
+/*----------
+ * Support to ease writing of functions dealing with VARIADIC inputs
+ *----------
+ *
+ * This function extracts a set of argument values, types and NULL markers
+ * for a given input function. This returns a set of data:
+ * - **values includes the set of Datum values extracted.
+ * - **types the data type OID for each element.
+ * - **nulls tracks if an element is NULL.
+ *
+ * convert_unknown set to true enforces conversion of unknown input type
+ * unknown to text.
+ * variadic_start tracks the argument number of the function call where the
+ * VARIADIC set of arguments begins.
+ *
+ * The return result is the number of elements stored. In the event of a
+ * NULL input, then the caller of this function ought to generate a NULL
+ * object as final result, and in this case a result value of -1 is used
+ * to be able to make the difference between an empty array or object.
+ */
+extern int extract_variadic_args(FunctionCallInfo fcinfo, int variadic_start,
+                                bool convert_unknown, Datum **values,
Oid **types,
+                                bool **nulls);

Got this bit as well: * funcapi.c *   Utility and convenience functions for fmgr functions that return
- *   sets and/or composite types.
+ *   sets and/or composite types, or deal with VARIADIC inputs. *
-- 
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 Mon, Oct 23, 2017 at 7:03 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
>> Looks good otherwise.
>
> My set of diffs for funcapi.h are actually that:
>   * funcapi.h
>   *   Definitions for functions which return composite type and/or sets
> + *   or work on VARIADIC inputs.
> [...]
> +/*----------
> + * Support to ease writing of functions dealing with VARIADIC inputs
> + *----------
> + *
> + * This function extracts a set of argument values, types and NULL markers
> + * for a given input function. This returns a set of data:
> + * - **values includes the set of Datum values extracted.
> + * - **types the data type OID for each element.
> + * - **nulls tracks if an element is NULL.
> + *
> + * convert_unknown set to true enforces conversion of unknown input type
> + * unknown to text.
> + * variadic_start tracks the argument number of the function call where the
> + * VARIADIC set of arguments begins.
> + *
> + * The return result is the number of elements stored. In the event of a
> + * NULL input, then the caller of this function ought to generate a NULL
> + * object as final result, and in this case a result value of -1 is used
> + * to be able to make the difference between an empty array or object.
> + */
> +extern int extract_variadic_args(FunctionCallInfo fcinfo, int variadic_start,
> +                                bool convert_unknown, Datum **values,
> Oid **types,
> +                                bool **nulls);
>
> Got this bit as well:
>   * funcapi.c
>   *   Utility and convenience functions for fmgr functions that return
> - *   sets and/or composite types.
> + *   sets and/or composite types, or deal with VARIADIC inputs.
>   *

Okay, attached is what I think a fully implemented patch should look
like. On top of what Andrew has done, I added and reworked the
following:
- removed duplicate error handling.
- documented the function in funcapi.h and funcapi.c.
- Added a new section in funcapi.h to outline that this is for support
of VARIADIC inputs.
I have added a commit message as well. Hope this helps.

format, concat and potentially count_nulls could take advantage of
this new function, though refactoring is left for later. I am fine to
do the legwork on a different thread. Changing count_nulls would also
switch it to a o(n^2),  which is not cool either, so I think that it
could be left out. Still let's discuss that on another thread.
-- 
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 Mon, Oct 23, 2017 at 6:50 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Okay, attached is what I think a fully implemented patch should look
> like. On top of what Andrew has done, I added and reworked the
> following:
> - removed duplicate error handling.
> - documented the function in funcapi.h and funcapi.c.
> - Added a new section in funcapi.h to outline that this is for support
> of VARIADIC inputs.
> I have added a commit message as well. Hope this helps.

For the sake of the archives, the introduction of
extract_variadic_args is committed with f3c6e8a2, and the JSON fixes
with 18fc4ecf. Thanks Andrew for the commit, and thanks Tom, Andrew
and Dmitry for the reviews.
-- 
Michael


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