On 02/24/2013 02:09 PM, Steve Singer wrote:
> 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.
I'm not thrilled about that, because we're likely to want to add more
JSON-specific casts to built-in or extension types in the future. If
doing so changes behaviour, causing something that used to work to
continue to work but produce a different result, that'll result in
considerable arguments about backward compatibility.
I'd be happier to require explicit casts to text or require the user to
explicitly CREATE CAST where no JSON-aware cast is already defined.
-- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
