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

On Thu, Oct 19, 2017 at 2:54 AM, Andrew Dunstan
<andrew(dot)dunstan(at)2ndquadrant(dot)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