Re: PATCH: recursive json_populate_record() - Mailing list pgsql-hackers

On 04/04/2017 05:18 PM, Andrew Dunstan wrote:
>
> On 04/03/2017 05:17 PM, Andres Freund wrote:
>> On 2017-03-21 14:31:08 -0400, Andrew Dunstan wrote:
>>> On 03/21/2017 01:37 PM, David Steele wrote:
>>>> On 3/16/17 11:54 AM, David Steele wrote:
>>>>> On 2/1/17 12:53 AM, Michael Paquier wrote:
>>>>>> On Thu, Jan 26, 2017 at 6:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>>>>> Nikita Glukhov <n.gluhov@postgrespro.ru> writes:
>>>>>>>> On 25.01.2017 23:58, Tom Lane wrote:
>>>>>>>>> I think you need to take a second look at the code you're producing
>>>>>>>>> and realize that it's not so clean either.  This extract from
>>>>>>>>> populate_record_field, for example, is pretty horrid:
>>>>>>>> But what if we introduce some helper macros like this:
>>>>>>>> #define JsValueIsNull(jsv) \
>>>>>>>>      ((jsv)->is_json ? !(jsv)->val.json.str \
>>>>>>>>          : !(jsv)->val.jsonb || (jsv)->val.jsonb->type == jbvNull)
>>>>>>>> #define JsValueIsString(jsv) \
>>>>>>>>      ((jsv)->is_json ? (jsv)->val.json.type == JSON_TOKEN_STRING \
>>>>>>>>          : (jsv)->val.jsonb && (jsv)->val.jsonb->type == jbvString)
>>>>>>> Yeah, I was wondering about that too.  I'm not sure that you can make
>>>>>>> a reasonable set of helper macros that will fix this, but if you want
>>>>>>> to try, go for it.
>>>>>>>
>>>>>>> BTW, just as a stylistic thing, I find "a?b:c||d" unreadable: I have
>>>>>>> to go back to the manual every darn time to convince myself whether
>>>>>>> that means (a?b:c)||d or a?b:(c||d).  It's better not to rely on
>>>>>>> the reader (... or the author) having memorized C's precedence rules
>>>>>>> in quite that much detail.  Extra parens help.
>>>>>> Moved to CF 2017-03 as discussion is going on and more review is
>>>>>> needed on the last set of patches.
>>>>>>
>>>>> The current patches do not apply cleanly at cccbdde:
>>>>>
>>>>> $ git apply ../other/0001-introduce-JsonContainerXxx-macros-v04.patch
>>>>> error: patch failed: src/backend/utils/adt/jsonb_util.c:328
>>>>> error: src/backend/utils/adt/jsonb_util.c: patch does not apply
>>>>> error: patch failed: src/backend/utils/adt/jsonfuncs.c:1266
>>>>> error: src/backend/utils/adt/jsonfuncs.c: patch does not apply
>>>>> error: patch failed: src/include/utils/jsonb.h:218
>>>>> error: src/include/utils/jsonb.h: patch does not apply
>>>>>
>>>>> In addition, it appears a number of suggestions have been made by Tom
>>>>> that warrant new patches.  Marked "Waiting on Author".
>>>> This thread has been idle for months since Tom's review.
>>>>
>>>> The submission has been marked "Returned with Feedback".  Please feel
>>>> free to resubmit to a future commitfest.
>>>>
>>>>
>>> Please revive. I am planning to look at this later this week.
>> Since that was 10 days ago - you're still planning to get this in before
>> the freeze?
>>
>
> Hoping to, other demands have intervened a bit, but I might be able to.
>



OK, I have been through this and I think it's basically committable. I
am currently doing some cleanup, such as putting all the typedefs in one
place, fixing redundant comments and adding more comments, declaring all
the static functions, and minor code changes, but nothing major. I
should be ready to commit tomorrow some time.

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services