Re: [PATCH] Generalized JSON output functions

forgotten attachment

Regards

Pavel

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

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