new json funcs

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andrew Dunstan <andrew@dunslane.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 01/10/2014 01:27 PM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andrew Dunstan <andrew@dunslane.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 01/10/2014 01:58 PM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera <alvherre@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andrew Dunstan <andrew@dunslane.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jan 10, 2014 at 02:39:12PM -0500, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.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@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera <alvherre@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 01/10/2014 07:16 PM, Tom Lane wrote:

Alvaro Herrera <alvherre@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andrew Dunstan <andrew@dunslane.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers