Re: [PATCH] Generalized JSON output functions

Hi

I am sending review of this patch:

1. I reread a previous discussion and almost all are for this patch (me too)

2. I have to fix a typo in hstore_io.c function (update attached), other
(patching, regress tests) without problems

My objections:

1. comments - missing comment for some basic API, basic fields like
"key_scalar" and similar
2. why you did indirect call via JsonOutContext?

What is benefit

dst.value(&dst, (Datum) 0, JSONTYPE_NULL, InvalidOid, InvalidOid, false);

instead

json_out_value(&dst, ....)

? Is it necessary?

3. if it should be used everywhere, then in EXPLAIN statement too.

Regards

Pavel

2015-07-10 6:31 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 2015-07-03 12:27 GMT+02:00 Heikki Linnakangas <hlinnaka(at)iki(dot)fi>:
>
>> On 05/27/2015 09:51 PM, Andrew Dunstan wrote:
>>
>>>
>>> On 05/27/2015 02:37 PM, Robert Haas wrote:
>>>
>>>> On Tue, May 26, 2015 at 2:50 AM, Shulgin, Oleksandr
>>>> <oleksandr(dot)shulgin(at)zalando(dot)de> wrote:
>>>>
>>>>> Is it reasonable to add this patch to CommitFest now?
>>>>>
>>>> It's always reasonable to add a patch to the CommitFest if you would
>>>> like for it to be reviewed and avoid having it get forgotten about.
>>>> There seems to be some disagreement about whether we want this, but
>>>> don't let that stop you from adding it to the next CommitFest.
>>>>
>>>
>>> I'm not dead set against it either. When I have time I will take a
>>> closer look.
>>>
>>
>> Andrew, will you have the time to review this? Please add yourself as
>> reviewer in the commitfest app if you do.
>>
>> My 2 cents is that I agree with your initial reaction: This is a lot of
>> infrastructure and generalizing things, for little benefit. Let's change
>> the current code where we generate JSON to be consistent with whitespace,
>> and call it a day.
>>
>
> I am thinking so it is not bad idea. This code can enforce uniform
> format, and it can check if produced value is correct. It can be used in
> our code, it can be used by extension's developers.
>
> This patch is not small, but really new lines are not too much.
>
> I'll do review today.
>
> Regards
>
> Pavel
>
>
>
>
>> - Heikki
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>
>