Nikita Glukhov <n.glu...@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
