Re: [HACKERS] new json funcs

[Andrew Dunstan]

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 
 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



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

Re: [HACKERS] new json funcs

[Marko Tiikkaja]

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


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

Re: [HACKERS] new json funcs

[Andrew Dunstan]

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


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

Re: [HACKERS] new json funcs

[Alvaro Herrera]

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 Herrerahttp://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

Re: [HACKERS] new json funcs

[Josh Berkus]

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


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

Re: [HACKERS] new json funcs

[Alvaro Herrera]

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 Herrerahttp://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

Re: [HACKERS] new json funcs

[Andrew Dunstan]

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


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

Re: [HACKERS] new json funcs

[Alvaro Herrera]

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 Herrerahttp://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

Re: [HACKERS] new json funcs

[Andrew Dunstan]

On 01/27/2014 12:43 PM, Merlin Moncure wrote:

On Fri, Jan 24, 2014 at 3:26 PM, Josh Berkus  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



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

Re: [HACKERS] new json funcs

[Merlin Moncure]

On Fri, Jan 24, 2014 at 3:26 PM, Josh Berkus  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


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

Re: [HACKERS] new json funcs

[Josh Berkus]

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


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

Re: [HACKERS] new json funcs

[Andrew Dunstan]

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


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

Re: [HACKERS] new json funcs

[Laurence Rowe]

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  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@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

Re: [HACKERS] new json funcs

[Andrew Dunstan]

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


diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index c0a75de..d20e0ea 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -10300,6 +10300,136 @@ table2-mapping
json_typeof('-123.4')
number
   
+  
+   
+ 
+  json_build_array
+ 
+ json_build_array(VARIADIC "any")
+   
+   json
+   
+ Builds a heterogeneously typed json array out of a variadic agument list.
+   
+   SELECT json_build_array(1,2,'3',4,5);
+   
+
+ json_build_array
+---
+ [1, 2, "3", 4, 5]
+ 
+   
+  
+  
+   
+ 
+  json_build_object
+ 
+ json_build_object(VARIADIC "any")
+   
+   json
+   
+ Builds a JSON array out of a variadic agument list.  By convention, the object is 
+ constructed out of alternating name/value arguments.
+   
+   SELECT json_build_object('foo',1,'bar',2);
+   
+
+   json_build_object
+
+ {"foo" : 1, "bar" : 2}
+ 
+   
+  
+  
+   
+ 
+  json_object
+ 
+ json_object(text[])
+   
+   json
+   
+ Builds a JSON object out of a text array.  The array must have 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
+ are taken as a name/value pair.
+   
+   select * from json_object('{a, 1, b, "def", c, 3.5}')  or select * from json_object('{{a, 1},{b, "def"},{c, 3.5}}')
+   
+
+  json_object
+---
+ {"a" : "1", "b" : "def", "c" : "3.5"}
+ 
+   
+  
+  
+   
+ json_object(keys text[], values text[])
+   
+   json
+   
+ The two argument form of JSON object takes keys and values pairwise from two separate
+ arrays. In all other respects it is identical to the one argument form.
+   
+   select * from json_object('{a, b}', '{1,2}');
+   
+
+  json_object
+
+ {"a" : "1", "b" : "2"}
+ 
+   
+  
+  
+   
+ 
+  json_to_record
+ 
+ json_to_record(json, nested_as_text bool)
+   
+   record
+   
+ json_to_record returns an arbitrary record from a JSON object.  As with all functions 
+ returning 'record', the caller must explicitly define the structure of the record 
+ when making the call. The input JSON must be an object, not a scalar or an array.
+ If nested_as_text is true, the function coerces nested complex elements to text.
+ Also, see notes below on columns and types.
+   
+   select * from json_to_record('{"a":1,"b":[1,2,3],"c":"bar"}',true) as x(a int, b text, d text) 
+   
+
+ a |b| d 
+---+-+---
+ 1 | [1,2,3] | 
+ 
+   
+  
+  
+   
+ 
+  json_to_recordset
+ 
+ json_to_recordset(json, nested_as_text bool)
+   
+   setof record
+   
+ json_to_recordset returns an arbitrary set of records from a JSON object.  As with 
+ json_to_record, the structure of the record must be explicitly defined when making the
+ call.  However, with json_to_recordset the input JSON must be an array containing 
+ objects.  nested_as_text works as with json_to_record.
+   
+   select * from json_to_recordset('[{"a":1,"b":"foo"},{"a":"2","c":"bar"}]',true) as x(a int, b text);
+   
+
+ a |  b
+---+-
+ 1 | foo
+ 2 |
+ 
+   
+  
  
 

