From: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Subject: Re: PATCH: recursive json_populate_record()
To: Andres Freund <andres@anarazel.de>
References: <687e19a7-14e2-d0f5-2bff-35d62bc5c05a@postgrespro.ru>
 <934.1485111512@sss.pgh.pa.us>
 <9e21a39c-c1d7-b9b5-44a0-c5345a5029f6@postgrespro.ru>
 <13020.1485377905@sss.pgh.pa.us>
 <295ad4a5-2923-1834-5140-35d0b9a5818c@postgrespro.ru>
 <16179.1485380983@sss.pgh.pa.us>
 <CAB7nPqSGyCxh9CqPXp5TTE+A0svyWvbO1sp3BT87VtDxtT4tLQ@mail.gmail.com>
 <9bf14a7c-0794-f90c-c012-8dd0335ab364@pgmasters.net>
 <bcb6edee-f124-d5f4-3314-09b5274bd328@pgmasters.net>
 <754780af-d905-9704-cf4d-b64e632b86a3@2ndQuadrant.com>
 <20170403211741.7ommm5dssfv3abo6@alap3.anarazel.de>
 <45c0aa55-1b62-c622-31fd-cb21e017d5cd@2ndQuadrant.com>
Cc: David Steele <david@pgmasters.net>,
 Nikita Glukhov <n.gluhov@postgrespro.ru>,
 Michael Paquier <michael.paquier@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
 Aleksander Alekseev <a.alekseev@postgrespro.ru>,
 PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Message-ID: <8fcd9e65-acb0-8221-ef23-01e5c2fe90e4@2ndQuadrant.com>
Date: Wed, 5 Apr 2017 19:24:08 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.0
MIME-Version: 1.0
In-Reply-To: <45c0aa55-1b62-c622-31fd-cb21e017d5cd@2ndQuadrant.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers