From: Aleksander Alekseev <a.alekseev@postgrespro.ru>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Nikita Glukhov <n.gluhov@postgrespro.ru>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: PATCH: recursive json_populate_record()
Date: Tue, 27 Dec 2016 16:15:41 +0300
Message-ID: <20161227131541.GA6976@e733.localdomain> (raw)
In-Reply-To: <CAB7nPqR_su1w5yC2KMSuH=XmmY8JHL+aDz-sJzc3Ln-juOAgtQ@mail.gmail.com>
References: <222c7cfb-73c8-a56e-7844-3dc51682760d@postgrespro.ru>
<CAB7nPqR_su1w5yC2KMSuH=XmmY8JHL+aDz-sJzc3Ln-juOAgtQ@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
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 = 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 quoting of
> > string json values:
> >
> > [master]=# 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
>
> The docs mention that this is based on a best effort now. It may be
> interesting to improve that.
>
> > [master]=# select * from json_to_record('{"js": "true"}') as rec(js json);
> > js
> > ------
> > true
> >
> > [patched]=# select * from json_to_record('{"js": "a"}') as rec(js json);
> > js
> > -----
> > "a"
>
> That's indeed valid JSON.
>
> > [patched]=# select * from json_to_record('{"js": "true"}') as rec(js json);
> > js
> > --------
> > "true"
>
> And so is that.
>
> > The second patch adds assignment of correct ndims to array fields of RECORD
> > function result types.
> > Without this patch, attndims in tuple descriptors of RECORD types is always
> > 0 and the corresponding assertion fails in the next test:
> >
> > [patched]=# select json_to_record('{"a": [1, 2, 3]}') as rec(a int[]);
> >
> >
> > Should I submit these patches to commitfest?
>
> It seems to me that it would be a good idea to add them.
> --
> Michael
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
--
Best regards,
Aleksander Alekseev
Attachments:
signature.asc (application/pgp-signature, 832B) download
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: a.alekseev@postgrespro.ru, michael.paquier@gmail.com, n.gluhov@postgrespro.ru
Subject: Re: PATCH: recursive json_populate_record()
In-Reply-To: <20161227131541.GA6976@e733.localdomain>
* 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