Re: [PATCH] Generalized JSON output functions

2015-07-10 15:57 GMT+02:00 Shulgin, Oleksandr <oleksandr(dot)shulgin(at)zalando(dot)de>
:

> 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
>>>
>>
> I thought it was pretty obvious from the code, because it's sort of the
> only source for docs on the subject right now. Should we add proper
> documentation section, this would have been documented for sure.
>
>
>> 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, ....)
>>>
>>
> For consistency. Even though we initialize the output context ourselves,
> there might be some code introduced between json_out_init_context() and
> dst.value() calls that replaces some of the callbacks, and then there would
> be a difference.
>

with this consistency? I didn't see this style everywhere in Postgres?
Isn't it premature optimization?

>
>> 3. if it should be used everywhere, then in EXPLAIN statement too.
>>>
>>
> Ahh.. good catch. I'll have a look on this now.
>
> Thanks for the review!
>
> --
> Alex
>
>