Re: jsonb generator functions

Here is a patch for the generator and aggregate functions for jsonb that
we didn't manage to get done in time for 9.4. They are all equivalents
of the similarly names json functions. Included are

to_jsonb
jsonb_build_object
jsonb_build_array
jsonb_object
jsonb_agg
jsonb_object_agg

Still to come: documentation.

Adding to the next commitfest.

cheers

andrew

On Fri, Sep 26, 2014 at 1:54 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> Here is a patch for the generator and aggregate functions for jsonb that we
> didn't manage to get done in time for 9.4.

That's cool, but I hope someone revisits adding a concatenate
operator. That's a biggest omission IMHO. I'm not going to have time
for that.

--
Peter Geoghegan

On 09/26/2014 05:00 PM, Peter Geoghegan wrote:
> On Fri, Sep 26, 2014 at 1:54 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> Here is a patch for the generator and aggregate functions for jsonb that we
>> didn't manage to get done in time for 9.4.
>
> That's cool, but I hope someone revisits adding a concatenate
> operator. That's a biggest omission IMHO. I'm not going to have time
> for that.
>

This patch is the work that I have publicly promised to do.

Dmitry Dolgov is in fact working on jsonb_concat(), and several other
utility functions.

cheers

andrew

On 09/26/2014 04:54 PM, Andrew Dunstan wrote:
>
> Here is a patch for the generator and aggregate functions for jsonb
> that we didn't manage to get done in time for 9.4. They are all
> equivalents of the similarly names json functions. Included are
>
> to_jsonb
> jsonb_build_object
> jsonb_build_array
> jsonb_object
> jsonb_agg
> jsonb_object_agg
>
>
> Still to come: documentation.
>
> Adding to the next commitfest.

Revised patch to fix compiler warnings.

cheers

andrew

On 10/13/2014 09:37 AM, Andrew Dunstan wrote:
>
> On 09/26/2014 04:54 PM, Andrew Dunstan wrote:
>>
>> Here is a patch for the generator and aggregate functions for jsonb
>> that we didn't manage to get done in time for 9.4. They are all
>> equivalents of the similarly names json functions. Included are
>>
>> to_jsonb
>> jsonb_build_object
>> jsonb_build_array
>> jsonb_object
>> jsonb_agg
>> jsonb_object_agg
>>
>>
>> Still to come: documentation.
>>
>> Adding to the next commitfest.
>
>
> Revised patch to fix compiler warnings.
>

And again, initializing an incompletely initialized variable, as found
by Pavel Stehule.

cheers

andrew

2014-10-13 17:22 GMT+02:00 Andrew Dunstan <andrew(at)dunslane(dot)net>:

>
> On 10/13/2014 09:37 AM, Andrew Dunstan wrote:
>
>>
>> On 09/26/2014 04:54 PM, Andrew Dunstan wrote:
>>
>>>
>>> Here is a patch for the generator and aggregate functions for jsonb that
>>> we didn't manage to get done in time for 9.4. They are all equivalents of
>>> the similarly names json functions. Included are
>>>
>>> to_jsonb
>>> jsonb_build_object
>>> jsonb_build_array
>>> jsonb_object
>>> jsonb_agg
>>> jsonb_object_agg
>>>
>>>
>>> Still to come: documentation.
>>>
>>> Adding to the next commitfest.
>>>
>>
>>
>> Revised patch to fix compiler warnings.
>>
>>
> And again, initializing an incompletely initialized variable, as found by
> Pavel Stehule.
>

I checked a code, and I have only two small objection - a name
"jsonb_object_two_arg" is not good - maybe "json_object_keys_values" ?

Next: there are no tests for to_jsonb function.

Regards

Pavel

>
> cheers
>
> andrew
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

On 10/15/2014 07:38 AM, Pavel Stehule wrote:
>
>
> 2014-10-13 17:22 GMT+02:00 Andrew Dunstan <andrew(at)dunslane(dot)net
> <mailto:andrew(at)dunslane(dot)net>>:
>
>
> On 10/13/2014 09:37 AM, Andrew Dunstan wrote:
>
>
> On 09/26/2014 04:54 PM, Andrew Dunstan wrote:
>
>
> Here is a patch for the generator and aggregate functions
> for jsonb that we didn't manage to get done in time for
> 9.4. They are all equivalents of the similarly names json
> functions. Included are
>
> to_jsonb
> jsonb_build_object
> jsonb_build_array
> jsonb_object
> jsonb_agg
> jsonb_object_agg
>
>
> Still to come: documentation.
>
> Adding to the next commitfest.
>
>
>
> Revised patch to fix compiler warnings.
>
>
> And again, initializing an incompletely initialized variable, as
> found by Pavel Stehule.
>
>
> I checked a code, and I have only two small objection - a name
> "jsonb_object_two_arg" is not good - maybe "json_object_keys_values" ?

It's consistent with the existing json_object_two_arg. In all cases I
think I kept the names the same except for changing "json" to "jsonb".
Note that these _two_arg functions are not visible at the SQL level -
they are only visible in the C code.

I'm happy to be guided by others in changing or keeping these names.

>
> Next: there are no tests for to_jsonb function.
>
>

Oh, my bad. I'll add some.

Thank for the review.

cheers

andrew

On 10/15/2014 03:54 PM, I wrote:
>
> On 10/15/2014 07:38 AM, Pavel Stehule wrote:
>>
>>
>>
>> I checked a code, and I have only two small objection - a name
>> "jsonb_object_two_arg" is not good - maybe "json_object_keys_values" ?
>
> It's consistent with the existing json_object_two_arg. In all cases I
> think I kept the names the same except for changing "json" to "jsonb".
> Note that these _two_arg functions are not visible at the SQL level -
> they are only visible in the C code.
>
> I'm happy to be guided by others in changing or keeping these names.

If we really want to change the name of json_object_two_arg, it would
probably be best to change it NOW in 9.4 before it gets out into a
production release at all.

Thoughts?

cheers

andrew

Andrew Dunstan wrote:

> If we really want to change the name of json_object_two_arg, it
> would probably be best to change it NOW in 9.4 before it gets out
> into a production release at all.

Doesn't it require initdb? If so, I think it's too late now.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 10/15/2014 05:47 PM, Alvaro Herrera wrote:
> Andrew Dunstan wrote:
>
>> If we really want to change the name of json_object_two_arg, it
>> would probably be best to change it NOW in 9.4 before it gets out
>> into a production release at all.
> Doesn't it require initdb? If so, I think it's too late now.
>

Yeah, you're right, it would.

OK, forget that.

cheers

andrew

2014-10-15 23:49 GMT+02:00 Andrew Dunstan <andrew(at)dunslane(dot)net>:

>
> On 10/15/2014 05:47 PM, Alvaro Herrera wrote:
>
>> Andrew Dunstan wrote:
>>
>> If we really want to change the name of json_object_two_arg, it
>>> would probably be best to change it NOW in 9.4 before it gets out
>>> into a production release at all.
>>>
>> Doesn't it require initdb? If so, I think it's too late now.
>>
>>
yes, it is too heavy argument.

ok

Pavel

>
> Yeah, you're right, it would.
>
> OK, forget that.
>
> cheers
>
> andrew
>

On 10/15/2014 03:54 PM, Andrew Dunstan wrote:
>
>>
>> I checked a code, and I have only two small objection - a name
>> "jsonb_object_two_arg" is not good - maybe "json_object_keys_values" ?
>
> It's consistent with the existing json_object_two_arg. In all cases I
> think I kept the names the same except for changing "json" to "jsonb".
> Note that these _two_arg functions are not visible at the SQL level -
> they are only visible in the C code.
>
> I'm happy to be guided by others in changing or keeping these names.
>
>>
>> Next: there are no tests for to_jsonb function.
>>
>>
>
> Oh, my bad. I'll add some.
>
> Thank for the review.
>

Here is a new patch that includes documentation and addresses all these
issues, except that I didn't change the name of jsonb_object_two_arg to
keep it consistent with the name of json_object_two_arg. I'm happy to
change both if people feel it matters.

cheers

andrew

Hi

2014-10-27 15:33 GMT+01:00 Andrew Dunstan <andrew(at)dunslane(dot)net>:

>
> On 10/15/2014 03:54 PM, Andrew Dunstan wrote:
>
>>
>>
>>> I checked a code, and I have only two small objection - a name
>>> "jsonb_object_two_arg" is not good - maybe "json_object_keys_values" ?
>>>
>>
>> It's consistent with the existing json_object_two_arg. In all cases I
>> think I kept the names the same except for changing "json" to "jsonb". Note
>> that these _two_arg functions are not visible at the SQL level - they are
>> only visible in the C code.
>>
>> I'm happy to be guided by others in changing or keeping these names.
>>
>>
>>> Next: there are no tests for to_jsonb function.
>>>
>>>
>>>
>> Oh, my bad. I'll add some.
>>
>> Thank for the review.
>>
>>
>
> Here is a new patch that includes documentation and addresses all these
> issues, except that I didn't change the name of jsonb_object_two_arg to
> keep it consistent with the name of json_object_two_arg. I'm happy to
> change both if people feel it matters.
>

I checked last patch "jsonbmissingfunc5.patch" and I have no any objections:

1. This jsonb API is consistent with current JSON API, so we surely would
to this functionality

2. A implementation is clean without side effects and without impact on
current code.

3. Patching and compilation are without any issues and warnings

4. Source code respects PostgreSQL coding rules

5. All regress tests was passed without problems

6. Documentation was build without problems

7. Patch contains necessary regress tests

8. Patch contains necessary documentation for new functions.

Patch is ready for commiters

Thank you for patch

Regards

Pavel

>
> cheers
>
> andrew
>

Andrew Dunstan wrote:

This bit:

> +/*
> + * Determine how we want to render values of a given type in datum_to_jsonb.
> + *
> + * Given the datatype OID, return its JsonbTypeCategory, as well as the type's
> + * output function OID. If the returned category is JSONBTYPE_CAST, we
> + * return the OID of the type->JSON cast function instead.
> + */
> +static void
> +jsonb_categorize_type(Oid typoid,
> + JsonbTypeCategory * tcategory,
> + Oid *outfuncoid)
> +{

seems like it can return without having set the category and func OID,
if there's no available cast. Callers don't seem to check for this
condition; is this a bug? If not, why not? Maybe some extra comments
are warranted.

Right now, for the "general case" there, there are two syscache lookups
rather than one. The fix is simple: just do the getTypeOutputInfo call
inside each case inside the switch instead of once at the beginning, so
that the general case can omit it; then there is just one syscache
access in all the cases. json.c suffers from the same problem.

Anyway this whole business of searching through the CASTSOURCETARGET
syscache seems like it could be refactored. If I'm counting correctly,
that block now appears four times (three in this patch, once in json.c).
Can't we add a new function to (say) lsyscache and remove that?

I'm just commenting on that part because the syscache.h/pg_cast.h
inclusions look a bit out of place; it's certainly not a serious issue.

I looked at what makes you include miscadmin.h. It's only USE_XSD_DATES
as far as I can tell. I looked at how that might be fixed, and a quick
patch that moves DateStyle, DateOrder and IntervalStyle (and associated
definitions) to datetime.h seems to work pretty well ... except that
initdb.c requires to know about some DATEORDER constants; but frontend
code cannot include datetime.h because of Datum. So that idea crashed
and burned until someone reorganizes the whole datetime code, which
currently is pretty messy.

I don't have any further comments on this patch, other than please add
JsonbTypeCategory to pgindent/typedefs.list before doing your pgindent
run.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 10/27/2014 05:57 PM, Alvaro Herrera wrote:
> Andrew Dunstan wrote:
>
> This bit:
>
>> +/*
>> + * Determine how we want to render values of a given type in datum_to_jsonb.
>> + *
>> + * Given the datatype OID, return its JsonbTypeCategory, as well as the type's
>> + * output function OID. If the returned category is JSONBTYPE_CAST, we
>> + * return the OID of the type->JSON cast function instead.
>> + */
>> +static void
>> +jsonb_categorize_type(Oid typoid,
>> + JsonbTypeCategory * tcategory,
>> + Oid *outfuncoid)
>> +{
> seems like it can return without having set the category and func OID,
> if there's no available cast. Callers don't seem to check for this
> condition; is this a bug? If not, why not? Maybe some extra comments
> are warranted.

Umm, no. The outfuncoid is set by the call to getTypeOutputInfo() and
the category is set by every branch of the switch. We override the
funcoid in the case where there's a cast to json or jsonb.

I'll add a comment to that effect.

>
> Right now, for the "general case" there, there are two syscache lookups
> rather than one. The fix is simple: just do the getTypeOutputInfo call
> inside each case inside the switch instead of once at the beginning, so
> that the general case can omit it; then there is just one syscache
> access in all the cases. json.c suffers from the same problem.

We only do more than one if it's not a builtin type, or an array or
composite. So 99% of the time this won't even be called.

> Anyway this whole business of searching through the CASTSOURCETARGET
> syscache seems like it could be refactored. If I'm counting correctly,
> that block now appears four times (three in this patch, once in json.c).
> Can't we add a new function to (say) lsyscache and remove that?

Twice, not three times in this patch, unless I'm going crazier than I
thought.

I can add a function to lsyscache along the lines of

Oid get_cast_func(Oid from_type, Oid to_type)

if you think it's worth it.

cheers

andrew

On 10/28/2014 09:49 AM, Andrew Dunstan wrote:
>
> On 10/27/2014 05:57 PM, Alvaro Herrera wrote:
>
>> Anyway this whole business of searching through the CASTSOURCETARGET
>> syscache seems like it could be refactored. If I'm counting correctly,
>> that block now appears four times (three in this patch, once in json.c).
>> Can't we add a new function to (say) lsyscache and remove that?
>
> Twice, not three times in this patch, unless I'm going crazier than I
> thought.
>
> I can add a function to lsyscache along the lines of
>
> Oid get_cast_func(Oid from_type, Oid to_type)
>
> if you think it's worth it.
>
>

OK, here is a new patch version that

* uses find_coercion_path() to find the cast function if any, as
discussed elsewhere
* removes calls to getTypeOutputInfo() except where required
* honors a cast to json only for rendering both json and jsonb
* adds processing for the date type that was previously missing in
datum_to_jsonb

cheers

andrew

Andrew Dunstan wrote:

> OK, here is a new patch version that
>
> * uses find_coercion_path() to find the cast function if any, as
> discussed elsewhere
> * removes calls to getTypeOutputInfo() except where required
> * honors a cast to json only for rendering both json and jsonb
> * adds processing for the date type that was previously missing in
> datum_to_jsonb

Did this go anywhere?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 12/08/2014 04:21 AM, Alvaro Herrera wrote:
> Andrew Dunstan wrote:
>
>> OK, here is a new patch version that
>>
>> * uses find_coercion_path() to find the cast function if any, as
>> discussed elsewhere
>> * removes calls to getTypeOutputInfo() except where required
>> * honors a cast to json only for rendering both json and jsonb
>> * adds processing for the date type that was previously missing in
>> datum_to_jsonb
> Did this go anywhere?
>

Not, yet. I hope to get to it this week.

cheers

andrew

On 12/08/2014 01:00 PM, Andrew Dunstan wrote:
>
> On 12/08/2014 04:21 AM, Alvaro Herrera wrote:
>> Andrew Dunstan wrote:
>>
>>> OK, here is a new patch version that
>>>
>>> * uses find_coercion_path() to find the cast function if any, as
>>> discussed elsewhere
>>> * removes calls to getTypeOutputInfo() except where required
>>> * honors a cast to json only for rendering both json and jsonb
>>> * adds processing for the date type that was previously missing in
>>> datum_to_jsonb
>> Did this go anywhere?
>>
>
> Not, yet. I hope to get to it this week.
>
>

OK, here is a new version.

The major change is that the aggregate final functions now clone the
transition value rather than modifying it directly, avoiding a similar
nearby error which Tom fixed recently.

Also here is a patch factored out which applies the
find_coercion_pathway change to json.c. I'm inclined to say we should
backpatch this to 9.4 (and with a small change 9.3). Thoughts?

cheers

andrew

On 12/12/2014 01:10 PM, Andrew Dunstan wrote:
>
> On 12/08/2014 01:00 PM, Andrew Dunstan wrote:
>>
>> On 12/08/2014 04:21 AM, Alvaro Herrera wrote:
>>> Andrew Dunstan wrote:
>>>
>>>> OK, here is a new patch version that
>>>>
>>>> * uses find_coercion_path() to find the cast function if any, as
>>>> discussed elsewhere
>>>> * removes calls to getTypeOutputInfo() except where required
>>>> * honors a cast to json only for rendering both json and jsonb
>>>> * adds processing for the date type that was previously missing in
>>>> datum_to_jsonb
>>> Did this go anywhere?
>>>
>>
>> Not, yet. I hope to get to it this week.
>>
>>
>
>
> OK, here is a new version.
>
> The major change is that the aggregate final functions now clone the
> transition value rather than modifying it directly, avoiding a similar
> nearby error which Tom fixed recently.
>
> Also here is a patch factored out which applies the
> find_coercion_pathway change to json.c. I'm inclined to say we should
> backpatch this to 9.4 (and with a small change 9.3). Thoughts?
>

Er this time with patches.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> Also here is a patch factored out which applies the
>> find_coercion_pathway change to json.c. I'm inclined to say we should
>> backpatch this to 9.4 (and with a small change 9.3). Thoughts?

Meh. Maybe I'm just feeling gunshy because I broke something within
the past 24 hours, but at this point (with 9.4.0 wrap only 3 days away)
I'm inclined to avoid any 9.4 code churn that's not clearly necessary.
You argued upthread that this change would not result in any behavioral
changes in which cast method gets selected. If that's true, then we don't
really need to back-patch; while if it turns out not to be true, we
definitely don't want it in 9.3 and I'd argue it's too late for 9.4 also.

In short, I think it's fine for the 9.4 JSON code to start diverging
from HEAD at this point ...

regards, tom lane

On 12/12/2014 01:55 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>> Also here is a patch factored out which applies the
>>> find_coercion_pathway change to json.c. I'm inclined to say we should
>>> backpatch this to 9.4 (and with a small change 9.3). Thoughts?
> Meh. Maybe I'm just feeling gunshy because I broke something within
> the past 24 hours, but at this point (with 9.4.0 wrap only 3 days away)
> I'm inclined to avoid any 9.4 code churn that's not clearly necessary.
> You argued upthread that this change would not result in any behavioral
> changes in which cast method gets selected. If that's true, then we don't
> really need to back-patch; while if it turns out not to be true, we
> definitely don't want it in 9.3 and I'd argue it's too late for 9.4 also.
>
> In short, I think it's fine for the 9.4 JSON code to start diverging
> from HEAD at this point ...

Ok

cheers

andrew