From: Tom Lane <tgl@sss.pgh.pa.us>
To: Nikita Glukhov <n.gluhov@postgrespro.ru>
cc: Aleksander Alekseev <a.alekseev@postgrespro.ru>,
        Michael Paquier <michael.paquier@gmail.com>,
        PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: PATCH: recursive json_populate_record()
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>
Comments: In-reply-to Nikita Glukhov <n.gluhov@postgrespro.ru>
	message dated "Wed, 25 Jan 2017 01:25:01 +0300"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <13019.1485377905.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 25 Jan 2017 15:58:25 -0500
Message-ID: <13020.1485377905@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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 !=3D typid || col->typmod !=3D typmod)
        prepare_column_cache(col, typid, typmod, mcxt, jsv->is_json);

    *isnull =3D jsv->is_json ? jsv->val.json.str =3D=3D NULL
                           : jsv->val.jsonb =3D=3D NULL ||
                             jsv->val.jsonb->type =3D=3D jbvNull;

    typcat =3D col->typcat;

    /* try to convert json string to a non-scalar type through input functi=
on */
    if ((jsv->is_json ? jsv->val.json.type =3D=3D JSON_TOKEN_STRING
                      : jsv->val.jsonb &&
                        jsv->val.jsonb->type =3D=3D jbvString) &&
         (typcat =3D=3D TYPECAT_ARRAY ||
          typcat =3D=3D TYPECAT_COMPOSITE))
        typcat =3D TYPECAT_SCALAR;

    /* we must perform domain checks for NULLs */
    if (*isnull && typcat !=3D 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 !=3D typid || col->typmod !=3D typmod)
        prepare_column_cache(col, typid, typmod, mcxt, jsv->is_json);

    *isnull =3D jsv->is_json ? jsv->val.json.str =3D=3D NULL
        : jsv->val.jsonb =3D=3D NULL ||
        jsv->val.jsonb->type =3D=3D jbvNull;

    typcat =3D col->typcat;

    /* try to convert json string to a non-scalar type through input functi=
on */
    if ((jsv->is_json ? jsv->val.json.type =3D=3D JSON_TOKEN_STRING
         : jsv->val.jsonb &&
         jsv->val.jsonb->type =3D=3D jbvString) &&
        (typcat =3D=3D TYPECAT_ARRAY ||
         typcat =3D=3D TYPECAT_COMPOSITE))
        typcat =3D TYPECAT_SCALAR;

    /* we must perform domain checks for NULLs */
    if (*isnull && typcat !=3D 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


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