@@ -10326,6 +10456,17 @@ t

Re: [HACKERS] new json funcs

[Andrew Dunstan]

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

diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index 481db16..ef5e125 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -68,6 +68,10 @@ static void array_dim_to_json(StringInfo result, int dim, int ndims, int *dims,
   bool use_line_feeds);
 static void array_to_json_internal(Datum array, StringInfo result,
 	   bool use_line_feeds);
+static void datum_to_json(Datum val, bool is_null, StringInfo result,
+			  TYPCATEGORY tcategory, Oid typoutputfunc, bool key_scalar);
+static void add_json(Datum orig_val, bool is_null, StringInfo result,
+		 Oid val_type, bool key_scalar);
 
 /* the null action object used for pure validation */
 static JsonSemAction nullSemAction =
@@ -1219,7 +1223,7 @@ extract_mb_char(char *s)
  */
 static void
 datum_to_json(Datum val, bool is_null, StringInfo result,
-			  TYPCATEGORY tcategory, Oid typoutputfunc)
+			  TYPCATEGORY tcategory, Oid typoutputfunc, bool key_scalar)
 {
 	char	   *outputstr;
 	text	   *jsontext;
@@ -1241,24 +1245,32 @@ datum_to_json(Datum val, bool is_null, StringInfo result,
 			composite_to_json(val, result, false);
 			break;
 		case TYPCATEGORY_BOOLEAN:
-			if (DatumGetBool(val))
-appendStringInfoString(result, "true");
+			if (!key_scalar)
+appendStringInfoString(result, DatumGetBool(val) ? "true" : "false");
 			else
-appendStringInfoString(result, "false");
+escape_json(result, DatumGetBool(val) ? "true" : "false");
 			break;
 		case TYPCATEGORY_NUMERIC:
 			outputstr = OidOutputFunctionCall(typoutputfunc, val);
-
-			/*
-			 * Don't call escape_json here if it's a valid JSON number.
-			 */
-			dummy_lex.input = *outputstr == '-' ? outputstr + 1 : outputstr;
-			dummy_lex.input_length = strlen(dummy_lex.input);
-			json_lex_number(&dummy_lex, dummy_lex.input, &numeric_error);
-			if (!numeric_error)
-appendStringInfoString(result, outputstr);
-			else
+			if (key_scalar)
+			{
+/* always quote keys */
 escape_json(result, outputstr);
+			}
+			else
+			{
+/*
+ * Don't call escape_json for a non-key if it's a valid JSON
+ * number.
+ */
+dummy_lex.input = *outputstr == '-' ? outputstr + 1 : outputstr;
+dummy_lex.input_length = strlen(dummy_lex.input);
+json_lex_number(&dummy_lex, dummy_lex.input, &numeric_error);
+if (!numeric_error)
+	appendStringInfoString(result, outputstr);
+else
+	escape_json(result, outputstr);
+			}
 			pfree(outputstr);
 			break;
 		case TYPCATEGORY_JSON:
@@ -1276,6 +1288,10 @@ datum_to_json(Datum val, bool is_null, StringInfo result,
 			break;
 		default:
 			outputstr = OidOutputFunctionCall(typoutputfunc, val);
+			if (key_scalar && *outputstr == '\0')
+ereport(ERROR,
+		(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+		 errmsg("key value must not be empty")));
 			escape_json(result, outputstr);
 			pfree(outputstr);
 			break;
@@ -1309,7 +1325,7 @@ array_dim_to_json(StringInfo result, int dim, int ndims, int *dims, Datum *vals,
 		if (dim + 1 == ndims)
 		{
 			datum_to_json(vals[*valcount], nulls[*valcount], result, tcategory,
-		  typoutputfunc);
+		  typoutputfunc, false);
 			(*valcount)++;
 		}
 		else
@@ -1490,13 +1506,85 @@ composite_to_json(Datum composite, StringInfo result, bool use_line_feeds)
 		else
 			tcategory = TypeCategory(tupdesc->attrs[i]->atttypid);
 
-		datum_to_json(val, isnull, result, tcategory, typoutput);
+		datum_to_json(val, isnull, result, tcategory, typoutput, false);
 	}
 
 	appendStringInfoChar(result, '}');
 	ReleaseTupleDesc(tupdesc);
 }
 
+static void
+add_json(Datum orig_val, bool is_null, StringInfo result, Oid val_type, bool key_scalar)
+{
+	Datum		val;
+	TYPCATEGORY tcategory;
+	Oid			typoutput;
+	bool		typisvarlena;
+	Oid			castfunc = InvalidOid;
+
+	if (val_type == InvalidOid)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("could not determine input data type")));
+
+
+	getTypeOutputInfo(val_type, &typoutput, &typisvarlena);
+
+	if (val_type > FirstNormalObjectId)
+	{
+		HeapTuple	tuple;

Re: [HACKERS] new json funcs

[Andrew Dunstan]

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


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

Re: [HACKERS] new json funcs

[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


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

Re: [HACKERS] new json funcs

[Marko Tiikkaja]

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


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

Re: [HACKERS] new json funcs

[Andrew Dunstan]

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

Re: [HACKERS] new json funcs

[Marko Tiikkaja]

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

Re: [HACKERS] new json funcs

[Andrew Dunstan]

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
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index 21a2336..ef5e125 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -68,6 +68,10 @@ static void array_dim_to_json(StringInfo result, int dim, int ndims, int *dims,
   bool use_line_feeds);
 static void array_to_json_internal(Datum array, StringInfo result,
 	   bool use_line_feeds);
+static void datum_to_json(Datum val, bool is_null, StringInfo result,
+			  TYPCATEGORY tcategory, Oid typoutputfunc, bool key_scalar);
+static void add_json(Datum orig_val, bool is_null, StringInfo result,
+		 Oid val_type, bool key_scalar);
 
 /* the null action object used for pure validation */
 static JsonSemAction nullSemAction =
@@ -751,11 +755,12 @@ json_lex_string(JsonLexContext *lex)
  report_json_context(lex)));
 
 	/*
-	 * For UTF8, replace the escape sequence by the actual utf8
-	 * character in lex->strval. Do this also for other encodings
-	 * if the escape designates an ASCII character, otherwise
-	 * raise an error. We don't ever unescape a \u, since that
-	 * would result in an impermissible nul byte.
+	 * For UTF8, replace the escape sequence by the actual
+	 * utf8 character in lex->strval. Do this also for other
+	 * encodings if the escape designates an ASCII character,
+	 * otherwise raise an error. We don't ever unescape a
+	 * \u, since that would result in an impermissible nul
+	 * byte.
 	 */
 
 	if (ch == 0)
