new json funcs

Here is a patch for the new json functions I mentioned a couple of
months ago. These are:

json_to_record
json_to_recordset
json_object
json_build_array
json_build_object
json_object_agg

So far there are no docs, but the way these work is illustrated in the
regression tests - I hope to have docs within a few days.

cheers

andrew

Is it just me, or is the json_array_element(json, int) function not
documented?

(Not a bug in this patch, I think ...)

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

On 01/10/2014 12:42 PM, Alvaro Herrera wrote:
> Is it just me, or is the json_array_element(json, int) function not
> documented?
>
> (Not a bug in this patch, I think ...)
>

As discussed at the time, we didn't document the functions underlying
the json operators, just the operators themselves.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 01/10/2014 12:42 PM, Alvaro Herrera wrote:
>> Is it just me, or is the json_array_element(json, int) function not
>> documented?

> As discussed at the time, we didn't document the functions underlying
> the json operators, just the operators themselves.

I see though that json_array_element has a DESCR comment. I believe
project policy is that if a function is not meant to be invoked by name
but only through an operator, its pg_description entry should just be
"implementation of xyz operator", with the real comment attached only
to the operator. Otherwise \df users are likely to be misled into using
the function when they're not really supposed to; and at the very least
they will bitch about its lack of documentation.

See commits 94133a935414407920a47d06a6e22734c974c3b8 and
908ab80286401bb20a519fa7dc7a837631f20369.

regards, tom lane

On 01/10/2014 01:27 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 01/10/2014 12:42 PM, Alvaro Herrera wrote:
>>> Is it just me, or is the json_array_element(json, int) function not
>>> documented?
>> As discussed at the time, we didn't document the functions underlying
>> the json operators, just the operators themselves.
> I see though that json_array_element has a DESCR comment. I believe
> project policy is that if a function is not meant to be invoked by name
> but only through an operator, its pg_description entry should just be
> "implementation of xyz operator", with the real comment attached only
> to the operator. Otherwise \df users are likely to be misled into using
> the function when they're not really supposed to; and at the very least
> they will bitch about its lack of documentation.
>
> See commits 94133a935414407920a47d06a6e22734c974c3b8 and
> 908ab80286401bb20a519fa7dc7a837631f20369.
>
>

OK, I can fix that I guess.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 01/10/2014 01:27 PM, Tom Lane wrote:
>> See commits 94133a935414407920a47d06a6e22734c974c3b8 and
>> 908ab80286401bb20a519fa7dc7a837631f20369.

> OK, I can fix that I guess.

Sure, just remove the DESCR comments for the functions that aren't meant
to be used directly.

I don't think this is back-patchable, but it's a minor point, so at least
for me a fix in HEAD is sufficient.

I wonder whether we should add an opr_sanity test verifying that operator
implementation functions don't have their own comments? The trouble is
that there are a few that are supposed to, but maybe that list is stable
enough that it'd be okay to memorialize in the expected output.

regards, tom lane

Andrew Dunstan wrote:
>
> On 01/10/2014 12:42 PM, Alvaro Herrera wrote:
> >Is it just me, or is the json_array_element(json, int) function not
> >documented?
>
> As discussed at the time, we didn't document the functions
> underlying the json operators, just the operators themselves.

