Re: [HACKERS] PATCH: recursive json_populate_record()

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