Thread: [HACKERS] Re: [BUGS] BUG #14849: jsonb_build_object doesn't like VARIADIC callsvery much

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-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers