Re: json generation enhancements

On 02/24/2013 01:09 AM, Steve Singer wrote:
> On 13-01-11 11:03 AM, Andrew Dunstan wrote:
>>
>> On 01/11/2013 11:00 AM, Andrew Dunstan wrote:
>>>
>>> I have not had anyone follow up on this, so I have added docs and
>>> will add this to the commitfest.
>>>
>>> Recap:
>>>
>>> This adds the following:
>>>
>>> json_agg(anyrecord) -> json
>>> to_json(any) -> json
>>> hstore_to_json(hstore) -> json (also used as a cast)
>>> hstore_to_json_loose(hstore) -> json
>>>
>>> Also, in json generation, if any non-builtin type has a cast to
>>> json, that function is used instead of the type's output function.
>>>
>>>
>>
>> This time with a patch.
>>
>
> Here is a review of this patch.,
>
> Overview
> ---------------------
> This patch adds a set of functions to convert types to json. Specifically
>
> * An aggregate that take the elements and builds up a json array.
> * Conversions from an hstore to json
>
> For non-builtin types the text conversion is used unless a cast has
> specifically been defined from the type to json.
>
> There was some discussion last year on this patch that raised three
> issues
>
> a) Robert was concerned that if someone added a cast from a non-core
> type like citext to json that it would change the behaviour of this
> function. No one else offered an opinion on this at the time. I don't
> see this as a problem, if I add a cast between citext (or any other
> non-core datatype) to json I would expect it to effect how that
> datatype is generated as a json object in any functions that generate
> json. I don't see why this would be surprising behaviour. If I add
> a cast between my datatype and json to generate a json representation
> that differs from the textout representation then I would expect that
> representation to be used in the json_agg function as well.
>
> b) There was some talk in the json bikeshedding thread that json_agg()
> should be renamed to collect_json() but more people preferred json_agg().
>
> c) Should an aggregate of an empty result produce NULL or an empty
> json element. Most people who commented on the thread seemed to feel
> that NULL is preferred because it is consistent with other aggregates
>
> I feel that the functionality of this patch is fine.
>
> Testing
> -------------------
>
> When I try running the regression tests with this patch I get:
> creating template1 database in
> /usr/local/src/postgresql/src/test/regress/./tmp_check/data/base/1 ...
> FATAL: could not create unique index "pg_proc_oid_index"
> DETAIL: Key (oid)=(3171) is duplicated.
> child process exited with exit code 1
>
> oid 3170, 3171 and 3172 appears to be used by lo_tell64 and lo_lseek64
>
> If I fix those up the regression tests pass.
>
> I tried using the new functions in a few different ways and everything
> worked as expected.
>
> Code Review
> -----------
> in contrib/hstore/hstore_io.c
> + /* not an int - try a double */
> + double doubleres =
> strtod(src->data,&endptr);
> + if (*endptr == '\0')
> + is_number = true;
> + else if (false)
> + {
> + /* shut the compiler
> up about unused variables */
> + longres = (long)
> doubleres;
> + longres = longres / 2;
>
>
> I dislike that we have to do this to avoid compiler warnings. I am
> also worried the some other compiler might decide that the else if
> (false) is worthy of a warning. Does anyone have cleaner way of
> getting rid of the warning we get if we don't store the strtol/strtod
> result?
>
> Should we do something like:
>
> (void) ( strtol(src->data,&endptr,10)+1);
>
> Other than that I don't see any issues.
>
>

Thanks for the review. Revised patch attached with fresh OIDs and
numeric detection code cleaned up along the lines you suggest.

cheers

andrew