Oh, I see. That's fine with me. From the source code it's hard to see
when a SQL-callable function is only there to implement an operator,
though (and it seems a bit far-fetched to suppose that the developer
will think, upon seeing an undocumented function, "oh this must
implement some operator, I will look it up at pg_proc.h").

I think the operator(s) should be mentioned in the comment on top of the
function.

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

Tom Lane wrote:

> I wonder whether we should add an opr_sanity test verifying that operator
> implementation functions don't have their own comments? The trouble is
> that there are a few that are supposed to, but maybe that list is stable
> enough that it'd be okay to memorialize in the expected output.

+1. It's an easy rule to overlook.

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

On 01/10/2014 01:58 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 01/10/2014 01:27 PM, Tom Lane wrote:
>>> See commits 94133a935414407920a47d06a6e22734c974c3b8 and
>>> 908ab80286401bb20a519fa7dc7a837631f20369.
>> OK, I can fix that I guess.
> Sure, just remove the DESCR comments for the functions that aren't meant
> to be used directly.
>
> I don't think this is back-patchable, but it's a minor point, so at least
> for me a fix in HEAD is sufficient.
>
> I wonder whether we should add an opr_sanity test verifying that operator
> implementation functions don't have their own comments? The trouble is
> that there are a few that are supposed to, but maybe that list is stable
> enough that it'd be okay to memorialize in the expected output.
>
>

Well, that would be ok as long as there was a comment in the file so
that developers don't just think it's OK to extend the list (it's a bit
like part of the reason we don't allow shift/reduce conflicts - if we
allowed them people would just keep adding more, and they wouldn't stick
out like a sore thumb.)

The comment in the current test says:

-- Check that operators' underlying functions have suitable comments,
-- namely 'implementation of XXX operator'. In some cases involving
legacy
-- names for operators, there are multiple operators referencing the
same
-- pg_proc entry, so ignore operators whose comments say they are
deprecated.
-- We also have a few functions that are both operator support and
meant to
-- be called directly; those should have comments matching their
operator.

The history here is that originally I was intending to have these
functions documented, and so the descriptions were made to match the
operator descriptions, so that we didn't get a failure on this test.
Later we decided not to document them as part of last release's
bike-shedding, but the function descriptions didn't get changed / removed.

cheers

andrew

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Oh, I see. That's fine with me. From the source code it's hard to see
> when a SQL-callable function is only there to implement an operator,
> though (and it seems a bit far-fetched to suppose that the developer
> will think, upon seeing an undocumented function, "oh this must
> implement some operator, I will look it up at pg_proc.h").

> I think the operator(s) should be mentioned in the comment on top of the
> function.

Oh, you're complaining about the lack of any header comment for the
function in the source code. That's a different matter from the
user-visible docs, but I agree that it's poor practice to not have
anything.

regards, tom lane

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> The history here is that originally I was intending to have these
> functions documented, and so the descriptions were made to match the
> operator descriptions, so that we didn't get a failure on this test.
> Later we decided not to document them as part of last release's
> bike-shedding, but the function descriptions didn't get changed / removed.

Ah. I suppose there's no way to cross-check the state of the function's
pg_description comment against whether it has SGML documentation :-(

regards, tom lane

On Fri, Jan 10, 2014 at 02:39:12PM -0500, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> > The history here is that originally I was intending to have these
> > functions documented, and so the descriptions were made to match the
> > operator descriptions, so that we didn't get a failure on this test.
> > Later we decided not to document them as part of last release's
> > bike-shedding, but the function descriptions didn't get changed / removed.
>
> Ah. I suppose there's no way to cross-check the state of the function's
> pg_description comment against whether it has SGML documentation :-(

FDWs to the rescue!

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Tom Lane wrote:
>> I wonder whether we should add an opr_sanity test verifying that operator
>> implementation functions don't have their own comments? The trouble is
>> that there are a few that are supposed to, but maybe that list is stable
>> enough that it'd be okay to memorialize in the expected output.

> +1. It's an easy rule to overlook.

Here's a proposed addition to opr_sanity.sql for that:

-- Show all the operator-implementation functions that have their own
-- comments. This should happen only for cases where the function and
-- operator syntaxes are equally preferred (and are both documented).
-- Because it's a pretty short list, it's okay to have all the occurrences
-- appearing in the expected output.
WITH funcdescs AS (
SELECT p.oid as p_oid, proname, o.oid as o_oid,
obj_description(p.oid, 'pg_proc') as prodesc,
'implementation of ' || oprname || ' operator' as expecteddesc,
obj_description(o.oid, 'pg_operator') as oprdesc
FROM pg_proc p JOIN pg_operator o ON oprcode = p.oid
WHERE o.oid <= 9999
)
SELECT p_oid, proname, prodesc FROM funcdescs
WHERE prodesc IS DISTINCT FROM expecteddesc
AND oprdesc NOT LIKE 'deprecated%'
ORDER BY 1;

In HEAD, this query produces

p_oid | proname | prodesc
-------+---------------------------+------------------------------------------------
378 | array_append | append element onto end of array
379 | array_prepend | prepend element onto front of array
1035 | aclinsert | add/update ACL item
1036 | aclremove | remove ACL item
1037 | aclcontains | contains
3947 | json_object_field | get json object field
3948 | json_object_field_text | get json object field as text
3949 | json_array_element | get json array element
3950 | json_array_element_text | get json array element as text
3952 | json_extract_path_op | get value from json with path elements
3954 | json_extract_path_text_op | get value from json as text with path elements
(11 rows)

The first five of these, I believe, are the cases I left alone back in
commit 94133a935414407920a47d06a6e22734c974c3b8. I'm thinking the
other six are the ones Andrew needs to remove the DESCR entries for.

regards, tom lane

On 01/10/2014 07:16 PM, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> Tom Lane wrote:
>>> I wonder whether we should add an opr_sanity test verifying that operator
>>> implementation functions don't have their own comments? The trouble is
>>> that there are a few that are supposed to, but maybe that list is stable
>>> enough that it'd be okay to memorialize in the expected output.
>> +1. It's an easy rule to overlook.
> Here's a proposed addition to opr_sanity.sql for that:
>
> -- Show all the operator-implementation functions that have their own
> -- comments. This should happen only for cases where the function and
> -- operator syntaxes are equally preferred (and are both documented).
> -- Because it's a pretty short list, it's okay to have all the occurrences
> -- appearing in the expected output.
> WITH funcdescs AS (
> SELECT p.oid as p_oid, proname, o.oid as o_oid,
> obj_description(p.oid, 'pg_proc') as prodesc,
> 'implementation of ' || oprname || ' operator' as expecteddesc,
> obj_description(o.oid, 'pg_operator') as oprdesc
> FROM pg_proc p JOIN pg_operator o ON oprcode = p.oid
> WHERE o.oid <= 9999
> )
> SELECT p_oid, proname, prodesc FROM funcdescs
> WHERE prodesc IS DISTINCT FROM expecteddesc
> AND oprdesc NOT LIKE 'deprecated%'
> ORDER BY 1;
>
> In HEAD, this query produces
>
> p_oid | proname | prodesc
> -------+---------------------------+------------------------------------------------
> 378 | array_append | append element onto end of array
> 379 | array_prepend | prepend element onto front of array
> 1035 | aclinsert | add/update ACL item
> 1036 | aclremove | remove ACL item
> 1037 | aclcontains | contains
> 3947 | json_object_field | get json object field
> 3948 | json_object_field_text | get json object field as text
> 3949 | json_array_element | get json array element
> 3950 | json_array_element_text | get json array element as text
> 3952 | json_extract_path_op | get value from json with path elements
> 3954 | json_extract_path_text_op | get value from json as text with path elements
> (11 rows)
>
> The first five of these, I believe, are the cases I left alone back in
> commit 94133a935414407920a47d06a6e22734c974c3b8. I'm thinking the
> other six are the ones Andrew needs to remove the DESCR entries for.
>
>

Yeah, you just knocked out the last condition in the where clause, right?

Confirmed that when I do that and remove those DESCR entries we're left
with those 5 rows.

Is it better to knock out the DESCR entries totally or make them read
"implementation of foo operator"?

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Is it better to knock out the DESCR entries totally or make them read
> "implementation of foo operator"?

Just delete them --- initdb is responsible for inserting that boilerplate
where appropriate.

regards, tom lane

On 1/3/14, 9:00 PM, Andrew Dunstan wrote:
>
> Here is a patch for the new json functions I mentioned a couple of
> months ago. These are:
>
> json_to_record
> json_to_recordset
> json_object
> json_build_array
> json_build_object
> json_object_agg
>
> So far there are no docs, but the way these work is illustrated in the
> regression tests - I hope to have docs within a few days.

Compiler warnings:

json.c: In function ‘json_object_two_arg’:
json.c:2210:5: warning: unused variable ‘count’ [-Wunused-variable]

jsonfuncs.c: In function ‘json_to_record’:
jsonfuncs.c:1955:16: warning: unused variable ‘tuple’ [-Wunused-variable]
jsonfuncs.c:1953:18: warning: variable ‘rec’ set but not used [-Wunused-but-set-variable]

Also, please run your patch through git diff --check. I have noticed
that several of your patches have hilarious whitespace, maybe
something with your editor.

On 01/16/2014 01:57 PM, Peter Eisentraut wrote:
> On 1/3/14, 9:00 PM, Andrew Dunstan wrote:
>> Here is a patch for the new json functions I mentioned a couple of
>> months ago. These are:
>>
>> json_to_record
>> json_to_recordset
>> json_object
>> json_build_array
>> json_build_object
>> json_object_agg
>>
>> So far there are no docs, but the way these work is illustrated in the
>> regression tests - I hope to have docs within a few days.
> Compiler warnings:
>
> json.c: In function ‘json_object_two_arg’:
> json.c:2210:5: warning: unused variable ‘count’ [-Wunused-variable]
>
> jsonfuncs.c: In function ‘json_to_record’:
> jsonfuncs.c:1955:16: warning: unused variable ‘tuple’ [-Wunused-variable]
> jsonfuncs.c:1953:18: warning: variable ‘rec’ set but not used [-Wunused-but-set-variable]
>
> Also, please run your patch through git diff --check. I have noticed
> that several of your patches have hilarious whitespace, maybe
> something with your editor.
>

I'm happy to keep you amused. Some of this was probably due to cutting
and pasting.

all these issues are fixed in the attached patch.

cheers

andrew

On 01/16/2014 07:39 PM, Andrew Dunstan wrote:
>
> On 01/16/2014 01:57 PM, Peter Eisentraut wrote:
>> On 1/3/14, 9:00 PM, Andrew Dunstan wrote:
>>> Here is a patch for the new json functions I mentioned a couple of
>>> months ago. These are:
>>>
>>> json_to_record
>>> json_to_recordset
>>> json_object
>>> json_build_array
>>> json_build_object
>>> json_object_agg
>>>
>>> So far there are no docs, but the way these work is illustrated in the
>>> regression tests - I hope to have docs within a few days.
>> Compiler warnings:
>>
>> json.c: In function ‘json_object_two_arg’:
>> json.c:2210:5: warning: unused variable ‘count’ [-Wunused-variable]
>>
>> jsonfuncs.c: In function ‘json_to_record’:
>> jsonfuncs.c:1955:16: warning: unused variable ‘tuple’
>> [-Wunused-variable]
>> jsonfuncs.c:1953:18: warning: variable ‘rec’ set but not used
>> [-Wunused-but-set-variable]
>>
>> Also, please run your patch through git diff --check. I have noticed
>> that several of your patches have hilarious whitespace, maybe
>> something with your editor.
>>
>
>
> I'm happy to keep you amused. Some of this was probably due to cutting
> and pasting.
>
> all these issues are fixed in the attached patch.

In case anyone feels like really nitpicking, this version fixes some
pgindent weirdness due to an outdated typedef list in the previous version.

cheers

andrew

On 2014-01-17 03:08, Andrew Dunstan wrote:
> In case anyone feels like really nitpicking, this version fixes some
> pgindent weirdness due to an outdated typedef list in the previous version.

Is it possible for you to generate a diff that doesn't have all these
unrelated changes in it (from a pgindent run, I presume)? I just read
through three pagefuls and I didn't see any relevant changes, but I'm
not sure I want to keep doing that for the rest of the patch.

Regards,
Marko Tiikkaja

On 01/18/2014 12:34 PM, Marko Tiikkaja wrote:
> On 2014-01-17 03:08, Andrew Dunstan wrote:
>> In case anyone feels like really nitpicking, this version fixes some
>> pgindent weirdness due to an outdated typedef list in the previous
>> version.
>
> Is it possible for you to generate a diff that doesn't have all these
> unrelated changes in it (from a pgindent run, I presume)? I just read
> through three pagefuls and I didn't see any relevant changes, but I'm
> not sure I want to keep doing that for the rest of the patch.
>
>
>

This seems to be quite overstated. The chunks in the version 3 patch
that only contain pgindent effects are those at lines 751,771,866 and
1808 of json.c, by my reckoning. All the other changes are more than
formatting.

And undoing the pgindent changes and then individually applying all but
those mentioned above would take me a lot of time.

cheers

andrew

On 1/18/14, 9:38 PM, Andrew Dunstan wrote:
> On 01/18/2014 12:34 PM, Marko Tiikkaja wrote:
>> Is it possible for you to generate a diff that doesn't have all these
>> unrelated changes in it (from a pgindent run, I presume)? I just read
>> through three pagefuls and I didn't see any relevant changes, but I'm
>> not sure I want to keep doing that for the rest of the patch.
>>
>
> This seems to be quite overstated. The chunks in the version 3 patch
> that only contain pgindent effects are those at lines 751,771,866 and
> 1808 of json.c, by my reckoning. All the other changes are more than
> formatting.

Oh I see, there's a version 3 which improves things by a lot. I just
took the latest patch from the CF app and that was the v2 patch. Now
looking at it again, I see that it actually added new lines around
json.c:68, which I believe proves my point that reviewing such a patch
is hard.

> And undoing the pgindent changes and then individually applying all but
> those mentioned above would take me a lot of time.

v3 looks "ok". I would have preferred a patch with no unrelated
changes, but I can live with what we have there.

Something like the first three pagefuls of v2, however, would take *me*
a lot of time, which I believe is not acceptable. I don't care why a
patch has lots of unrelated stuff in it, I'm not going to waste my time
trying to figure out which parts are relevant and which aren't. That's
a lot of time wasted just to end up with a review possibly full of
missed problems and misunderstood code.

But I'll continue with my review now that this has been sorted out.

Regards,
Marko Tiikkaja

Hi Andrew,

On 1/18/14, 10:05 PM, I wrote:
> But I'll continue with my review now that this has been sorted out.

Sorry about the delay.

I think the API for the new functions looks good. They are all welcome
additions to the JSON family.

The implementation side looks reasonable to me. I'm not sure there's
need to duplicate so much code, though. E.g. json_to_recordset is
almost identical to json_populate_recordset, and json_to_record has a
bit of the same disease.

Finally, (as I'm sure you know already), docs are still missing.
Marking the patch Waiting on Author for the time being.

Regards,
Marko Tiikkaja

On 01/21/2014 06:21 PM, Marko Tiikkaja wrote:
> Hi Andrew,
>
> On 1/18/14, 10:05 PM, I wrote:
>> But I'll continue with my review now that this has been sorted out.
>
> Sorry about the delay.
>
> I think the API for the new functions looks good. They are all
> welcome additions to the JSON family.
>
> The implementation side looks reasonable to me. I'm not sure there's
> need to duplicate so much code, though. E.g. json_to_recordset is
> almost identical to json_populate_recordset, and json_to_record has a
> bit of the same disease.

I can probably factor some of that out. Of course, when it was an
extension there wasn't the possibility.

>
> Finally, (as I'm sure you know already), docs are still missing.
> Marking the patch Waiting on Author for the time being.
>
>

Yes, I have a draft, just waiting for time to go through it.

Thanks for the review.

cheers

andrew

On 01/21/2014 06:21 PM, Marko Tiikkaja wrote:
> Hi Andrew,
>
> On 1/18/14, 10:05 PM, I wrote:
>> But I'll continue with my review now that this has been sorted out.
>
> Sorry about the delay.
>
> I think the API for the new functions looks good. They are all
> welcome additions to the JSON family.
>
> The implementation side looks reasonable to me. I'm not sure there's
> need to duplicate so much code, though. E.g. json_to_recordset is
> almost identical to json_populate_recordset, and json_to_record has a
> bit of the same disease.
>
> Finally, (as I'm sure you know already), docs are still missing.
> Marking the patch Waiting on Author for the time being.
>
>
>

New patch attached. Main change is I changed
json_populate_record/json_to_record to call a common worker function,
and likewise with json_populate_recordset/json_to_recordset.

We're still finalizing the docs - should be ready in the next day or so.

cheers

andrew

On 01/22/2014 12:49 PM, Andrew Dunstan wrote:
>
> On 01/21/2014 06:21 PM, Marko Tiikkaja wrote:
>> Hi Andrew,
>>
>> On 1/18/14, 10:05 PM, I wrote:
>>> But I'll continue with my review now that this has been sorted out.
>>
>> Sorry about the delay.
>>
>> I think the API for the new functions looks good. They are all
>> welcome additions to the JSON family.
>>
>> The implementation side looks reasonable to me. I'm not sure there's
>> need to duplicate so much code, though. E.g. json_to_recordset is
>> almost identical to json_populate_recordset, and json_to_record has a
>> bit of the same disease.
>>
>> Finally, (as I'm sure you know already), docs are still missing.
>> Marking the patch Waiting on Author for the time being.
>>
>>
>>
>
>
> New patch attached. Main change is I changed
> json_populate_record/json_to_record to call a common worker function,
> and likewise with json_populate_recordset/json_to_recordset.
>
> We're still finalizing the docs - should be ready in the next day or so.

OK, here's the patch, this time with docs, thanks to Merlin Moncure and
Josh Berkus for help with that.

I want to do some more wordsmithing around json_to_record{set} and
json_populate_record{set}, but I think this is close to being
committable as is.

cheers

andrew

For consistency with the existing json functions (json_each,
json_each_text, etc.) it might be better to add separate
json_to_record_text and json_to_recordset_text functions in place of the
nested_as_text parameter to json_to_record and json_to_recordset.

On 24 January 2014 10:26, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

>
> On 01/22/2014 12:49 PM, Andrew Dunstan wrote:
>
>>
>> On 01/21/2014 06:21 PM, Marko Tiikkaja wrote:
>>
>>> Hi Andrew,
>>>
>>> On 1/18/14, 10:05 PM, I wrote:
>>>
>>>> But I'll continue with my review now that this has been sorted out.
>>>>
>>>
>>> Sorry about the delay.
>>>
>>> I think the API for the new functions looks good. They are all welcome
>>> additions to the JSON family.
>>>
>>> The implementation side looks reasonable to me. I'm not sure there's
>>> need to duplicate so much code, though. E.g. json_to_recordset is almost
>>> identical to json_populate_recordset, and json_to_record has a bit of the
>>> same disease.
>>>
>>> Finally, (as I'm sure you know already), docs are still missing. Marking
>>> the patch Waiting on Author for the time being.
>>>
>>>
>>>
>>>
>>
>> New patch attached. Main change is I changed json_populate_record/json_to_record
>> to call a common worker function, and likewise with
>> json_populate_recordset/json_to_recordset.
>>
>> We're still finalizing the docs - should be ready in the next day or so.
>>
>
>
> OK, here's the patch, this time with docs, thanks to Merlin Moncure and
> Josh Berkus for help with that.
>
> I want to do some more wordsmithing around json_to_record{set} and
> json_populate_record{set}, but I think this is close to being committable
> as is.
>
> 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 01/24/2014 03:40 PM, Laurence Rowe wrote:
> For consistency with the existing json functions (json_each,
> json_each_text, etc.) it might be better to add separate
> json_to_record_text and json_to_recordset_text functions in place of
> the nested_as_text parameter to json_to_record and json_to_recordset.
>
>

It wouldn't be consistent with json_populate_record() and
json_populate_recordset(), the two closest relatives, however.

And yes, I appreciate that we have not been 100% consistent. Community
design can be a bit messy that way.

cheers

andrew

On 01/24/2014 12:59 PM, Andrew Dunstan wrote:
>
> On 01/24/2014 03:40 PM, Laurence Rowe wrote:
>> For consistency with the existing json functions (json_each,
>> json_each_text, etc.) it might be better to add separate
>> json_to_record_text and json_to_recordset_text functions in place of
>> the nested_as_text parameter to json_to_record and json_to_recordset.
>>
>>
>
> It wouldn't be consistent with json_populate_record() and
> json_populate_recordset(), the two closest relatives, however.
>
> And yes, I appreciate that we have not been 100% consistent. Community
> design can be a bit messy that way.

FWIW, I prefer the parameter to having differently named functions.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Fri, Jan 24, 2014 at 3:26 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 01/24/2014 12:59 PM, Andrew Dunstan wrote:
>>
>> On 01/24/2014 03:40 PM, Laurence Rowe wrote:
>>> For consistency with the existing json functions (json_each,
>>> json_each_text, etc.) it might be better to add separate
>>> json_to_record_text and json_to_recordset_text functions in place of
>>> the nested_as_text parameter to json_to_record and json_to_recordset.
>>>
>>>
>>
>> It wouldn't be consistent with json_populate_record() and
>> json_populate_recordset(), the two closest relatives, however.
>>
>> And yes, I appreciate that we have not been 100% consistent. Community
>> design can be a bit messy that way.
>
> FWIW, I prefer the parameter to having differently named functions.

+1.

merlin

On 01/27/2014 12:43 PM, Merlin Moncure wrote:
> On Fri, Jan 24, 2014 at 3:26 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> On 01/24/2014 12:59 PM, Andrew Dunstan wrote:
>>> On 01/24/2014 03:40 PM, Laurence Rowe wrote:
>>>> For consistency with the existing json functions (json_each,
>>>> json_each_text, etc.) it might be better to add separate
>>>> json_to_record_text and json_to_recordset_text functions in place of
>>>> the nested_as_text parameter to json_to_record and json_to_recordset.
>>>>
>>>>
>>> It wouldn't be consistent with json_populate_record() and
>>> json_populate_recordset(), the two closest relatives, however.
>>>
>>> And yes, I appreciate that we have not been 100% consistent. Community
>>> design can be a bit messy that way.
>> FWIW, I prefer the parameter to having differently named functions.
> +1.
>

Note that we can only do this when the result type stays the same. It
does not for json_each/json_each_text or
json_extract_path/json_extract_path_text, which is why we have different
functions for those cases.

cheers

andrew

Andrew Dunstan escribió:

> Note that we can only do this when the result type stays the same.
> It does not for json_each/json_each_text or
> json_extract_path/json_extract_path_text, which is why we have
> different functions for those cases.

In C code, if I extract a value using json_object_field or
json_array_element, is there a way to turn it into the dequoted version,
that is, the value that I would have gotten had I called
json_object_field_text or json_array_element_text instead?

I wrote a quick and dirty hack in the event triggers patch that just
removes the outermost "" and turns any \" into ", but that's probably
incomplete. Does jsonfuncs.c offer any way to do this? That might be
useful for the crowd that cares about the detail being discussed in this
subthread.

Thanks,

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

On 01/27/2014 03:53 PM, Alvaro Herrera wrote:
> Andrew Dunstan escribió:
>
>> Note that we can only do this when the result type stays the same.
>> It does not for json_each/json_each_text or
>> json_extract_path/json_extract_path_text, which is why we have
>> different functions for those cases.
> In C code, if I extract a value using json_object_field or
> json_array_element, is there a way to turn it into the dequoted version,
> that is, the value that I would have gotten had I called
> json_object_field_text or json_array_element_text instead?
>
> I wrote a quick and dirty hack in the event triggers patch that just
> removes the outermost "" and turns any \" into ", but that's probably
> incomplete. Does jsonfuncs.c offer any way to do this? That might be
> useful for the crowd that cares about the detail being discussed in this
> subthread.
>

I'm not sure I understand the need. This is the difference between the
_text variants and their parents. Why would you call json_object_field
when you want the dequoted text?

cheers

andrew

Andrew Dunstan escribió:

> I'm not sure I understand the need. This is the difference between
> the _text variants and their parents. Why would you call
> json_object_field when you want the dequoted text?

Because I first need to know its type. Sometimes it's an array, or an
object, or a boolean, and for those I won't call the _text version
afterwards but just use the original.

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

On 01/27/2014 01:06 PM, Alvaro Herrera wrote:
> Andrew Dunstan escribió:
>
>> I'm not sure I understand the need. This is the difference between
>> the _text variants and their parents. Why would you call
>> json_object_field when you want the dequoted text?
>
> Because I first need to know its type. Sometimes it's an array, or an
> object, or a boolean, and for those I won't call the _text version
> afterwards but just use the original.

It would make more sense to extract them as JSON, check the type, and
convert.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus wrote:
> On 01/27/2014 01:06 PM, Alvaro Herrera wrote:
> > Andrew Dunstan escribió:
> >
> >> I'm not sure I understand the need. This is the difference between
> >> the _text variants and their parents. Why would you call
> >> json_object_field when you want the dequoted text?
> >
> > Because I first need to know its type. Sometimes it's an array, or an
> > object, or a boolean, and for those I won't call the _text version
> > afterwards but just use the original.
>
> It would make more sense to extract them as JSON, check the type, and
> convert.

That's what I'm already doing. My question is how do I convert it?
I have this, but would like to get rid of it:

/*
* dequote_jsonval
* Take a string value extracted from a JSON object, and return a copy of it
* with the quoting removed.
*
* Another alternative to this would be to run the extraction routine again,
* using the "_text" variant which returns the value without quotes; but this
* complicates the logic a lot because not all values are extracted in
* the same way (some are extracted using json_object_field, others
* using json_array_element). Dequoting the object already at hand is a
* lot easier.
*/
static char *
dequote_jsonval(char *jsonval)
{
char *result;
int inputlen = strlen(jsonval);
int i;
int j = 0;

result = palloc(strlen(jsonval) + 1);

/* skip the start and end quotes right away */
for (i = 1; i < inputlen - 1; i++)
{
/*
* XXX this skips the \ in a \" sequence but leaves other escaped
* sequences in place. Are there other cases we need to handle
* specially?
*/
if (jsonval[i] == '\\' &&
jsonval[i + 1] == '"')
{
i++;
continue;
}

result[j++] = jsonval[i];
}
result[j] = '\0';

return result;
}

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

On 01/28/2014 01:22 PM, Alvaro Herrera wrote:
> Josh Berkus wrote:
>> On 01/27/2014 01:06 PM, Alvaro Herrera wrote:
>>> Andrew Dunstan escribió:
>>>
>>>> I'm not sure I understand the need. This is the difference between
>>>> the _text variants and their parents. Why would you call
>>>> json_object_field when you want the dequoted text?
>>> Because I first need to know its type. Sometimes it's an array, or an
>>> object, or a boolean, and for those I won't call the _text version
>>> afterwards but just use the original.
>> It would make more sense to extract them as JSON, check the type, and
>> convert.
> That's what I'm already doing. My question is how do I convert it?
> I have this, but would like to get rid of it:
>
> /*
> * dequote_jsonval
> * Take a string value extracted from a JSON object, and return a copy of it
> * with the quoting removed.
> *
> * Another alternative to this would be to run the extraction routine again,
> * using the "_text" variant which returns the value without quotes; but this
> * complicates the logic a lot because not all values are extracted in
> * the same way (some are extracted using json_object_field, others
> * using json_array_element). Dequoting the object already at hand is a
> * lot easier.
> */
> static char *
> dequote_jsonval(char *jsonval)
> {
> char *result;
> int inputlen = strlen(jsonval);
> int i;
> int j = 0;
>
> result = palloc(strlen(jsonval) + 1);
>
> /* skip the start and end quotes right away */
> for (i = 1; i < inputlen - 1; i++)
> {
> /*
> * XXX this skips the \ in a \" sequence but leaves other escaped
> * sequences in place. Are there other cases we need to handle
> * specially?
> */
> if (jsonval[i] == '\\' &&
> jsonval[i + 1] == '"')
> {
> i++;
> continue;
> }
>
> result[j++] = jsonval[i];
> }
> result[j] = '\0';
>
> return result;
> }
>

Well, TIMTOWTDI. Here's roughly what I would do, in json.c, making the
json lexer do the work for us:

extern char *
dequote_scalar_json_string(char *jsonval)
{
text *json = cstring_to_text(jsonval);
JsonLexContext *lex = makeJsonLexContext(json, true);
JsonTokenType tok;
char *ret;

json_lex(lex);
tok = lex_peek(lex);
if (tok == JSON_TOKEN_STRING)
ret=pstrdup(lex->strval->data);
else
ret = jsonval;
pfree(lex->strval->data);
pfree(lex->strval);
pfree(lex);
pfree(json);

return ret;
}

I'm not sure if we should have a gadget like this at the SQL level also.

cheers

andrew

Hi Andrew,

On 1/24/14, 7:26 PM, Andrew Dunstan wrote:
> OK, here's the patch, this time with docs, thanks to Merlin Moncure and
> Josh Berkus for help with that.

Thanks, this one is looking pretty good. A couple of small issues:

- The oid 3195 of json_object_agg_transfn has been taken by a recent
commit, so that had to be changed. The patch compiled and passed tests
after that.

- Typo in the description of json_build_array: "agument list"

- I find (perhaps due to not being a native speaker) the description
of json_object a bit painful to read. I would've expected something like:

- Builds a JSON object out of a text array. The array must have
exactly one dimension
+ Builds a JSON object out of a text array. The array must have
either exactly one dimension
with an even number of members, in which case they are taken
as alternating name/value
- pairs, or two dimensions with such that each inner array has
exactly two elements, which
+ pairs, or two dimensions such that each inner array has
exactly two elements, which
are taken as a name/value pair.

but I'm not sure about that either.

- There are a few cases of curly braces around a single-statement
else, which I believe is against the project's code style guidelines.

Otherwise this patch looks good to my eyes.

Regards,
Marko Tiikkaja

On 01/28/2014 05:07 PM, Marko Tiikkaja wrote:
> Hi Andrew,
>
> On 1/24/14, 7:26 PM, Andrew Dunstan wrote:
>> OK, here's the patch, this time with docs, thanks to Merlin Moncure and
>> Josh Berkus for help with that.
>
> Thanks, this one is looking pretty good. A couple of small issues:
>
> - The oid 3195 of json_object_agg_transfn has been taken by a recent
> commit, so that had to be changed. The patch compiled and passed
> tests after that.

Yeah. These days you can't even build if there's a duplicate oid, so
fixing that and a catalog version bump would be part of committing.

>
> - Typo in the description of json_build_array: "agument list"

will fix.

>
> - I find (perhaps due to not being a native speaker) the description
> of json_object a bit painful to read. I would've expected something
> like:
>
> - Builds a JSON object out of a text array. The array must
> have exactly one dimension
> + Builds a JSON object out of a text array. The array must
> have either exactly one dimension
> with an even number of members, in which case they are taken
> as alternating name/value
> - pairs, or two dimensions with such that each inner array has
> exactly two elements, which
> + pairs, or two dimensions such that each inner array has
> exactly two elements, which
> are taken as a name/value pair.
>
> but I'm not sure about that either.

Yes, yours looks better.

>
> - There are a few cases of curly braces around a single-statement
> else, which I believe is against the project's code style guidelines.

IIRC we actually stopped pgindent removing that quite a few years ago,
and the formatting guidelines at
<http://www.postgresql.org/docs/devel/static/source-format.html> don't
say anything about it. Personally, I prefer consistency - I think either
both branches of an if/else should use curly braces or neither should. I
find it quite ugly and jarring when one does and the other doesn't.

Thanks for the review.

cheers

andrew