Re: [PATCH] Generalized JSON output functions

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.stehule@gmail.com>:

2015-07-03 12:27 GMT+02:00 Heikki Linnakangas <hlinnaka@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.shulgin@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