From: Tom Lane <tgl@sss.pgh.pa.us>
To: Nikita Glukhov <n.gluhov@postgrespro.ru>
Cc: Aleksander Alekseev <a.alekseev@postgrespro.ru>
Cc: Michael Paquier <michael.paquier@gmail.com>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: PATCH: recursive json_populate_record()
Date: Wed, 25 Jan 2017 15:58:25 -0500
Message-ID: <13020.1485377905@sss.pgh.pa.us> (raw)
In-Reply-To: <9e21a39c-c1d7-b9b5-44a0-c5345a5029f6@postgrespro.ru>
References: <222c7cfb-73c8-a56e-7844-3dc51682760d@postgrespro.ru>
<CAB7nPqR_su1w5yC2KMSuH=XmmY8JHL+aDz-sJzc3Ln-juOAgtQ@mail.gmail.com>
<20161227131541.GA6976@e733.localdomain>
<1b72a62e-fd94-fd6d-c573-870af0c8600d@postgrespro.ru>
<11489.1483901523@sss.pgh.pa.us>
<687e19a7-14e2-d0f5-2bff-35d62bc5c05a@postgrespro.ru>
<934.1485111512@sss.pgh.pa.us>
<9e21a39c-c1d7-b9b5-44a0-c5345a5029f6@postgrespro.ru>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Nikita Glukhov <n.gluhov@postgrespro.ru> writes:
> On 22.01.2017 21:58, Tom Lane wrote:
>> 5. The business with having some arguments that are only for json and
>> others that are only for jsonb, eg in populate_scalar(), also makes me
>> itch.
> I've refactored json/jsonb passing using new struct JsValue which contains
> union for json/jsonb values.
I'm not too enamored of that fix. It doesn't do much for readability, and
at least with my compiler (gcc 4.4.7), I sometimes get "variable might be
used uninitialized" warnings, probably due to not all fields of the union
getting set in every code path.
>> I wonder whether this wouldn't all get a lot simpler and more
>> readable if we gave up on trying to share code between the two cases.
> Maybe two separate families of functions like this
> ...
> could slightly improve readability, but such code duplication (I believe it is
> a duplicate code) would never be acceptable to me.
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:
/* prepare column metadata cache for the given type */
if (col->typid != typid || col->typmod != typmod)
prepare_column_cache(col, typid, typmod, mcxt, jsv->is_json);
*isnull = jsv->is_json ? jsv->val.json.str == NULL
: jsv->val.jsonb == NULL ||
jsv->val.jsonb->type == jbvNull;
typcat = col->typcat;
/* try to convert json string to a non-scalar type through input function */
if ((jsv->is_json ? jsv->val.json.type == JSON_TOKEN_STRING
: jsv->val.jsonb &&
jsv->val.jsonb->type == jbvString) &&
(typcat == TYPECAT_ARRAY ||
typcat == TYPECAT_COMPOSITE))
typcat = TYPECAT_SCALAR;
/* we must perform domain checks for NULLs */
if (*isnull && typcat != TYPECAT_DOMAIN)
return (Datum) 0;
When every other line contains an is_json conditional, you are not making
good readable code. It's also worth noting that this is going to become
even less readable after pgindent gets done with it:
/* prepare column metadata cache for the given type */
if (col->typid != typid || col->typmod != typmod)
prepare_column_cache(col, typid, typmod, mcxt, jsv->is_json);
*isnull = jsv->is_json ? jsv->val.json.str == NULL
: jsv->val.jsonb == NULL ||
jsv->val.jsonb->type == jbvNull;
typcat = col->typcat;
/* try to convert json string to a non-scalar type through input function */
if ((jsv->is_json ? jsv->val.json.type == JSON_TOKEN_STRING
: jsv->val.jsonb &&
jsv->val.jsonb->type == jbvString) &&
(typcat == TYPECAT_ARRAY ||
typcat == TYPECAT_COMPOSITE))
typcat = TYPECAT_SCALAR;
/* we must perform domain checks for NULLs */
if (*isnull && typcat != TYPECAT_DOMAIN)
return (Datum) 0;
You could maybe improve that result with some different formatting
choices, but I think it's basically a readability fail to be relying
heavily on multiline x?y:z conditionals in the first place.
And I still maintain that it's entirely silly to be structuring
populate_scalar() this way.
So I really think it'd be a good idea to explore separating the json and
jsonb code paths. This direction isn't looking like a win.
>> 7. More generally, the patch is hard to review because it whacks the
>> existing code around so thoroughly that it's tough even to match up
>> old and new code to get a handle on what you changed functionally.
>> Maybe it would be good if you could separate it into a refactoring
>> patch that just moves existing code around without functional changes,
>> and then a patch on top of that that adds code and makes only localized
>> changes in what's already there.
> I've split this patch into two patches as you asked.
That didn't really help :-( ... the 0002 patch is still nigh unreadable.
Maybe it's trying to do too much in one step.
regards, tom lane
--
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: tgl@sss.pgh.pa.us, n.gluhov@postgrespro.ru, a.alekseev@postgrespro.ru, michael.paquier@gmail.com
Subject: Re: PATCH: recursive json_populate_record()
In-Reply-To: <13020.1485377905@sss.pgh.pa.us>
* 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