Hi I am sending review of this patch:
1. I reread a previous discussion and almost all are for this patch (me too) 2. I have to fix a typo in hstore_io.c function (update attached), other (patching, regress tests) without problems My objections: 1. comments - missing comment for some basic API, basic fields like "key_scalar" and similar 2. why you did indirect call via JsonOutContext? What is benefit dst.value(&dst, (Datum) 0, JSONTYPE_NULL, InvalidOid, InvalidOid, false); instead json_out_value(&dst, ....) ? Is it necessary? 3. if it should be used everywhere, then in EXPLAIN statement too. Regards Pavel 2015-07-10 6:31 GMT+02:00 Pavel Stehule <pavel.steh...@gmail.com>: > > > 2015-07-03 12:27 GMT+02:00 Heikki Linnakangas <hlinn...@iki.fi>: > >> On 05/27/2015 09:51 PM, Andrew Dunstan wrote: >> >>> >>> On 05/27/2015 02:37 PM, Robert Haas wrote: >>> >>>> On Tue, May 26, 2015 at 2:50 AM, Shulgin, Oleksandr >>>> <oleksandr.shul...@zalando.de> wrote: >>>> >>>>> Is it reasonable to add this patch to CommitFest now? >>>>> >>>> It's always reasonable to add a patch to the CommitFest if you would >>>> like for it to be reviewed and avoid having it get forgotten about. >>>> There seems to be some disagreement about whether we want this, but >>>> don't let that stop you from adding it to the next CommitFest. >>>> >>> >>> I'm not dead set against it either. When I have time I will take a >>> closer look. >>> >> >> Andrew, will you have the time to review this? Please add yourself as >> reviewer in the commitfest app if you do. >> >> My 2 cents is that I agree with your initial reaction: This is a lot of >> infrastructure and generalizing things, for little benefit. Let's change >> the current code where we generate JSON to be consistent with whitespace, >> and call it a day. >> > > I am thinking so it is not bad idea. This code can enforce uniform > format, and it can check if produced value is correct. It can be used in > our code, it can be used by extension's developers. > > This patch is not small, but really new lines are not too much. > > I'll do review today. > > Regards > > Pavel > > > > >> - Heikki >> >> >> -- >> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) >> To make changes to your subscription: >> http://www.postgresql.org/mailpref/pgsql-hackers >> > >
