Re: [PATCH] Generalized JSON output functions - Mailing list pgsql-hackers

From Pavel Stehule
Subject Re: [PATCH] Generalized JSON output functions
Date
Msg-id CAFj8pRAHRs3=YVE5yJ9b=-kgqAHzSjQcqgxXn8+kfKMmmbioWw@mail.gmail.com
Whole thread Raw
In response to Re: [PATCH] Generalized JSON output functions  ("Shulgin, Oleksandr" <oleksandr.shulgin@zalando.de>)
Responses Re: [PATCH] Generalized JSON output functions  ("Shulgin, Oleksandr" <oleksandr.shulgin@zalando.de>)
List pgsql-hackers


2015-07-10 15:57 GMT+02:00 Shulgin, Oleksandr <oleksandr.shulgin@zalando.de>:
2015-07-10 14:34 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:
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

I thought it was pretty obvious from the code, because it's sort of the only source for docs on the subject right now.  Should we add proper documentation section, this would have been documented for sure.
 
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, ....)

For consistency.  Even though we initialize the output context ourselves, there might be some code introduced between json_out_init_context() and dst.value() calls that replaces some of the callbacks, and then there would be a difference.
 
with this consistency? I didn't see this style everywhere in Postgres?  Isn't it premature optimization?


 
3. if it should be used everywhere, then in EXPLAIN statement too.

Ahh.. good catch.  I'll have a look on this now.

Thanks for the review!

--
Alex


pgsql-hackers by date:

Previous
From: Andres Freund
Date:
Subject: Re: LANGUAGE sql functions don't use the custom plan logic
Next
From: Sawada Masahiko
Date:
Subject: Re: Freeze avoidance of very large table.