@@ -771,8 +776,9 @@ json_lex_string(JsonLexContext *lex)
 	else if (ch <= 0x007f)
 	{
 		/*
-		 * This is the only way to designate things like a form feed
-		 * character in JSON, so it's useful in all encodings.
+		 * This is the only way to designate things like a
+		 * form feed character in JSON, so it's useful in all
+		 * encodings.
 		 */
 		appendStringInfoChar(lex->strval, (char) ch);
 	}
@@ -866,7 +872,7 @@ json_lex_string(JsonLexContext *lex)
 		ereport(ERROR,
 (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
  errmsg("invalid input syntax for type json"),
-		errdetail("Unicode low surrogate must follow a high surrogate."),
+			errdetail("Unicode low surrogate must follow a high surrogate."),
  report_json_context(lex)));
 
 	/* Hooray, we found the end of the string! */
@@ -1217,11 +1223,11 @@ extract_mb_char(char *s)
  */
 static void
 datum_to_json(Datum val, bool is_null, StringInfo result,
-			  TYPCATEGORY tcategory, Oid typoutputfunc)
+			  TYPCATEGORY tcategory, Oid typoutputfunc, bool key_scalar)
 {
 	char	   *outputstr;
 	text	   *jsontext;
-	bool   numeric_error;
+	bool		numeric_error;
 	JsonLexContext dummy_lex;
 
 	if (is_null)
@@ -1239,23 +1245,32 @@ datum_to_json(Datum val, bool is_null, StringInfo result,
 			composite_to_json(val, result, false);
 			break;
 		case TYPCATEGORY_BOOLEAN:
-			if (DatumGetBool(val))
-appendStringInfoString(result, "true");
+			if (!key_scalar)
+appendStringInfoString(result, DatumGetBool(val) ? "true" : "false");
 			else
-appendStringInfoString(result, "false");
+escape_json(result, DatumGetBool(val) ? "true" : "false");
 			break;
 		case TYPCATEGORY_NUMERIC:
 			outputstr = OidOutputFunctionCall(typoutputfunc, val);
-			/*
-			 * Don't call escape_json here if it's a valid JSON number.
-			 */
-			dummy_lex.input = *outputstr == '-' ? outputstr + 1 : outputstr;
-			dummy_lex.input_length = strlen(dummy_lex.input);
-			json_lex_number(&dummy_lex, dummy_lex.input, &numeric_error);
-			if (! numeric_error)
-appendStringInfoString(result, outputstr);
-			else
+			if (key_scalar)
+			{
+/* always qu

Re: [HACKERS] new json funcs

[Andrew Dunstan]

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

diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index 21a2336..7ed771d 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -46,16 +46,16 @@ typedef enum	/* contexts of JSON parser */
 	JSON_PARSE_OBJECT_NEXT,		/* saw object value, expecting ',' or '}' */
 	JSON_PARSE_OBJECT_COMMA,	/* saw object ',', expecting next label */
 	JSON_PARSE_END/* saw the end of a document, expect nothing */
-} JsonParseContext;
+}	JsonParseContext;
 
 static inline void json_lex(JsonLexContext *lex);
 static inline void json_lex_string(JsonLexContext *lex);
 static inline void json_lex_number(JsonLexContext *lex, char *s, bool *num_err);
-static inline void parse_scalar(JsonLexContext *lex, JsonSemAction *sem);
-static void parse_object_field(JsonLexContext *lex, JsonSemAction *sem);
-static void parse_object(JsonLexContext *lex, JsonSemAction *sem);
-static void parse_array_element(JsonLexContext *lex, JsonSemAction *sem);
-static void parse_array(JsonLexContext *lex, JsonSemAction *sem);
+static inline void parse_scalar(JsonLexContext *lex, JsonSemAction * sem);
+static void parse_object_field(JsonLexContext *lex, JsonSemAction * sem);
+static void parse_object(JsonLexContext *lex, JsonSemAction * sem);
+static void parse_array_element(JsonLexContext *lex, JsonSemAction * sem);
+static void parse_array(JsonLexContext *lex, JsonSemAction * sem);
 static void report_parse_error(JsonParseContext ctx, JsonLexContext *lex);
 static void report_invalid_token(JsonLexContext *lex);
 static int	report_json_context(JsonLexContext *lex);
@@ -68,6 +68,10 @@ static void array_dim_to_json(StringInfo result, int dim, int ndims, int *dims,
   bool use_line_feeds);
 static void array_to_json_internal(Datum array, StringInfo result,
 	   bool use_line_feeds);
+static void datum_to_json(Datum val, bool is_null, StringInfo result,
+			  TYPCATEGORY tcategory, Oid typoutputfunc, bool key_scalar);
+static void add_json(Datum orig_val, bool is_null, StringInfo result,
+		 Oid val_type, bool key_scalar);
 
 /* the null action object used for pure validation */
 static JsonSemAction nullSemAction =
@@ -257,7 +261,7 @@ makeJsonLexContext(text *json, bool need_escapes)
  * pointer to a state object to be passed to those routines.
  */
 void
-pg_parse_json(JsonLexContext *lex, JsonSemAction *sem)
+pg_parse_json(JsonLexContext *lex, JsonSemAction * sem)
 {
 	JsonTokenType tok;
 
@@ -293,7 +297,7 @@ pg_parse_json(JsonLexContext *lex, JsonSemAction *sem)
  *	  - object field
  */
 static inline void
-parse_scalar(JsonLexContext *lex, JsonSemAction *sem)
+parse_scalar(JsonLexContext *lex, JsonSemAction * sem)
 {
 	char	   *val = NULL;
 	json_scalar_action sfunc = sem->scalar;
@@ -329,7 +333,7 @@ parse_scalar(JsonLexContext *lex, JsonSemAction *sem)
 }
 
 static void
-parse_object_field(JsonLexContext *lex, JsonSemAction *sem)
+parse_object_field(JsonLexContext *lex, JsonSemAction * sem)
 {
 	/*
 	 * an object field is "fieldname" : value where value can be a scalar,
@@ -377,7 +381,7 @@ parse_object_field(JsonLexContext *lex, JsonSemAction *sem)
 }
 
 static void
-parse_object(JsonLexContext *lex, JsonSemAction *sem)
+parse_object(JsonLexContext *lex, JsonSemAction * sem)
 {
 	/*
 	 * an object is a possibly empty sequence of object fields, separated by
@@ -425,7 +429,7 @@ parse_object(JsonLexContext *lex, JsonSemAction *sem)
 }
 
 static void
-parse_array_element(JsonLexContext *lex, JsonSemAction *sem)
+parse_array_element(JsonLexContext *lex, JsonSemAction * sem)
 {
 	json_aelem_action astart = sem->array_element_start;
 	json_aelem_action aend = sem->array_element_end;
@@ -456,7 +460,7 @@ parse_array_element(JsonLexContext *lex, JsonSemAction *sem)
 }
 
 static void
-parse_array(JsonLexContext *lex, JsonSemAction *sem)
+parse_array(JsonLexContext *lex, JsonSemAction * sem)
 {
 	/*
 	 * an array is a possibl

Re: [HACKERS] new json funcs

[Peter Eisentraut]

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

Re: [HACKERS] new json funcs

[Tom Lane]

Andrew Dunstan  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

Re: [HACKERS] new json funcs

[Andrew Dunstan]

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

Alvaro Herrera  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 <= 
)
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

Re: [HACKERS] new json funcs

[Tom Lane]

Alvaro Herrera  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 <= 
)
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

Re: [HACKERS] new json funcs

[David Fetter]

On Fri, Jan 10, 2014 at 02:39:12PM -0500, Tom Lane wrote:
> Andrew Dunstan  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  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@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

Re: [HACKERS] new json funcs

[Tom Lane]

Andrew Dunstan  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

Re: [HACKERS] new json funcs

[Tom Lane]

Alvaro Herrera  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

Re: [HACKERS] new json funcs

[Andrew Dunstan]

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

Andrew Dunstan  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

Re: [HACKERS] new json funcs

[Alvaro Herrera]

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 Herrerahttp://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

Re: [HACKERS] new json funcs

[Alvaro Herrera]

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 Herrerahttp://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

Re: [HACKERS] new json funcs

[Tom Lane]

Andrew Dunstan  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

Re: [HACKERS] new json funcs

[Andrew Dunstan]

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

Andrew Dunstan  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

Re: [HACKERS] new json funcs

[Tom Lane]

Andrew Dunstan  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

Re: [HACKERS] new json funcs

[Andrew Dunstan]

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

Re: [HACKERS] new json funcs

[Alvaro Herrera]

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

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

-- 
Álvaro Herrerahttp://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