Date: Tue, 27 Dec 2016 16:15:41 +0300
From: Aleksander Alekseev <a.alekseev@postgrespro.ru>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Nikita Glukhov <n.gluhov@postgrespro.ru>,
	PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: PATCH: recursive json_populate_record()
Message-ID: <20161227131541.GA6976@e733.localdomain>
References: <222c7cfb-73c8-a56e-7844-3dc51682760d@postgrespro.ru>
 <CAB7nPqR_su1w5yC2KMSuH=XmmY8JHL+aDz-sJzc3Ln-juOAgtQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="PNTmBPCT7hxwcZjr"
Content-Disposition: inline
In-Reply-To: 
 <CAB7nPqR_su1w5yC2KMSuH=XmmY8JHL+aDz-sJzc3Ln-juOAgtQ@mail.gmail.com>
User-Agent: Mutt/1.7.2 (2016-11-26)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--PNTmBPCT7hxwcZjr
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hello.

I've noticed that this patch is on CF and needs a reviewer so I decided
to take a look. Code looks good to me in general, it's well covered by
tests and passes `make check-world`.

However it would be nice to have a little more comments. In my opinion
every procedure have to have at least a short description - what it
does, what arguments it receives and what it returns, even if it's a
static procedure. Same applies for structures and their fields.

Another thing that bothers me is a FIXME comment:

```
tupdesc =3D lookup_rowtype_tupdesc(type, typmod); /* FIXME cache */
```

I suggest remove it or implement a caching here as planned.

On Tue, Dec 13, 2016 at 10:07:46AM +0900, Michael Paquier wrote:
> On Tue, Dec 13, 2016 at 9:38 AM, Nikita Glukhov <n.gluhov@postgrespro.ru>=
 wrote:
> > It also fixes the following errors/inconsistencies caused by lost quoti=
ng of
> > string json values:
> >
> > [master]=3D# select * from json_to_record('{"js": "a"}') as rec(js json=
);
> > ERROR:  invalid input syntax for type json
> > DETAIL:  Token "a" is invalid.
> > CONTEXT:  JSON data, line 1: a
>=20
> The docs mention that this is based on a best effort now. It may be
> interesting to improve that.
>=20
> > [master]=3D# select * from json_to_record('{"js": "true"}') as rec(js j=
son);
> >   js
> > ------
> > true
> >
> > [patched]=3D# select * from json_to_record('{"js": "a"}') as rec(js jso=
n);
> >   js
> > -----
> >  "a"
>=20
> That's indeed valid JSON.
>=20
> > [patched]=3D# select * from json_to_record('{"js": "true"}') as rec(js =
json);
> >    js
> > --------
> >  "true"
>=20
> And so is that.
>=20
> > The second patch adds assignment of correct ndims to array fields of RE=
CORD
> > function result types.
> > Without this patch, attndims in tuple descriptors of RECORD types is al=
ways
> > 0 and the corresponding assertion fails in the next test:
> >
> > [patched]=3D# select json_to_record('{"a": [1, 2, 3]}') as rec(a int[]);
> >
> >
> > Should I submit these patches to commitfest?
>=20
> It seems to me that it would be a good idea to add them.
> --=20
> Michael
>=20
>=20
> --=20
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--=20
Best regards,
Aleksander Alekseev

--PNTmBPCT7hxwcZjr
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEECAokn0QZCoIHW4GpEEnCBLXCDtYFAlhiaXoACgkQEEnCBLXC
DtbSsA/+NtPU/tD/qGL/OkFVpwSJZB1MtH5lShL4wXui8lbsOiXvfVstbFRqev1c
5Rd9QH374+Z/oAgsgsD4HGmH2JNWrsu215CPVom38jTFThXgZtygBOsR3edHvhqQ
0hERIXBxXp45URT3q5iSIaSvhKaxm5xjdG+fd7UVeBeEvkOvJxEZUl6j64Cj2/0Q
pHLBBQEkFa9MZLquWPEhypPfXLvf2awykNBisZaqrNjeFIf3UICOCeaWSf5YN101
5TQHqpFui4fleR6E+tKLiVxHusjSZL+sEsfaqO6GS19n97yeXEC4QcUQMX+CXXpR
IJ0G1ktWU539g7Cz8FSaWvUlrazX4/9DxRIeccA4rj1C9KarHGq1YcH3eu4J/Us6
DgA2PjWlX8PuDZPCurPxlQiLBAIi8p4HPdZhIRetbfbPfc958bOg0CP+1mqk2y8x
5my8u4q1Lulgjm2ALXnumovd0CpS+SInmnI6c9zvk+ibSV21LdIfRQ1Uzo5+POo8
0YudzfDeQmPNR1nR9uS2HfHvzkTXwnVaIkC9VseiPVzcueez9TmYTySvXR7Y5lXk
j1XLuTmC3N8ufFPUg4sP+ItNQI37+6hGGPuGMMBQJKG8Nlj784cb8dseNQbg6RwB
GsT4OSmWQLkKaHZnxAKkE/H48y871xpXD5erRtht3qytV+9b+50=
=5HJQ
-----END PGP SIGNATURE-----

--PNTmBPCT7hxwcZjr--