From: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
To: Andres Freund <andres@anarazel.de>
Cc: David Steele <david@pgmasters.net>
Cc: Nikita Glukhov <n.gluhov@postgrespro.ru>
Cc: Michael Paquier <michael.paquier@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Aleksander Alekseev <a.alekseev@postgrespro.ru>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: PATCH: recursive json_populate_record()
Date: Wed, 5 Apr 2017 19:24:08 -0400
Message-ID: <8fcd9e65-acb0-8221-ef23-01e5c2fe90e4@2ndQuadrant.com> (raw)
In-Reply-To: <45c0aa55-1b62-c622-31fd-cb21e017d5cd@2ndQuadrant.com>
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>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: andrew.dunstan@2ndquadrant.com, andres@anarazel.de, david@pgmasters.net, n.gluhov@postgrespro.ru, michael.paquier@gmail.com, tgl@sss.pgh.pa.us, a.alekseev@postgrespro.ru
Subject: Re: PATCH: recursive json_populate_record()
In-Reply-To: <8fcd9e65-acb0-8221-ef23-01e5c2fe90e4@2ndQuadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox