Re: json generation enhancements

From: Andrew Dunstan <andrew(at)dunslane(dot)net>
To: Steve Singer <steve(at)ssinger(dot)info>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: json generation enhancements
Date: 2013-02-25 22:37:03
Message-ID: 512BE78F.2000209@dunslane.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


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

Attachment Content-Type Size
json_enhancements_part1-v4.patch text/x-patch 36.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2013-02-25 23:14:16 Re: pg_xlogdump
Previous Message Erik Rijkers 2013-02-25 22:07:27 Re: Materialized views WIP patch