Re: jsonb and nested hstore

Here is the latest set of patches for nested hstore and jsonb.

Because it's so large I've broken this into two patches and compressed
them. The jsonb patch should work standalone. The nested hstore patch
depends on it.

All the jsonb functions now use the jsonb API - there is no more turning
jsonb into text and reparsing it.

At this stage I'm going to be starting cleanup on the jsonb code
(indentation, error messages, comments etc.) as well get getting up some
jsonb docs.

cheers

andrew

On 01/26/2014 05:42 PM, Andrew Dunstan wrote:
>
> Here is the latest set of patches for nested hstore and jsonb.
>
> Because it's so large I've broken this into two patches and compressed
> them. The jsonb patch should work standalone. The nested hstore patch
> depends on it.
>
> All the jsonb functions now use the jsonb API - there is no more
> turning jsonb into text and reparsing it.
>
> At this stage I'm going to be starting cleanup on the jsonb code
> (indentation, error messages, comments etc.) as well get getting up
> some jsonb docs.
>
>
>

Here is an update of the jsonb part of this. Charges:

* there is now documentation for jsonb
* most uses of elog() in json_funcs.c are replaced by ereport().
* indentation fixes and other tidying.

No changes in functionality.

cheers

andrew

On Mon, Jan 27, 2014 at 9:43 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 01/26/2014 05:42 PM, Andrew Dunstan wrote:
>>
>>
>> Here is the latest set of patches for nested hstore and jsonb.
>>
>> Because it's so large I've broken this into two patches and compressed
>> them. The jsonb patch should work standalone. The nested hstore patch
>> depends on it.
>>
>> All the jsonb functions now use the jsonb API - there is no more turning
>> jsonb into text and reparsing it.
>>
>> At this stage I'm going to be starting cleanup on the jsonb code
>> (indentation, error messages, comments etc.) as well get getting up some
>> jsonb docs.
>>
>>
>>
>
>
> Here is an update of the jsonb part of this. Charges:
>
> * there is now documentation for jsonb
> * most uses of elog() in json_funcs.c are replaced by ereport().
> * indentation fixes and other tidying.
>
> No changes in functionality.

Don't have time to fire it up this morning, but a quick scan of the
patch turned up a few minor things:

* see a comment typo, line 290 'jsonn':
* line 332: 'bogus input' -- is this up to error reporting standards?
How about "value 'x' must be one of array, object, numeric, string,
bool"?
* line 357: "jsonb's key could be only a string" prefer non
possessive: jsonb keys must be a string
* line 374, 389: ditto 332
* line 513: is panic appropriate here?
* line 599: ditto
* line 730: odd phrasing in comment, also commenting on this function
is a little light
* line 807: slightly prefer 'with respect to'
* line 888: another PANIC: these maybe correct, seems odd to halt
server on corrupted datum though*
* line 1150: hm, is the jsonb internal hash structure documented?
Aside: why didn't we use standard hash table (performance maybe)?
* line 1805-6: poor phrasing. How about: "it will order and make
unique the hash keys. Otherwise we believe that pushed keys are
ordered and unique. (Don't like verbed 'unqiue').
* line 1860: "no break here: "

merlin

On 01/28/2014 09:38 AM, Merlin Moncure wrote:
> On Mon, Jan 27, 2014 at 9:43 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> On 01/26/2014 05:42 PM, Andrew Dunstan wrote:
>>>
>>> Here is the latest set of patches for nested hstore and jsonb.
>>>
>>> Because it's so large I've broken this into two patches and compressed
>>> them. The jsonb patch should work standalone. The nested hstore patch
>>> depends on it.
>>>
>>> All the jsonb functions now use the jsonb API - there is no more turning
>>> jsonb into text and reparsing it.
>>>
>>> At this stage I'm going to be starting cleanup on the jsonb code
>>> (indentation, error messages, comments etc.) as well get getting up some
>>> jsonb docs.
>>>
>>>
>>>
>>
>> Here is an update of the jsonb part of this. Charges:
>>
>> * there is now documentation for jsonb
>> * most uses of elog() in json_funcs.c are replaced by ereport().
>> * indentation fixes and other tidying.
>>
>> No changes in functionality.
> Don't have time to fire it up this morning, but a quick scan of the
> patch turned up a few minor things:
>
> * see a comment typo, line 290 'jsonn':
> * line 332: 'bogus input' -- is this up to error reporting standards?
> How about "value 'x' must be one of array, object, numeric, string,
> bool"?
> * line 357: "jsonb's key could be only a string" prefer non
> possessive: jsonb keys must be a string
> * line 374, 389: ditto 332
> * line 513: is panic appropriate here?
> * line 599: ditto
> * line 730: odd phrasing in comment, also commenting on this function
> is a little light
> * line 807: slightly prefer 'with respect to'
> * line 888: another PANIC: these maybe correct, seems odd to halt
> server on corrupted datum though*
> * line 1150: hm, is the jsonb internal hash structure documented?
> Aside: why didn't we use standard hash table (performance maybe)?
> * line 1805-6: poor phrasing. How about: "it will order and make
> unique the hash keys. Otherwise we believe that pushed keys are
> ordered and unique. (Don't like verbed 'unqiue').
> * line 1860: "no break here: "
>

Looks like this review is against jsonb-5, not jsonb-6.

cheers

andrew

> Looks like this review is against jsonb-5, not jsonb-6.

oh yep -- shoot, sorry for the noise.

merlin

Andrew Dunstan wrote:

> <para>
> + There are two JSON data types: <type>json</type> and <type>jsonb</type>.
> + Both accept identical sets of values as input. The difference is primarily
> + a matter of efficiency. The <type>json</type> data type stores an exact
> + copy of the the input text, and the processing functions have to reparse
> + it to precess it, while the <type>jsonb</type> is stored in a decomposed
> + form that makes it slightly less efficient to input but very much faster
> + to process, since it never needs reparsing.
> + </para>

typo "precess"
duplicated word "of the the input"

> + </indexterm><indexterm>
> + <primary>jsonb_each</primary>
> + </indexterm><para><literal>json_each(json)</literal>
> + </para><para><literal>jsonb_each(jsonb)</literal>
> + </para></entry>

This SGML nesting is odd and hard to read. Please place opening tags in
separate lines (or at least not immediately following a closing tag). I
am not sure whether the mentions of jsonb_each vs. json_each there are
correct or typos. This also occurs in other places.

> Expands the object in <replaceable>from_json</replaceable> to a row whose columns match
> the record type defined by base. Conversion will be best
> effort; columns in base with no corresponding key in <replaceable>from_json</replaceable>
> - will be left null. If a column is specified more than once, the last value is used.
> + will be left null. When processing <type>json</type>, if a column is
> + specified more than once, the last value is used.

Maybe you also need to specify what happens with jsonb?

> diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
> new file mode 100644
> index 0000000..107ebf0
> --- /dev/null
> +++ b/src/backend/utils/adt/jsonb.c
> @@ -0,0 +1,544 @@
> +/*-------------------------------------------------------------------------
> + *
> + * jsonb.c
> + * I/O for jsonb type
> + *
> + * Portions Copyright (c) 1996-2013, PostgreSQL Global Development Group

2014. Why "Portions", if we don't attribute any portion to UCB?

> + * NOTE. JSONB type is designed to be binary compatible with hstore.
> + *
> + * src/backend/utils/adt/jsonb_support.c

Typo'ed name here.

> +#include "postgres.h"
+
> +#include "libpq/pqformat.h"
> +#include "utils/builtins.h"
> +#include "utils/json.h"
> +#include "utils/jsonapi.h"
> +#include "utils/jsonb.h"

Misplaced prototype?

> +static void recvJsonb(StringInfo buf, JsonbValue *v, uint32 level, uint32 header);

Not sure about the jsonb_1.out file. Is that only due to encoding
differences? What happens if you run it in a completely different
encoding than whatever you tested with? (I would assume Latin-9 and
UTF8) If it fails, then I think you'll end up ripping those tests out,
so probably the _1.out file will have no value at all.

I also wonder if it'd be better to have one large .sql file that
produces the same output in all platforms that tests most of the common
stuff, so that tests that changes output in different platforms can have
smaller alternative expected files.

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

On 01/28/2014 10:50 AM, Alvaro Herrera wrote:
>> + </indexterm><indexterm>
>> + <primary>jsonb_each</primary>
>> + </indexterm><para><literal>json_each(json)</literal>
>> + </para><para><literal>jsonb_each(jsonb)</literal>
>> + </para></entry>
> This SGML nesting is odd and hard to read. Please place opening tags in
> separate lines (or at least not immediately following a closing tag). I
> am not sure whether the mentions of jsonb_each vs. json_each there are
> correct or typos. This also occurs in other places.
>
>

As I understand it, an <entry> tag can only contain block-level elements
like <para> if there are no inline elements (including white space).

If that's not correct I'll change it, but that's what I read here:
<http://oreilly.com/openbook/docbook/book/entry.html>

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 01/28/2014 10:50 AM, Alvaro Herrera wrote:
> + </indexterm><indexterm>
> + <primary>jsonb_each</primary>
> + </indexterm><para><literal>json_each(json)</literal>
> + </para><para><literal>jsonb_each(jsonb)</literal>
> + </para></entry>
>> This SGML nesting is odd and hard to read. Please place opening tags in
>> separate lines (or at least not immediately following a closing tag). I
>> am not sure whether the mentions of jsonb_each vs. json_each there are
>> correct or typos. This also occurs in other places.

> As I understand it, an <entry> tag can only contain block-level elements
> like <para> if there are no inline elements (including white space).

Practically every existing use of <indexterm> is freer than this in its
use of whitespace. It sounds to me like maybe you are trying to put the
<indexterm> inside something it shouldn't go inside of.

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> > On 01/28/2014 10:50 AM, Alvaro Herrera wrote:
> > + </indexterm><indexterm>
> > + <primary>jsonb_each</primary>
> > + </indexterm><para><literal>json_each(json)</literal>
> > + </para><para><literal>jsonb_each(jsonb)</literal>
> > + </para></entry>
> >> This SGML nesting is odd and hard to read. Please place opening tags in
> >> separate lines (or at least not immediately following a closing tag). I
> >> am not sure whether the mentions of jsonb_each vs. json_each there are
> >> correct or typos. This also occurs in other places.
>
> > As I understand it, an <entry> tag can only contain block-level elements
> > like <para> if there are no inline elements (including white space).
>
> Practically every existing use of <indexterm> is freer than this in its
> use of whitespace. It sounds to me like maybe you are trying to put the
> <indexterm> inside something it shouldn't go inside of.

FWIW I was just talking about formatting of the SGML source so that it
is easier to read.

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

On 01/28/2014 11:09 AM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 01/28/2014 10:50 AM, Alvaro Herrera wrote:
>> + </indexterm><indexterm>
>> + <primary>jsonb_each</primary>
>> + </indexterm><para><literal>json_each(json)</literal>
>> + </para><para><literal>jsonb_each(jsonb)</literal>
>> + </para></entry>
>>> This SGML nesting is odd and hard to read. Please place opening tags in
>>> separate lines (or at least not immediately following a closing tag). I
>>> am not sure whether the mentions of jsonb_each vs. json_each there are
>>> correct or typos. This also occurs in other places.
>> As I understand it, an <entry> tag can only contain block-level elements
>> like <para> if there are no inline elements (including white space).
> Practically every existing use of <indexterm> is freer than this in its
> use of whitespace. It sounds to me like maybe you are trying to put the
> <indexterm> inside something it shouldn't go inside of.

The problem is not the indexterm element, it's the space that might
exist outside it. Are we using block level elements like <para> inside
entry elements anywhere else? If not, then your observation is not
relevant. If there are no block level elements then AIUI we can space
things out how we like inside the entry element.

If you can show me how else legally to get a line break inside an entry
element I'm very interested. I tried several things before I found this
way of making it work.

cheers

andrew

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Tom Lane wrote:
>> Practically every existing use of <indexterm> is freer than this in its
>> use of whitespace. It sounds to me like maybe you are trying to put the
>> <indexterm> inside something it shouldn't go inside of.

> FWIW I was just talking about formatting of the SGML source so that it
> is easier to read.

Yeah, me too. I'm just suggesting that maybe Andrew needs to move the
indexterm so that he can format it more readably.

regards, tom lane

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> The problem is not the indexterm element, it's the space that might
> exist outside it. Are we using block level elements like <para> inside
> entry elements anywhere else?

Probably not, and I wonder why you're trying to. Whole paras inside
a table entry (this is a table no?) don't sound like they are going
to lead to nice-looking results.

regards, tom lane

On 01/28/2014 11:27 AM, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> Tom Lane wrote:
>>> Practically every existing use of <indexterm> is freer than this in its
>>> use of whitespace. It sounds to me like maybe you are trying to put the
>>> <indexterm> inside something it shouldn't go inside of.
>> FWIW I was just talking about formatting of the SGML source so that it
>> is easier to read.
> Yeah, me too. I'm just suggesting that maybe Andrew needs to move the
> indexterm so that he can format it more readably.
>
>

Hmm. Maybe I could put them inside the para elements. So we'd have:

<entry><para>
<indexterm>
</indexterm>
para text
</para><para>
<indexterm>
</indexterm>
para text
</para></entry>

cheers

andrew

On 01/28/2014 11:29 AM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> The problem is not the indexterm element, it's the space that might
>> exist outside it. Are we using block level elements like <para> inside
>> entry elements anywhere else?
> Probably not, and I wonder why you're trying to. Whole paras inside
> a table entry (this is a table no?) don't sound like they are going
> to lead to nice-looking results.

See <http://developer.postgresql.org/~adunstan/functions-json.html>

cheers

andrew

On Tue, Jan 28, 2014 at 10:46 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 01/28/2014 11:29 AM, Tom Lane wrote:
>>
>> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>>
>>> The problem is not the indexterm element, it's the space that might
>>> exist outside it. Are we using block level elements like <para> inside
>>> entry elements anywhere else?
>>
>> Probably not, and I wonder why you're trying to. Whole paras inside
>> a table entry (this is a table no?) don't sound like they are going
>> to lead to nice-looking results.
>
> See <http://developer.postgresql.org/~adunstan/functions-json.html>

yeah. note: I think the json documentation needs *major* overhaul. too
much is going in inside the function listings where there really
should be a big breakout discussing the "big picture" of json/jsonb
with examples of various use cases. I want to give it a shot but
unfortunately can not commit to do that by the end of the 'fest.

merlin

On 01/28/2014 09:58 AM, Merlin Moncure wrote:
> yeah. note: I think the json documentation needs *major* overhaul. too
> much is going in inside the function listings where there really
> should be a big breakout discussing the "big picture" of json/jsonb
> with examples of various use cases. I want to give it a shot but
> unfortunately can not commit to do that by the end of the 'fest.

FWIW, I've promised Andrew that I'll overhaul this by the end of beta.
Given that we have all of beta for doc refinements.

In addition to this, the JSON vs JSONB datatype page really needs
expansion and clarification.

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

On Tue, Jan 28, 2014 at 12:09 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 01/28/2014 09:58 AM, Merlin Moncure wrote:
>> yeah. note: I think the json documentation needs *major* overhaul. too
>> much is going in inside the function listings where there really
>> should be a big breakout discussing the "big picture" of json/jsonb
>> with examples of various use cases. I want to give it a shot but
>> unfortunately can not commit to do that by the end of the 'fest.
>
> FWIW, I've promised Andrew that I'll overhaul this by the end of beta.
> Given that we have all of beta for doc refinements.
>
> In addition to this, the JSON vs JSONB datatype page really needs
> expansion and clarification.

right: exactly. I'd be happy to help (such as I can) ...I wanted to
see if jsonb to make it in on this 'fest (doc issues notwithstanding);
it hasn't been formally reviewed yet AFAICT. So my thinking here is
to get docs to minimum acceptable standards in the short term and
focus on the structural code issues for the 'fest (if jsonb slips then
it's moot obviously).

merlin

On 01/28/2014 10:29 AM, Merlin Moncure wrote:
>> In addition to this, the JSON vs JSONB datatype page really needs
>> expansion and clarification.
>
> right: exactly. I'd be happy to help (such as I can) ...I wanted to
> see if jsonb to make it in on this 'fest (doc issues notwithstanding);
> it hasn't been formally reviewed yet AFAICT. So my thinking here is
> to get docs to minimum acceptable standards in the short term and
> focus on the structural code issues for the 'fest (if jsonb slips then
> it's moot obviously).

Well, having reviewed the docs before Andrew sent them in, I felt they
already *were* "minimum acceptable". Certainly they're as complete as
the original JSON docs were.

Or is this just about whitespace and line breaks?

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

Josh Berkus escribió:

> Or is this just about whitespace and line breaks?

If the docs are going to be rehauled, please ignore my whitespace
comments.

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

On 01/28/2014 10:56 AM, Alvaro Herrera wrote:
> Josh Berkus escribió:
>
>> Or is this just about whitespace and line breaks?
>
> If the docs are going to be rehauled, please ignore my whitespace
> comments.

I'm sure you'll find plenty to criticize in my version. ;-)

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

On 01/27/2014 10:43 PM, Andrew Dunstan wrote:
>
> On 01/26/2014 05:42 PM, Andrew Dunstan wrote:
>>
>> Here is the latest set of patches for nested hstore and jsonb.
>>
>> Because it's so large I've broken this into two patches and
>> compressed them. The jsonb patch should work standalone. The nested
>> hstore patch depends on it.
>>
>> All the jsonb functions now use the jsonb API - there is no more
>> turning jsonb into text and reparsing it.
>>
>> At this stage I'm going to be starting cleanup on the jsonb code
>> (indentation, error messages, comments etc.) as well get getting up
>> some jsonb docs.
>>
>>
>>
>
>
> Here is an update of the jsonb part of this. Charges:
>
> * there is now documentation for jsonb
> * most uses of elog() in json_funcs.c are replaced by ereport().
> * indentation fixes and other tidying.
>
> No changes in functionality.
>

Further update of jsonb portion.

Only change in functionality is the addition of casts between jsonb and
json.

The other changes are the merge with the new json functions code, and
rearrangement of the docs changes to make them less ugly. Essentially I
moved the indexterm tags right out of the table as is done in some other
parts pf the docs. That makes the entry tags much clearer to read.

cheers

andrew

On Wed, Jan 29, 2014 at 12:03 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> Only change in functionality is the addition of casts between jsonb and
> json.
>
> The other changes are the merge with the new json functions code, and
> rearrangement of the docs changes to make them less ugly. Essentially I
> moved the indexterm tags right out of the table as is done in some other
> parts pf the docs. That makes the entry tags much clearer to read.

I think the opening paragraphs contrasting json/jsonb be needs
refinement. json is going to be slightly faster than jsonb for input
*and* output. For example, in one application I store fairly large
json objects containing pre-compiled static polygon data that is
simply flipped up to google maps. This case will likely be pessimal
for jsonb. For the next paragaph, I'd like to expand it a bit on
'specialized needs' and boil it down to specific uses cases.
Basically, json will likely be more compact in most cases and slightly
faster for input/output; jsonb would be preferred in any context
where processing, or searching or extensive server side parsing is
employed.

If you agree, I'd be happy to do that...

merlin

On 01/29/2014 12:46 PM, Merlin Moncure wrote:
> I think the opening paragraphs contrasting json/jsonb be needs
> refinement. json is going to be slightly faster than jsonb for input
> *and* output. For example, in one application I store fairly large
> json objects containing pre-compiled static polygon data that is
> simply flipped up to google maps. This case will likely be pessimal
> for jsonb. For the next paragaph, I'd like to expand it a bit on
> 'specialized needs' and boil it down to specific uses cases.
> Basically, json will likely be more compact in most cases and slightly
> faster for input/output; jsonb would be preferred in any context
> where processing, or searching or extensive server side parsing is
> employed.
>
> If you agree, I'd be happy to do that...

Please take a stab at it, I'll be happy to revise it.

I was working on doing a two-column table comparison chart; I still
think that's the best way to go.

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

On 01/29/2014 01:03 PM, Andrew Dunstan wrote:
>
> On 01/27/2014 10:43 PM, Andrew Dunstan wrote:
>>
>> On 01/26/2014 05:42 PM, Andrew Dunstan wrote:
>>>
>>> Here is the latest set of patches for nested hstore and jsonb.
>>>
>>> Because it's so large I've broken this into two patches and
>>> compressed them. The jsonb patch should work standalone. The nested
>>> hstore patch depends on it.
>>>
>>> All the jsonb functions now use the jsonb API - there is no more
>>> turning jsonb into text and reparsing it.
>>>
>>> At this stage I'm going to be starting cleanup on the jsonb code
>>> (indentation, error messages, comments etc.) as well get getting up
>>> some jsonb docs.
>>>
>>>
>>>
>>
>>
>> Here is an update of the jsonb part of this. Charges:
>>
>> * there is now documentation for jsonb
>> * most uses of elog() in json_funcs.c are replaced by ereport().
>> * indentation fixes and other tidying.
>>
>> No changes in functionality.
>>
>
>
> Further update of jsonb portion.
>
> Only change in functionality is the addition of casts between jsonb
> and json.
>
> The other changes are the merge with the new json functions code, and
> rearrangement of the docs changes to make them less ugly. Essentially
> I moved the indexterm tags right out of the table as is done in some
> other parts pf the docs. That makes the entry tags much clearer to read.
>
>
>

Updated to apply cleanly after recent commits.

cheers

andrew

On Wed, Jan 29, 2014 at 3:56 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 01/29/2014 01:03 PM, Andrew Dunstan wrote:
>>
>>
>> On 01/27/2014 10:43 PM, Andrew Dunstan wrote:
>>>
>>>
>>> On 01/26/2014 05:42 PM, Andrew Dunstan wrote:
>>>>
>>>>
>>>> Here is the latest set of patches for nested hstore and jsonb.
>>>>
>>>> Because it's so large I've broken this into two patches and compressed
>>>> them. The jsonb patch should work standalone. The nested hstore patch
>>>> depends on it.
>>>>
>>>> All the jsonb functions now use the jsonb API - there is no more turning
>>>> jsonb into text and reparsing it.
>>>>
>>>> At this stage I'm going to be starting cleanup on the jsonb code
>>>> (indentation, error messages, comments etc.) as well get getting up some
>>>> jsonb docs.
>>>>
>>>>
>>>>
>>>
>>>
>>> Here is an update of the jsonb part of this. Charges:
>>>
>>> * there is now documentation for jsonb
>>> * most uses of elog() in json_funcs.c are replaced by ereport().
>>> * indentation fixes and other tidying.
>>>
>>> No changes in functionality.
>>>
>>
>>
>> Further update of jsonb portion.
>>
>> Only change in functionality is the addition of casts between jsonb and
>> json.
>>
>> The other changes are the merge with the new json functions code, and
>> rearrangement of the docs changes to make them less ugly. Essentially I
>> moved the indexterm tags right out of the table as is done in some other
>> parts pf the docs. That makes the entry tags much clearer to read.
>
> Updated to apply cleanly after recent commits.

ok, great. This is really fabulous. So far most everything feels
natural and good.

I see something odd in terms of the jsonb use case coverage. One of
the major headaches with json deserialization presently is that
there's no easy way to easily move a complex (record- or array-
containing) json structure into a row object. For example,

create table bar(a int, b int[]);
postgres=# select jsonb_populate_record(null::bar, '{"a": 1, "b":
[1,2]}'::jsonb, false);
ERROR: cannot populate with a nested object unless use_json_as_text is true

If find the use_json_as_text argument here to be pretty useless
(unlike in the json_build to_record variants where it least provides
some hope for an escape hatch) for handling this since it will just
continue to fail:

postgres=# select jsonb_populate_record(null::bar, '{"a": 1, "b":
[1,2]}'::jsonb, true);
ERROR: missing "]" in array dimensions

OTOH, the nested hstore handles this no questions asked:

postgres=# select * from populate_record(null::bar, '"a"=>1,
"b"=>{1,2}'::hstore);
a | b
---+-------
1 | {1,2}

So, if you need to convert a complex json to a row type, the only
effective way to do that is like this:
postgres=# select* from populate_record(null::bar, '{"a": 1, "b":
[1,2]}'::json::hstore);
a | b
---+-------
1 | {1,2}

Not a big deal really. But it makes me wonder (now that we have the
internal capability of properly mapping to a record) why *both* the
json/jsonb populate record variants shouldn't point to what the nested
hstore behavior is when the 'as_text' flag is false. That would
demolish the error and remove the dependency on hstore in order to do
effective rowtype mapping. In an ideal world the json_build
'to_record' variants would behave similarly I think although there's
no existing hstore analog so I'm assuming it's a non-trival amount of
work.

Now, if we're agreed on that, I then also wonder if the 'as_text'
argument needs to exist at all for the populate functions except for
backwards compatibility on the json side (not jsonb). For non-complex
structures it does best effort casting anyways so the flag is moot.

merlin

On 01/29/2014 05:37 PM, Merlin Moncure wrote:
> On Wed, Jan 29, 2014 at 3:56 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> On 01/29/2014 01:03 PM, Andrew Dunstan wrote:
>>>
>>> On 01/27/2014 10:43 PM, Andrew Dunstan wrote:
>>>>
>>>> On 01/26/2014 05:42 PM, Andrew Dunstan wrote:
>>>>>
>>>>> Here is the latest set of patches for nested hstore and jsonb.
>>>>>
>>>>> Because it's so large I've broken this into two patches and compressed
>>>>> them. The jsonb patch should work standalone. The nested hstore patch
>>>>> depends on it.
>>>>>
>>>>> All the jsonb functions now use the jsonb API - there is no more turning
>>>>> jsonb into text and reparsing it.
>>>>>
>>>>> At this stage I'm going to be starting cleanup on the jsonb code
>>>>> (indentation, error messages, comments etc.) as well get getting up some
>>>>> jsonb docs.
>>>>>
>>>>>
>>>>>
>>>>
>>>> Here is an update of the jsonb part of this. Charges:
>>>>
>>>> * there is now documentation for jsonb
>>>> * most uses of elog() in json_funcs.c are replaced by ereport().
>>>> * indentation fixes and other tidying.
>>>>
>>>> No changes in functionality.
>>>>
>>>
>>> Further update of jsonb portion.
>>>
>>> Only change in functionality is the addition of casts between jsonb and
>>> json.
>>>
>>> The other changes are the merge with the new json functions code, and
>>> rearrangement of the docs changes to make them less ugly. Essentially I
>>> moved the indexterm tags right out of the table as is done in some other
>>> parts pf the docs. That makes the entry tags much clearer to read.
>> Updated to apply cleanly after recent commits.
> ok, great. This is really fabulous. So far most everything feels
> natural and good.
>
> I see something odd in terms of the jsonb use case coverage. One of
> the major headaches with json deserialization presently is that
> there's no easy way to easily move a complex (record- or array-
> containing) json structure into a row object. For example,
>
> create table bar(a int, b int[]);
> postgres=# select jsonb_populate_record(null::bar, '{"a": 1, "b":
> [1,2]}'::jsonb, false);
> ERROR: cannot populate with a nested object unless use_json_as_text is true
>
> If find the use_json_as_text argument here to be pretty useless
> (unlike in the json_build to_record variants where it least provides
> some hope for an escape hatch) for handling this since it will just
> continue to fail:
>
> postgres=# select jsonb_populate_record(null::bar, '{"a": 1, "b":
> [1,2]}'::jsonb, true);
> ERROR: missing "]" in array dimensions
>
> OTOH, the nested hstore handles this no questions asked:
>
> postgres=# select * from populate_record(null::bar, '"a"=>1,
> "b"=>{1,2}'::hstore);
> a | b
> ---+-------
> 1 | {1,2}
>
> So, if you need to convert a complex json to a row type, the only
> effective way to do that is like this:
> postgres=# select* from populate_record(null::bar, '{"a": 1, "b":
> [1,2]}'::json::hstore);
> a | b
> ---+-------
> 1 | {1,2}
>
> Not a big deal really. But it makes me wonder (now that we have the
> internal capability of properly mapping to a record) why *both* the
> json/jsonb populate record variants shouldn't point to what the nested
> hstore behavior is when the 'as_text' flag is false. That would
> demolish the error and remove the dependency on hstore in order to do
> effective rowtype mapping. In an ideal world the json_build
> 'to_record' variants would behave similarly I think although there's
> no existing hstore analog so I'm assuming it's a non-trival amount of
> work.
>
> Now, if we're agreed on that, I then also wonder if the 'as_text'
> argument needs to exist at all for the populate functions except for
> backwards compatibility on the json side (not jsonb). For non-complex
> structures it does best effort casting anyways so the flag is moot.
>

Well, I could certainly look at making the populate_record{set} and
to_record{set} logic handle types that are arrays or composites inside
the record. It might not be terribly hard to do - not sure.

cheers

andrew

On 01/29/2014 02:37 PM, Merlin Moncure wrote:
> create table bar(a int, b int[]);
> postgres=# select jsonb_populate_record(null::bar, '{"a": 1, "b":
> [1,2]}'::jsonb, false);
> ERROR: cannot populate with a nested object unless use_json_as_text is true

Hmmm. What about just making any impossibly complex objects type JSON?

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

>> ok, great. This is really fabulous. So far most everything feels
>> natural and good.
>>
>> I see something odd in terms of the jsonb use case coverage. One of
>> the major headaches with json deserialization presently is that
>> there's no easy way to easily move a complex (record- or array-
>> containing) json structure into a row object. For example,
>>
>> create table bar(a int, b int[]);
>> postgres=# select jsonb_populate_record(null::bar, '{"a": 1, "b":
>> [1,2]}'::jsonb, false);
>> ERROR: cannot populate with a nested object unless use_json_as_text
>> is true
>>
>> If find the use_json_as_text argument here to be pretty useless
>> (unlike in the json_build to_record variants where it least provides
>> some hope for an escape hatch) for handling this since it will just
>> continue to fail:
>>
>> postgres=# select jsonb_populate_record(null::bar, '{"a": 1, "b":
>> [1,2]}'::jsonb, true);
>> ERROR: missing "]" in array dimensions
>>
>> OTOH, the nested hstore handles this no questions asked:
>>
>> postgres=# select * from populate_record(null::bar, '"a"=>1,
>> "b"=>{1,2}'::hstore);
>> a | b
>> ---+-------
>> 1 | {1,2}
>>
>> So, if you need to convert a complex json to a row type, the only
>> effective way to do that is like this:
>> postgres=# select* from populate_record(null::bar, '{"a": 1, "b":
>> [1,2]}'::json::hstore);
>> a | b
>> ---+-------
>> 1 | {1,2}
>>
>> Not a big deal really. But it makes me wonder (now that we have the
>> internal capability of properly mapping to a record) why *both* the
>> json/jsonb populate record variants shouldn't point to what the nested
>> hstore behavior is when the 'as_text' flag is false. That would
>> demolish the error and remove the dependency on hstore in order to do
>> effective rowtype mapping. In an ideal world the json_build
>> 'to_record' variants would behave similarly I think although there's
>> no existing hstore analog so I'm assuming it's a non-trival amount of
>> work.
>>
>> Now, if we're agreed on that, I then also wonder if the 'as_text'
>> argument needs to exist at all for the populate functions except for
>> backwards compatibility on the json side (not jsonb). For non-complex
>> structures it does best effort casting anyways so the flag is moot.
>>
>
> Well, I could certainly look at making the populate_record{set} and
> to_record{set} logic handle types that are arrays or composites inside
> the record. It might not be terribly hard to do - not sure.
>
>

A quick analysis suggests that this is fixable with fairly minimal
disturbance in the jsonb case. In the json case it would probably
involve reparsing the inner json. That's probably doable, because the
routines are all reentrant, but not likely to be terribly efficient. It
will also be a deal more work.

cheers

andrew

On Thu, Jan 30, 2014 at 9:50 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>>> Now, if we're agreed on that, I then also wonder if the 'as_text'
>>> argument needs to exist at all for the populate functions except for
>>> backwards compatibility on the json side (not jsonb). For non-complex
>>> structures it does best effort casting anyways so the flag is moot.
>>>
>>
>> Well, I could certainly look at making the populate_record{set} and
>> to_record{set} logic handle types that are arrays or composites inside the
>> record. It might not be terribly hard to do - not sure.
>
> A quick analysis suggests that this is fixable with fairly minimal
> disturbance in the jsonb case. In the json case it would probably involve
> reparsing the inner json. That's probably doable, because the routines are
> all reentrant, but not likely to be terribly efficient. It will also be a
> deal more work.

Right. Also the text json functions are already in the wild anyways
-- that's not in the scope of this patch so if they need to be fixed
that could be done later.

ISTM then the right course of action is to point jsonb 'populate'
variants at hstore implementation, not the text json one and remove
the 'as text' argument. Being able to ditch that argument is the main
reason why I think this should be handled now (not forcing hstore
dependency to handle complex json is gravy).

People handling json as text would then invoke a ::jsonb cast trading
off performance for flexibility which is perfectly fine. If you
agree, perhaps we can HINT the error in certain places that return
"ERROR: cannot call json_populate_record on a nested object" that the
jsonb variant can be used as a workaround.

merlin

On 01/30/2014 12:34 PM, Merlin Moncure wrote:
> On Thu, Jan 30, 2014 at 9:50 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>>>> Now, if we're agreed on that, I then also wonder if the 'as_text'
>>>> argument needs to exist at all for the populate functions except for
>>>> backwards compatibility on the json side (not jsonb). For non-complex
>>>> structures it does best effort casting anyways so the flag is moot.
>>>>
>>> Well, I could certainly look at making the populate_record{set} and
>>> to_record{set} logic handle types that are arrays or composites inside the
>>> record. It might not be terribly hard to do - not sure.
>> A quick analysis suggests that this is fixable with fairly minimal
>> disturbance in the jsonb case. In the json case it would probably involve
>> reparsing the inner json. That's probably doable, because the routines are
>> all reentrant, but not likely to be terribly efficient. It will also be a
>> deal more work.
> Right. Also the text json functions are already in the wild anyways
> -- that's not in the scope of this patch so if they need to be fixed
> that could be done later.
>
> ISTM then the right course of action is to point jsonb 'populate'
> variants at hstore implementation, not the text json one and remove
> the 'as text' argument. Being able to ditch that argument is the main
> reason why I think this should be handled now (not forcing hstore
> dependency to handle complex json is gravy).

We can't reference any hstore code in jsonb. There is no guarantee that
hstore will even be loaded.

We'd have to move that code from hstore to jsonb_support.c and then make
hstore refer to it.

cheers

andrew

On 01/30/2014 06:45 PM, Andrew Dunstan wrote:
>
> On 01/30/2014 12:34 PM, Merlin Moncure wrote:
>> On Thu, Jan 30, 2014 at 9:50 AM, Andrew Dunstan <andrew(at)dunslane(dot)net>
>> wrote:
>>>>> Now, if we're agreed on that, I then also wonder if the 'as_text'
>>>>> argument needs to exist at all for the populate functions except for
>>>>> backwards compatibility on the json side (not jsonb). For
>>>>> non-complex
>>>>> structures it does best effort casting anyways so the flag is moot.
>>>>>
>>>> Well, I could certainly look at making the populate_record{set} and
>>>> to_record{set} logic handle types that are arrays or composites
>>>> inside the
>>>> record. It might not be terribly hard to do - not sure.
>>> A quick analysis suggests that this is fixable with fairly minimal
>>> disturbance in the jsonb case.
As row_to_json() works with arbitrarily complex nested types (for
example row having a field
of type array of another (table)type containing arrays of third type) it
would be really nice if
you can get the result back into that row without too much hassle.

and it should be ok to treat json as "source type" and require it to be
translated to jsonb
for more complex operations
>>> In the json case it would probably involve
>>> reparsing the inner json. That's probably doable, because the
>>> routines are
>>> all reentrant, but not likely to be terribly efficient. It will also
>>> be a
>>> deal more work.
>> Right. Also the text json functions are already in the wild anyways
>> -- that's not in the scope of this patch so if they need to be fixed
>> that could be done later.
>>
>> ISTM then the right course of action is to point jsonb 'populate'
>> variants at hstore implementation, not the text json one and remove
>> the 'as text' argument. Being able to ditch that argument is the main
>> reason why I think this should be handled now (not forcing hstore
>> dependency to handle complex json is gravy).
>
>
> We can't reference any hstore code in jsonb. There is no guarantee
> that hstore will even be loaded.
>
> We'd have to move that code from hstore to jsonb_support.c and then
> make hstore refer to it.
Or just copy it and leave hstore alone - the code duplication is not
terribly huge
here and hstore might still want to develop independently.

Cheers

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On 01/30/2014 01:03 PM, Hannu Krosing wrote:
> On 01/30/2014 06:45 PM, Andrew Dunstan wrote:
>> On 01/30/2014 12:34 PM, Merlin Moncure wrote:
>>> On Thu, Jan 30, 2014 at 9:50 AM, Andrew Dunstan <andrew(at)dunslane(dot)net>
>>> wrote:
>>>>>> Now, if we're agreed on that, I then also wonder if the 'as_text'
>>>>>> argument needs to exist at all for the populate functions except for
>>>>>> backwards compatibility on the json side (not jsonb). For
>>>>>> non-complex
>>>>>> structures it does best effort casting anyways so the flag is moot.
>>>>>>
>>>>> Well, I could certainly look at making the populate_record{set} and
>>>>> to_record{set} logic handle types that are arrays or composites
>>>>> inside the
>>>>> record. It might not be terribly hard to do - not sure.

>>>> A quick analysis suggests that this is fixable with fairly minimal
>>>> disturbance in the jsonb case.
> As row_to_json() works with arbitrarily complex nested types (for
> example row having a field
> of type array of another (table)type containing arrays of third type) it
> would be really nice if
> you can get the result back into that row without too much hassle.
>
> and it should be ok to treat json as "source type" and require it to be
> translated to jsonb
> for more complex operations

Might be possible.

>>>> In the json case it would probably involve
>>>> reparsing the inner json. That's probably doable, because the
>>>> routines are
>>>> all reentrant, but not likely to be terribly efficient. It will also
>>>> be a
>>>> deal more work.
>>> Right. Also the text json functions are already in the wild anyways
>>> -- that's not in the scope of this patch so if they need to be fixed
>>> that could be done later.
>>>
>>> ISTM then the right course of action is to point jsonb 'populate'
>>> variants at hstore implementation, not the text json one and remove
>>> the 'as text' argument. Being able to ditch that argument is the main
>>> reason why I think this should be handled now (not forcing hstore
>>> dependency to handle complex json is gravy).
>>
>> We can't reference any hstore code in jsonb. There is no guarantee
>> that hstore will even be loaded.
>>
>> We'd have to move that code from hstore to jsonb_support.c and then
>> make hstore refer to it.
> Or just copy it and leave hstore alone - the code duplication is not
> terribly huge
> here and hstore might still want to develop independently.
>

We have gone to great deal of trouble to make jsonb and nested hstore
more or less incarnations of the same thing. The new hstore relies
heavily on the new jsonb. So what you're suggesting is the opposite of
what's been developed these last months.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 01/30/2014 01:03 PM, Hannu Krosing wrote:
>> On 01/30/2014 06:45 PM, Andrew Dunstan wrote:
>>> We'd have to move that code from hstore to jsonb_support.c and then
>>> make hstore refer to it.

>> Or just copy it and leave hstore alone - the code duplication is not
>> terribly huge here and hstore might still want to develop independently.

> We have gone to great deal of trouble to make jsonb and nested hstore
> more or less incarnations of the same thing. The new hstore relies
> heavily on the new jsonb. So what you're suggesting is the opposite of
> what's been developed these last months.

If so, why would you be resistant to pushing more code out of hstore
and into jsonb?

regards, tom lane

On 01/30/2014 01:50 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 01/30/2014 01:03 PM, Hannu Krosing wrote:
>>> On 01/30/2014 06:45 PM, Andrew Dunstan wrote:
>>>> We'd have to move that code from hstore to jsonb_support.c and then
>>>> make hstore refer to it.
>>> Or just copy it and leave hstore alone - the code duplication is not
>>> terribly huge here and hstore might still want to develop independently.
>> We have gone to great deal of trouble to make jsonb and nested hstore
>> more or less incarnations of the same thing. The new hstore relies
>> heavily on the new jsonb. So what you're suggesting is the opposite of
>> what's been developed these last months.
> If so, why would you be resistant to pushing more code out of hstore
> and into jsonb?
>

I'm not. Above I suggested exactly that. I was simply opposed to Hannu's
suggestion that instead of making hstore refer to the adopted code we
maintain two copies of code that does essentially the same thing.

cheers

andrew

On 01/29/2014 04:56 PM, Andrew Dunstan wrote:
>
> On 01/29/2014 01:03 PM, Andrew Dunstan wrote:
>>
>> On 01/27/2014 10:43 PM, Andrew Dunstan wrote:
>>>
>>> On 01/26/2014 05:42 PM, Andrew Dunstan wrote:
>>>>
>>>> Here is the latest set of patches for nested hstore and jsonb.
>>>>
>>>> Because it's so large I've broken this into two patches and
>>>> compressed them. The jsonb patch should work standalone. The nested
>>>> hstore patch depends on it.
>>>>
>>>> All the jsonb functions now use the jsonb API - there is no more
>>>> turning jsonb into text and reparsing it.
>>>>
>>>> At this stage I'm going to be starting cleanup on the jsonb code
>>>> (indentation, error messages, comments etc.) as well get getting up
>>>> some jsonb docs.
>>>>
>>>>
>>>>
>>>
>>>
>>> Here is an update of the jsonb part of this. Charges:
>>>
>>> * there is now documentation for jsonb
>>> * most uses of elog() in json_funcs.c are replaced by ereport().
>>> * indentation fixes and other tidying.
>>>
>>> No changes in functionality.
>>>
>>
>>
>> Further update of jsonb portion.
>>
>> Only change in functionality is the addition of casts between jsonb
>> and json.
>>
>> The other changes are the merge with the new json functions code, and
>> rearrangement of the docs changes to make them less ugly. Essentially
>> I moved the indexterm tags right out of the table as is done in some
>> other parts pf the docs. That makes the entry tags much clearer to read.
>>
>>
>>
>
>
> Updated to apply cleanly after recent commits.
>
>

Updated patches for both pieces. Included is some tidying done by
Teodor, and fixes for remaining whitespace issues. This now passes "git
diff --check master" cleanly for me.

cheers

andrew

On Thu, January 30, 2014 20:07, Andrew Dunstan wrote:
>
> Updated patches for both pieces. Included is some tidying done by
>
> [ nested-hstore-9.patch.gz ]

Here is a small doc-patch to Table F-6. hstore Operators

It corrects its booleans in the 'Result' column ( t and f instead of true and false ).

Thanks,

Erik Rijkers

On Thu, January 30, 2014 23:15, Erik Rijkers wrote:
> On Thu, January 30, 2014 20:07, Andrew Dunstan wrote:
>>
>> Updated patches for both pieces. Included is some tidying done by
>>
>> [ nested-hstore-9.patch.gz ]
>
> Here is a small doc-patch to Table F-6. hstore Operators
>
> It corrects its booleans in the 'Result' column ( t and f instead of true and false ).

I mean, here it is...

On Thu, Jan 30, 2014 at 1:07 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 01/29/2014 04:56 PM, Andrew Dunstan wrote:
>>
>>
>> On 01/29/2014 01:03 PM, Andrew Dunstan wrote:
>>>
>>>
>>> On 01/27/2014 10:43 PM, Andrew Dunstan wrote:
>>>>
>>>>
>>>> On 01/26/2014 05:42 PM, Andrew Dunstan wrote:
>>>>>
>>>>>
>>>>> Here is the latest set of patches for nested hstore and jsonb.
>>>>>
>>>>> Because it's so large I've broken this into two patches and compressed
>>>>> them. The jsonb patch should work standalone. The nested hstore patch
>>>>> depends on it.
>>>>>
>>>>> All the jsonb functions now use the jsonb API - there is no more
>>>>> turning jsonb into text and reparsing it.
>>>>>
>>>>> At this stage I'm going to be starting cleanup on the jsonb code
>>>>> (indentation, error messages, comments etc.) as well get getting up some
>>>>> jsonb docs.
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> Here is an update of the jsonb part of this. Charges:
>>>>
>>>> * there is now documentation for jsonb
>>>> * most uses of elog() in json_funcs.c are replaced by ereport().
>>>> * indentation fixes and other tidying.
>>>>
>>>> No changes in functionality.
>>>>
>>>
>>>
>>> Further update of jsonb portion.
>>>
>>> Only change in functionality is the addition of casts between jsonb and
>>> json.
>>>
>>> The other changes are the merge with the new json functions code, and
>>> rearrangement of the docs changes to make them less ugly. Essentially I
>>> moved the indexterm tags right out of the table as is done in some other
>>> parts pf the docs. That makes the entry tags much clearer to read.
>>>
>>>
>>>
>>
>>
>> Updated to apply cleanly after recent commits.
>>
>>
>
> Updated patches for both pieces. Included is some tidying done by Teodor,
> and fixes for remaining whitespace issues. This now passes "git diff --check
> master" cleanly for me.

Something seems off:

postgres=# create type z as (a int, b int[]);
CREATE TYPE
postgres=# create type y as (a int, b z[]);
CREATE TYPE
postgres=# create type x as (a int, b y[]);
CREATE TYPE

-- test a complicated construction
postgres=# select row(1, array[row(1, array[row(1, array[1,2])::z])::y])::x;
row
-------------------------------------------------------------------------------------
(1,"{""(1,\\""{\\""\\""(1,\\\\\\\\\\""\\""{1,2}\\\\\\\\\\""\\"")\\""\\""}\\"")""}")

postgres=# select hstore(row(1, array[row(1, array[row(1,
array[1,2])::z])::y])::x);
hstore
----------------------------------------------------------------------------------------------
"a"=>1, "b"=>"{\"(1,\\\"{\\\"\\\"(1,\\\\\\\\\\\"\\\"{1,2}\\\\\\\\\\\"\\\")\\\"\\\"}\\\")\"}"

here, the output escaping has leaked into the internal array
structures. istm we should have a json expressing the internal
structure. It does (weirdly) map back however:

postgres=# select populate_record(null::x, hstore(row(1, array[row(1,
array[row(1, array[1,2])::z])::y])::x));
populate_record
-------------------------------------------------------------------------------------
(1,"{""(1,\\""{\\""\\""(1,\\\\\\\\\\""\\""{1,2}\\\\\\\\\\""\\"")\\""\\""}\\"")""}")

OTOH, if I go via json route:

postgres=# select row_to_json(row(1, array[row(1, array[row(1,
array[1,2])::z])::y])::x);
row_to_json
-----------------------------------------------
{"a":1,"b":[{"a":1,"b":[{"a":1,"b":[1,2]}]}]}

so far, so good. let's push to hstore:
postgres=# select row_to_json(row(1, array[row(1, array[row(1,
array[1,2])::z])::y])::x)::jsonb::hstore;
row_to_json
-------------------------------------------------------
"a"=>1, "b"=>[{"a"=>1, "b"=>[{"a"=>1, "b"=>[1, 2]}]}]

this ISTM is the 'right' behavior. but what if we bring it back to
record object?

postgres=# select populate_record(null::x, row_to_json(row(1,
array[row(1, array[row(1, array[1,2])::z])::y])::x)::jsonb::hstore);
ERROR: malformed array literal: "{{"a"=>1, "b"=>{{"a"=>1, "b"=>{1, 2}}}}}"

yikes. The situation as I read it is that (notwithstanding my comments
upthread) there is no clean way to slide rowtypes to/from hstore and
jsonb while preserving structure. IMO, the above query should work
and the populate function record above should return the internally
structured row object, not the text escaped version.

merlin

On 01/30/2014 07:21 PM, Merlin Moncure wrote:

> Something seems off:
>
> postgres=# create type z as (a int, b int[]);
> CREATE TYPE
> postgres=# create type y as (a int, b z[]);
> CREATE TYPE
> postgres=# create type x as (a int, b y[]);
> CREATE TYPE
>
> -- test a complicated construction
> postgres=# select row(1, array[row(1, array[row(1, array[1,2])::z])::y])::x;
> row
> -------------------------------------------------------------------------------------
> (1,"{""(1,\\""{\\""\\""(1,\\\\\\\\\\""\\""{1,2}\\\\\\\\\\""\\"")\\""\\""}\\"")""}")
>
> postgres=# select hstore(row(1, array[row(1, array[row(1,
> array[1,2])::z])::y])::x);
> hstore
> ----------------------------------------------------------------------------------------------
> "a"=>1, "b"=>"{\"(1,\\\"{\\\"\\\"(1,\\\\\\\\\\\"\\\"{1,2}\\\\\\\\\\\"\\\")\\\"\\\"}\\\")\"}"
>
> here, the output escaping has leaked into the internal array
> structures. istm we should have a json expressing the internal
> structure.

What has this to do with json at all? It's clearly a failure in the
hstore() function.

> It does (weirdly) map back however:
>
> postgres=# select populate_record(null::x, hstore(row(1, array[row(1,
> array[row(1, array[1,2])::z])::y])::x));
> populate_record
> -------------------------------------------------------------------------------------
> (1,"{""(1,\\""{\\""\\""(1,\\\\\\\\\\""\\""{1,2}\\\\\\\\\\""\\"")\\""\\""}\\"")""}")
>
>
> OTOH, if I go via json route:
>
> postgres=# select row_to_json(row(1, array[row(1, array[row(1,
> array[1,2])::z])::y])::x);
> row_to_json
> -----------------------------------------------
> {"a":1,"b":[{"a":1,"b":[{"a":1,"b":[1,2]}]}]}
>
>
> so far, so good. let's push to hstore:
> postgres=# select row_to_json(row(1, array[row(1, array[row(1,
> array[1,2])::z])::y])::x)::jsonb::hstore;
> row_to_json
> -------------------------------------------------------
> "a"=>1, "b"=>[{"a"=>1, "b"=>[{"a"=>1, "b"=>[1, 2]}]}]
>
> this ISTM is the 'right' behavior. but what if we bring it back to
> record object?
>
> postgres=# select populate_record(null::x, row_to_json(row(1,
> array[row(1, array[row(1, array[1,2])::z])::y])::x)::jsonb::hstore);
> ERROR: malformed array literal: "{{"a"=>1, "b"=>{{"a"=>1, "b"=>{1, 2}}}}}"
>
> yikes. The situation as I read it is that (notwithstanding my comments
> upthread) there is no clean way to slide rowtypes to/from hstore and
> jsonb while preserving structure. IMO, the above query should work
> and the populate function record above should return the internally
> structured row object, not the text escaped version.

And this is a failure in populate_record().

I think we possibly need to say that handling of nested composites and
arrays is an area that needs further work. OTOH, the refusal of
json_populate_record() and json_populate_recordset() to handle these in
9.3 has not generated a flood of complaints, so I don't think it's a
tragedy, just a limitation, which should be documented if it's not
already. (And of course hstore hasn't handled nested anything before now.)

Meanwhile, maybe Teodor can fix the two hstore bugs shown here.

cheers

andrew

On Thu, Jan 30, 2014 at 4:52 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 01/30/2014 07:21 PM, Merlin Moncure wrote:
>> postgres=# select hstore(row(1, array[row(1, array[row(1,
>> array[1,2])::z])::y])::x);
>> hstore
>>
>> ----------------------------------------------------------------------------------------------
>> "a"=>1,
>> "b"=>"{\"(1,\\\"{\\\"\\\"(1,\\\\\\\\\\\"\\\"{1,2}\\\\\\\\\\\"\\\")\\\"\\\"}\\\")\"}"
>>
>> here, the output escaping has leaked into the internal array
>> structures. istm we should have a json expressing the internal
>> structure.
>
> What has this to do with json at all? It's clearly a failure in the hstore()
> function.

yeah -- meant to say 'hstore' there. Also I'm not sure that it's
'wrong'; it's just doing what it always did. That brings up another
point: are there any interesting cases of compatibility breakage? I'm
inclined not to care about this particular case though...

>> array[row(1, array[row(1, array[1,2])::z])::y])::x)::jsonb::hstore);
>> ERROR: malformed array literal: "{{"a"=>1, "b"=>{{"a"=>1, "b"=>{1,
>> 2}}}}}"
>>
>> yikes. The situation as I read it is that (notwithstanding my comments
>> upthread) there is no clean way to slide rowtypes to/from hstore and
>> jsonb while preserving structure. IMO, the above query should work
>> and the populate function record above should return the internally
>> structured row object, not the text escaped version.
>
>
>
> And this is a failure in populate_record().
>
> I think we possibly need to say that handling of nested composites and
> arrays is an area that needs further work. OTOH, the refusal of
> json_populate_record() and json_populate_recordset() to handle these in 9.3
> has not generated a flood of complaints, so I don't think it's a tragedy,
> just a limitation, which should be documented if it's not already. (And of
> course hstore hasn't handled nested anything before now.)
>
> Meanwhile, maybe Teodor can fix the two hstore bugs shown here.

While not a "flood", there certainly have been complaints. See
http://postgresql.1045698.n5.nabble.com/Best-way-to-populate-nested-composite-type-from-JSON-td5770566.html
http://osdir.com/ml/postgresql-pgsql-general/2014-01/msg00205.html

But, if we had to drop this in the interests of time I'd rather see
the behavior cauterized off so that it errored out 'not supported' (as
json_populate does) that attempt to implement the wrong behavior.

merlin

Hmm,
neither me, nor Teodor have experience and knowledge with
populate_record() and moreover hstore here is virgin and we don't know
the right behaviour, so I think we better take it from jsonb, once
Andrew realize it. Andrew ?

On Fri, Jan 31, 2014 at 4:52 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 01/30/2014 07:21 PM, Merlin Moncure wrote:
>
>> Something seems off:
>>
>> postgres=# create type z as (a int, b int[]);
>> CREATE TYPE
>> postgres=# create type y as (a int, b z[]);
>> CREATE TYPE
>> postgres=# create type x as (a int, b y[]);
>> CREATE TYPE
>>
>> -- test a complicated construction
>> postgres=# select row(1, array[row(1, array[row(1,
>> array[1,2])::z])::y])::x;
>> row
>>
>> -------------------------------------------------------------------------------------
>>
>> (1,"{""(1,\\""{\\""\\""(1,\\\\\\\\\\""\\""{1,2}\\\\\\\\\\""\\"")\\""\\""}\\"")""}")
>>
>> postgres=# select hstore(row(1, array[row(1, array[row(1,
>> array[1,2])::z])::y])::x);
>> hstore
>>
>> ----------------------------------------------------------------------------------------------
>> "a"=>1,
>> "b"=>"{\"(1,\\\"{\\\"\\\"(1,\\\\\\\\\\\"\\\"{1,2}\\\\\\\\\\\"\\\")\\\"\\\"}\\\")\"}"
>>
>> here, the output escaping has leaked into the internal array
>> structures. istm we should have a json expressing the internal
>> structure.
>
>
> What has this to do with json at all? It's clearly a failure in the hstore()
> function.
>
>
>
>> It does (weirdly) map back however:
>>
>> postgres=# select populate_record(null::x, hstore(row(1, array[row(1,
>> array[row(1, array[1,2])::z])::y])::x));
>> populate_record
>>
>> -------------------------------------------------------------------------------------
>>
>> (1,"{""(1,\\""{\\""\\""(1,\\\\\\\\\\""\\""{1,2}\\\\\\\\\\""\\"")\\""\\""}\\"")""}")
>>
>>
>> OTOH, if I go via json route:
>>
>> postgres=# select row_to_json(row(1, array[row(1, array[row(1,
>> array[1,2])::z])::y])::x);
>> row_to_json
>> -----------------------------------------------
>> {"a":1,"b":[{"a":1,"b":[{"a":1,"b":[1,2]}]}]}
>>
>>
>> so far, so good. let's push to hstore:
>> postgres=# select row_to_json(row(1, array[row(1, array[row(1,
>> array[1,2])::z])::y])::x)::jsonb::hstore;
>> row_to_json
>> -------------------------------------------------------
>> "a"=>1, "b"=>[{"a"=>1, "b"=>[{"a"=>1, "b"=>[1, 2]}]}]
>>
>> this ISTM is the 'right' behavior. but what if we bring it back to
>> record object?
>>
>> postgres=# select populate_record(null::x, row_to_json(row(1,
>> array[row(1, array[row(1, array[1,2])::z])::y])::x)::jsonb::hstore);
>> ERROR: malformed array literal: "{{"a"=>1, "b"=>{{"a"=>1, "b"=>{1,
>> 2}}}}}"
>>
>> yikes. The situation as I read it is that (notwithstanding my comments
>> upthread) there is no clean way to slide rowtypes to/from hstore and
>> jsonb while preserving structure. IMO, the above query should work
>> and the populate function record above should return the internally
>> structured row object, not the text escaped version.
>
>
>
> And this is a failure in populate_record().
>
> I think we possibly need to say that handling of nested composites and
> arrays is an area that needs further work. OTOH, the refusal of
> json_populate_record() and json_populate_recordset() to handle these in 9.3
> has not generated a flood of complaints, so I don't think it's a tragedy,
> just a limitation, which should be documented if it's not already. (And of
> course hstore hasn't handled nested anything before now.)
>
> Meanwhile, maybe Teodor can fix the two hstore bugs shown here.
>
> 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 Fri, Jan 31, 2014 at 4:03 AM, Oleg Bartunov <obartunov(at)gmail(dot)com> wrote:
> Hmm,
> neither me, nor Teodor have experience and knowledge with
> populate_record() and moreover hstore here is virgin and we don't know
> the right behaviour, so I think we better take it from jsonb, once
> Andrew realize it. Andrew ?

Andrew Gierth wrote the current implementation of htsore
populate_record IIRC. Unfortunately the plan for jsonb was to borrow
hstore's (I don't think hstore can use the jsonb implementation
because you'd be taking away the ability to handle internally nested
structures it currently has). Of my two complaints upthread, the
second one, not being able to populate from and internally well formed
structure, is by far the more serious one I think.

merlin

On 01/31/2014 08:57 AM, Merlin Moncure wrote:
> On Fri, Jan 31, 2014 at 4:03 AM, Oleg Bartunov <obartunov(at)gmail(dot)com> wrote:
>> Hmm,
>> neither me, nor Teodor have experience and knowledge with
>> populate_record() and moreover hstore here is virgin and we don't know
>> the right behaviour, so I think we better take it from jsonb, once
>> Andrew realize it. Andrew ?
> Andrew Gierth wrote the current implementation of htsore
> populate_record IIRC. Unfortunately the plan for jsonb was to borrow
> hstore's (I don't think hstore can use the jsonb implementation
> because you'd be taking away the ability to handle internally nested
> structures it currently has). Of my two complaints upthread, the
> second one, not being able to populate from and internally well formed
> structure, is by far the more serious one I think.
>

Umm, I think at least one of us is seriously confused.

I am going to look at dealing with these issues in a way that can be
used by both - at least the populate_record case.

As far as populate_record goes, there is a bit of an impedance mismatch,
since json/hstore records are heterogenous and one-dimensional, whereas
sql arrays are homogeneous and multidimensional. Right now I am thinking
I will deal with arrays up to two dimensions, because I can do that
relatively simply, and after that throw in the towel. That will surely
deal with 99.9% of use cases. Of course this would be documented.

Anyway, Let me see what I can do.

If Andrew Gierth wants to have a look at fixing the hstore() side that
might help speed things up.

cheers

andrew

On Fri, Jan 31, 2014 at 8:45 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 01/31/2014 08:57 AM, Merlin Moncure wrote:
>>
>> On Fri, Jan 31, 2014 at 4:03 AM, Oleg Bartunov <obartunov(at)gmail(dot)com>
>> wrote:
>>>
>>> Hmm,
>>> neither me, nor Teodor have experience and knowledge with
>>> populate_record() and moreover hstore here is virgin and we don't know
>>> the right behaviour, so I think we better take it from jsonb, once
>>> Andrew realize it. Andrew ?
>>
>> Andrew Gierth wrote the current implementation of htsore
>> populate_record IIRC. Unfortunately the plan for jsonb was to borrow
>> hstore's (I don't think hstore can use the jsonb implementation
>> because you'd be taking away the ability to handle internally nested
>> structures it currently has). Of my two complaints upthread, the
>> second one, not being able to populate from and internally well formed
>> structure, is by far the more serious one I think.
>>
>
>
> Umm, I think at least one of us is seriously confused.
>
> I am going to look at dealing with these issues in a way that can be used by
> both - at least the populate_record case.
>
> As far as populate_record goes, there is a bit of an impedance mismatch,
> since json/hstore records are heterogenous and one-dimensional, whereas sql
> arrays are homogeneous and multidimensional. Right now I am thinking I will
> deal with arrays up to two dimensions, because I can do that relatively
> simply, and after that throw in the towel. That will surely deal with 99.9%
> of use cases. Of course this would be documented.
>
> Anyway, Let me see what I can do.
>
> If Andrew Gierth wants to have a look at fixing the hstore() side that might
> help speed things up.

(ah, you beat me to it.)

Disregard my statements above. It works.

postgres=# select jsonb_populate_record(null::x, hstore(row(1,
array[row(1, array[row(1, array[1,2])::z])::y])::x)::jsonb);
jsonb_populate_record
-------------------------------------------------------------------------------------
(1,"{""(1,\\""{\\""\\""(1,\\\\\\\\\\""\\""{1,2}\\\\\\\\\\""\\"")\\""\\""}\\"")""}")

merlin

On 01/31/2014 09:53 AM, Merlin Moncure wrote:
> On Fri, Jan 31, 2014 at 8:45 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> On 01/31/2014 08:57 AM, Merlin Moncure wrote:
>>> On Fri, Jan 31, 2014 at 4:03 AM, Oleg Bartunov <obartunov(at)gmail(dot)com>
>>> wrote:
>>>> Hmm,
>>>> neither me, nor Teodor have experience and knowledge with
>>>> populate_record() and moreover hstore here is virgin and we don't know
>>>> the right behaviour, so I think we better take it from jsonb, once
>>>> Andrew realize it. Andrew ?
>>> Andrew Gierth wrote the current implementation of htsore
>>> populate_record IIRC. Unfortunately the plan for jsonb was to borrow
>>> hstore's (I don't think hstore can use the jsonb implementation
>>> because you'd be taking away the ability to handle internally nested
>>> structures it currently has). Of my two complaints upthread, the
>>> second one, not being able to populate from and internally well formed
>>> structure, is by far the more serious one I think.
>>>
>>
>> Umm, I think at least one of us is seriously confused.
>>
>> I am going to look at dealing with these issues in a way that can be used by
>> both - at least the populate_record case.
>>
>> As far as populate_record goes, there is a bit of an impedance mismatch,
>> since json/hstore records are heterogenous and one-dimensional, whereas sql
>> arrays are homogeneous and multidimensional. Right now I am thinking I will
>> deal with arrays up to two dimensions, because I can do that relatively
>> simply, and after that throw in the towel. That will surely deal with 99.9%
>> of use cases. Of course this would be documented.
>>
>> Anyway, Let me see what I can do.
>>
>> If Andrew Gierth wants to have a look at fixing the hstore() side that might
>> help speed things up.
> (ah, you beat me to it.)
>
> Disregard my statements above. It works.
>
> postgres=# select jsonb_populate_record(null::x, hstore(row(1,
> array[row(1, array[row(1, array[1,2])::z])::y])::x)::jsonb);
> jsonb_populate_record
> -------------------------------------------------------------------------------------
> (1,"{""(1,\\""{\\""\\""(1,\\\\\\\\\\""\\""{1,2}\\\\\\\\\\""\\"")\\""\\""}\\"")""}")
>

Actually, there is a workaround to the limitations of hstore(record):

andrew=# select row_to_json(row(1,
array[row(1, array[row(1, array[1,2])::z])::y])::x)::jsonb::hstore;
row_to_json
-------------------------------------------------------
"a"=>1, "b"=>[{"a"=>1, "b"=>[{"a"=>1, "b"=>[1, 2]}]}]

I think we could just document that for now, or possibly just use it
inside hstore(record) if we encounter a nested composite or array.

cheers

andrew

On Fri, Jan 31, 2014 at 9:26 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 01/31/2014 09:53 AM, Merlin Moncure wrote:
>>
>> On Fri, Jan 31, 2014 at 8:45 AM, Andrew Dunstan <andrew(at)dunslane(dot)net>
>> wrote:
>>>
>>> On 01/31/2014 08:57 AM, Merlin Moncure wrote:
>>>>
>>>> On Fri, Jan 31, 2014 at 4:03 AM, Oleg Bartunov <obartunov(at)gmail(dot)com>
>>>> wrote:
>>>>>
>>>>> Hmm,
>>>>> neither me, nor Teodor have experience and knowledge with
>>>>> populate_record() and moreover hstore here is virgin and we don't know
>>>>> the right behaviour, so I think we better take it from jsonb, once
>>>>> Andrew realize it. Andrew ?
>>>>
>>>> Andrew Gierth wrote the current implementation of htsore
>>>> populate_record IIRC. Unfortunately the plan for jsonb was to borrow
>>>> hstore's (I don't think hstore can use the jsonb implementation
>>>> because you'd be taking away the ability to handle internally nested
>>>> structures it currently has). Of my two complaints upthread, the
>>>> second one, not being able to populate from and internally well formed
>>>> structure, is by far the more serious one I think.
>>>>
>>>
>>> Umm, I think at least one of us is seriously confused.
>>>
>>> I am going to look at dealing with these issues in a way that can be used
>>> by
>>> both - at least the populate_record case.
>>>
>>> As far as populate_record goes, there is a bit of an impedance mismatch,
>>> since json/hstore records are heterogenous and one-dimensional, whereas
>>> sql
>>> arrays are homogeneous and multidimensional. Right now I am thinking I
>>> will
>>> deal with arrays up to two dimensions, because I can do that relatively
>>> simply, and after that throw in the towel. That will surely deal with
>>> 99.9%
>>> of use cases. Of course this would be documented.
>>>
>>> Anyway, Let me see what I can do.
>>>
>>> If Andrew Gierth wants to have a look at fixing the hstore() side that
>>> might
>>> help speed things up.
>>
>> (ah, you beat me to it.)
>>
>> Disregard my statements above. It works.
>>
>> postgres=# select jsonb_populate_record(null::x, hstore(row(1,
>> array[row(1, array[row(1, array[1,2])::z])::y])::x)::jsonb);
>> jsonb_populate_record
>>
>> -------------------------------------------------------------------------------------
>>
>> (1,"{""(1,\\""{\\""\\""(1,\\\\\\\\\\""\\""{1,2}\\\\\\\\\\""\\"")\\""\\""}\\"")""}")
>
>
> Actually, there is a workaround to the limitations of hstore(record):

yeah I'm ok with hstore() function as it is. That also eliminates
backwards compatibility concerns so things worked out. The only 'must
fix' 9.4 facing issue I see on the table is to make sure jsonb
populate function is forward compatible with future expectations of
behavior which to me means zeroing in on the necessity of the as_text
argument (but if you can expand coverage without jeopardizing 9.4
inclusion than great...).

For my part I'm going to continue functionally testing the rest of the
API (so far, a cursory look hasn't turned up anything else). I'm also
signing up for some documentation refinements which will be done after
you nail down these little bits but before the end of the 'fest.

IMNSHO, formal code review needs to begin ASAP (salahaldin is the
reviewer per the fest wiki)

merlin

On 01/31/2014 02:48 PM, Merlin Moncure wrote:

>>
>> Actually, there is a workaround to the limitations of hstore(record):
> yeah I'm ok with hstore() function as it is. That also eliminates
> backwards compatibility concerns so things worked out. The only 'must
> fix' 9.4 facing issue I see on the table is to make sure jsonb
> populate function is forward compatible with future expectations of
> behavior which to me means zeroing in on the necessity of the as_text
> argument (but if you can expand coverage without jeopardizing 9.4
> inclusion than great...).

This isn't terribly clear. Currently, if jsonb_populate_record{set}
encounters a nested array or object when populating the record it errors
out, regardless of the type of the field, unless as_text is set (it
defaults to off). In the latter case it tries to use the array or
object's json text representation as the value to populate the field
(realistically, this only works for text, json and jsonb fields). This
is exactly the current behaviour of json_populate_record{set}. The
enhancement would be to alter the behaviour when as_text is NOT set. In
this case, we would try recursively to populate an array or composite
field with the corresponding jsonb. i.e we would be removing some
current error conditions and returning a result. But we would not be
returning a different result in any case where we now return a result. I
think that's future-proof enough.

Frankly, I think the behaviour of hstore(record) with nested composites
and arrays is sufficiently counter-intuitive, to put it mildly, that we
should at least document the workaround from my previous email.

>
> For my part I'm going to continue functionally testing the rest of the
> API (so far, a cursory look hasn't turned up anything else). I'm also
> signing up for some documentation refinements which will be done after
> you nail down these little bits but before the end of the 'fest.
>
> IMNSHO, formal code review needs to begin ASAP (salahaldin is the
> reviewer per the fest wiki)

Yes, or anyone else who wants to join in. I'd very much welcome a
substantial code review - I have been staring at this far too long on my
own.

cheers

andrew

On 01/31/2014 11:35 PM, Andrew Dunstan wrote:
>
> Yes, or anyone else who wants to join in. I'd very much welcome a
> substantial code review - I have been staring at this far too long on
> my own.

I should mention that in fact by far the largest piece of this is not my
work, but Oleg and Teodor's work. I'm sure they would welcome an in
depth review too, but I realised that my message above might have given
a false impression. They deserve the credit for having come up with and
implemented this scheme for tree-ish data.

cheers

andrew

On 2014-01-30 14:07:42 -0500, Andrew Dunstan wrote:
> + <para id="functions-json-table">
> + <xref linkend="functions-json-creation-table"> shows the functions that are
> + available for creating <type>json</type> values.
> + (see <xref linkend="datatype-json">)
> </para>
>
> - <table id="functions-json-table">
> - <title>JSON Support Functions</title>
> + <indexterm>
> + <primary>array_to_json</primary>
> + </indexterm>
> + <indexterm>
> + <primary>row_to_json</primary>
> + </indexterm>
> + <indexterm>
> + <primary>to_json</primary>
> + </indexterm>
> + <indexterm>
> + <primary>json_build_array</primary>
> + </indexterm>
> + <indexterm>
> + <primary>json_build_object</primary>
> + </indexterm>
> + <indexterm>
> + <primary>json_object</primary>
> + </indexterm>

Hm, why are you collecting the indexterms at the top in the contrast to
the previous way of collecting them at the point of documentation?

> diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
> index 1ae9fa0..fd93d9b 100644
> --- a/src/backend/utils/adt/Makefile
> +++ b/src/backend/utils/adt/Makefile
> @@ -32,7 +32,8 @@ OBJS = acl.o arrayfuncs.o array_selfuncs.o array_typanalyze.o \
> tsquery_op.o tsquery_rewrite.o tsquery_util.o tsrank.o \
> tsvector.o tsvector_op.o tsvector_parser.o \
> txid.o uuid.o windowfuncs.o xml.o rangetypes_spgist.o \
> - rangetypes_typanalyze.o rangetypes_selfuncs.o
> + rangetypes_typanalyze.o rangetypes_selfuncs.o \
> + jsonb.o jsonb_support.o

Odd, most OBJS lines are kept in alphabetical order, but that doesn't
seem to be the case here.

> +/*
> + * for jsonb we always want the de-escaped value - that's what's in token
> + */
> +

strange newline.

> +static void
> +jsonb_in_scalar(void *state, char *token, JsonTokenType tokentype)
> +{
> + JsonbInState *_state = (JsonbInState *) state;
> + JsonbValue v;
> +
> + v.size = sizeof(JEntry);
> +
> + switch (tokentype)
> + {
> +
...

> + default: /* nothing else should be here in fact */
> + break;

Shouldn't this at least Assert(false) or something?

> +static void
> +recvJsonbValue(StringInfo buf, JsonbValue *v, uint32 level, int c)
> +{
> + uint32 hentry = c & JENTRY_TYPEMASK;
> +
> + if (hentry == JENTRY_ISNULL)
> + {
> + v->type = jbvNull;
> + v->size = sizeof(JEntry);
> + }
> + else if (hentry == JENTRY_ISOBJECT || hentry == JENTRY_ISARRAY || hentry == JENTRY_ISCALAR)
> + {
> + recvJsonb(buf, v, level + 1, (uint32) c);
> + }
> + else if (hentry == JENTRY_ISFALSE || hentry == JENTRY_ISTRUE)
> + {
> + v->type = jbvBool;
> + v->size = sizeof(JEntry);
> + v->boolean = (hentry == JENTRY_ISFALSE) ? false : true;
> + }
> + else if (hentry == JENTRY_ISNUMERIC)
> + {
> + v->type = jbvNumeric;
> + v->numeric = DatumGetNumeric(DirectFunctionCall3(numeric_recv, PointerGetDatum(buf),
> + Int32GetDatum(0), Int32GetDatum(-1)));
> +
> + v->size = sizeof(JEntry) * 2 + VARSIZE_ANY(v->numeric);

What's the *2 here?

> +static void
> +recvJsonb(StringInfo buf, JsonbValue *v, uint32 level, uint32 header)
> +{
> + uint32 hentry;
> + uint32 i;

This function and recvJsonbValue call each other recursively, afaics
without any limit, shouldn't they check for the stack depth?

> + hentry = header & JENTRY_TYPEMASK;
> +
> + v->size = 3 * sizeof(JEntry);

*3?

> + if (hentry == JENTRY_ISOBJECT)
> + {
> + v->type = jbvHash;
> + v->hash.npairs = header & JB_COUNT_MASK;
> + if (v->hash.npairs > 0)
> + {
> + v->hash.pairs = palloc(sizeof(*v->hash.pairs) * v->hash.npairs);
> +

Hm, if I understand correctly, we're just allocating JB_COUNT_MASK
(which is 0x0FFFFFFF) * sizeof(*v->hash.pairs) bytes here, without any
crosschecks about the actual length of the data? So with a few bytes the
server can be coaxed to allocate a gigabyte of data?
Since this immediately calls another input routine, this can be done in
a nested fashion, quickly OOMing the server.

I think this and several other places really need a bit more input
sanity checking.

> + for (i = 0; i < v->hash.npairs; i++)
> + {
> + recvJsonbValue(buf, &v->hash.pairs[i].key, level, pq_getmsgint(buf, 4));
> + if (v->hash.pairs[i].key.type != jbvString)
> + elog(ERROR, "jsonb's key could be only a string");

Shouldn't that be an ereport(ERRCODE_DATATYPE_MISMATCH)? Similar in a
few other places.

> +char *
> +JsonbToCString(StringInfo out, char *in, int estimated_len)
> +{
> + bool first = true;
> + JsonbIterator *it;
> + int type;
> + JsonbValue v;
> + int level = 0;
> +
> + if (out == NULL)
> + out = makeStringInfo();

Such a behaviour certainly deserves a documentary comment. Generally
some more functions could use that.

> + while ((type = JsonbIteratorGet(&it, &v, false)) != 0)
> + {
> +reout:
> + switch (type)
> + {
...
> + {
> + Assert(type == WJB_BEGIN_OBJECT || type == WJB_BEGIN_ARRAY);
> + goto reout;

Hrmpf.

> +Datum
> +jsonb_typeof(PG_FUNCTION_ARGS)
> +{
...
> +}

Hm, shouldn't that be in jsonfuncs.c?

> diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
> index a19b222..f1eacc6 100644
> --- a/src/backend/utils/adt/jsonfuncs.c
> +++ b/src/backend/utils/adt/jsonfuncs.c
> @@ -27,6 +27,7 @@
> #include "utils/builtins.h"
> #include "utils/hsearch.h"
> #include "utils/json.h"
> +#include "utils/jsonb.h"
> #include "utils/jsonapi.h"
> #include "utils/lsyscache.h"
> #include "utils/memutils.h"
> @@ -51,6 +52,7 @@ static inline Datum get_path_all(PG_FUNCTION_ARGS, bool as_text);
> static inline text *get_worker(text *json, char *field, int elem_index,
> char **tpath, int *ipath, int npath,
> bool normalize_results);
> +static inline Datum get_jsonb_path_all(PG_FUNCTION_ARGS, bool as_text);

I don't see the point of using PG_FUNCTION_ARGS if you're manually
calling it like
+ return get_jsonb_path_all(fcinfo, false);

That just makes it harder if someday PG_FUNCTION_ARGS grows a second
argument or something.

> +Datum
> +jsonb_object_keys(PG_FUNCTION_ARGS)
> +{
> + FuncCallContext *funcctx;
> + OkeysState *state;
> + int i;
> +
> + if (SRF_IS_FIRSTCALL())
> + {
> + MemoryContext oldcontext;
> + Jsonb *jb = PG_GETARG_JSONB(0);
> + bool skipNested = false;
> + JsonbIterator *it;
> + JsonbValue v;
> + int r = 0;
> +
> + if (JB_ROOT_IS_SCALAR(jb))
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("cannot call jsonb_object_keys on a scalar")));
> + else if (JB_ROOT_IS_ARRAY(jb))
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("cannot call jsonb_object_keys on an array")));
> +
> + funcctx = SRF_FIRSTCALL_INIT();
> + oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);

This will detoast 'jb' into the expression context, since
PG_GETARG_JSONB() is called before the MemoryContextSwitchTo. But that's
ok since the percall code only deals with ->result, right?

> - /* make these in a sufficiently long-lived memory context */
> old_cxt = MemoryContextSwitchTo(rsi->econtext->ecxt_per_query_memory);

wh remove that comment?

> +#define JENTRY_ISCALAR (0x10000000 | 0x40000000)

Isn't there an S missing here?

> --- a/contrib/hstore/hstore_compat.c
> +++ b/contrib/hstore/hstore_compat.c
> +/*
> + * New Old version (new not-nested version of hstore, v2 version)
> + * V2 and v3 (nested) are upward binary compatible. But
> + * framework was fully changed. Keep here old definitions (v2)
> + */

That's an, err, interesting sentence. I think referring to old new
version and stuff is less than helpful. I realize lots of that is
baggage from existing code, but yet another version doesn't make it
easier.

I lost my stomach (or maybe it was the glass of red) somewhere in the
middle, but I think this needs a lot of work. Especially the io code
doesn't seem ready to me. I'd consider ripping out the send/recv code
for 9.4, that seems the biggest can of worms. It will still be usable
without.

There's just about no comments in large and relevant parts of the
code. There's not much documentation about the binary layout of a
definitely not trivial type with convoluted interdependencies with
hstore...

Greetings,

Andres Freund

On 02/01/2014 05:20 PM, Andres Freund wrote:

[Long review]

Most of these comments actually refer to Teodor and Oleg's code.

I will attend to the parts that apply to my code.

Thanks for the review.

cheers

andrew

Hi,

On 2014-02-01 18:13:42 -0500, Andrew Dunstan wrote:
> [Long review]
>
> Most of these comments actually refer to Teodor and Oleg's code.
>
> I will attend to the parts that apply to my code.

Well, somebody will need to address them nonetheless :/

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 02/01/2014 06:15 PM, Andres Freund wrote:
> Hi,
>
> On 2014-02-01 18:13:42 -0500, Andrew Dunstan wrote:
>> [Long review]
>>
>> Most of these comments actually refer to Teodor and Oleg's code.
>>
>> I will attend to the parts that apply to my code.
> Well, somebody will need to address them nonetheless :/
>

Yes, of course, I didn't suggest otherwise.

cheers

andrew

On Sat, Feb 1, 2014 at 4:20 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-01-30 14:07:42 -0500, Andrew Dunstan wrote:
>> + <para id="functions-json-table">
>> + <xref linkend="functions-json-creation-table"> shows the functions that are
>> + available for creating <type>json</type> values.
>> + (see <xref linkend="datatype-json">)
>> </para>
>>
>> - <table id="functions-json-table">
>> - <title>JSON Support Functions</title>
>> + <indexterm>
>> + <primary>array_to_json</primary>
>> + </indexterm>
>> + <indexterm>
>> + <primary>row_to_json</primary>
>> + </indexterm>
>> + <indexterm>
>> + <primary>to_json</primary>
>> + </indexterm>
>> + <indexterm>
>> + <primary>json_build_array</primary>
>> + </indexterm>
>> + <indexterm>
>> + <primary>json_build_object</primary>
>> + </indexterm>
>> + <indexterm>
>> + <primary>json_object</primary>
>> + </indexterm>
>
> Hm, why are you collecting the indexterms at the top in the contrast to
> the previous way of collecting them at the point of documentation?
>
>> diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
>> index 1ae9fa0..fd93d9b 100644
>> --- a/src/backend/utils/adt/Makefile
>> +++ b/src/backend/utils/adt/Makefile
>> @@ -32,7 +32,8 @@ OBJS = acl.o arrayfuncs.o array_selfuncs.o array_typanalyze.o \
>> tsquery_op.o tsquery_rewrite.o tsquery_util.o tsrank.o \
>> tsvector.o tsvector_op.o tsvector_parser.o \
>> txid.o uuid.o windowfuncs.o xml.o rangetypes_spgist.o \
>> - rangetypes_typanalyze.o rangetypes_selfuncs.o
>> + rangetypes_typanalyze.o rangetypes_selfuncs.o \
>> + jsonb.o jsonb_support.o
>
> Odd, most OBJS lines are kept in alphabetical order, but that doesn't
> seem to be the case here.
>
>> +/*
>> + * for jsonb we always want the de-escaped value - that's what's in token
>> + */
>> +
>
> strange newline.
>
>> +static void
>> +jsonb_in_scalar(void *state, char *token, JsonTokenType tokentype)
>> +{
>> + JsonbInState *_state = (JsonbInState *) state;
>> + JsonbValue v;
>> +
>> + v.size = sizeof(JEntry);
>> +
>> + switch (tokentype)
>> + {
>> +
> ...
>
>> + default: /* nothing else should be here in fact */
>> + break;
>
> Shouldn't this at least Assert(false) or something?
>
>> +static void
>> +recvJsonbValue(StringInfo buf, JsonbValue *v, uint32 level, int c)
>> +{
>> + uint32 hentry = c & JENTRY_TYPEMASK;
>> +
>> + if (hentry == JENTRY_ISNULL)
>> + {
>> + v->type = jbvNull;
>> + v->size = sizeof(JEntry);
>> + }
>> + else if (hentry == JENTRY_ISOBJECT || hentry == JENTRY_ISARRAY || hentry == JENTRY_ISCALAR)
>> + {
>> + recvJsonb(buf, v, level + 1, (uint32) c);
>> + }
>> + else if (hentry == JENTRY_ISFALSE || hentry == JENTRY_ISTRUE)
>> + {
>> + v->type = jbvBool;
>> + v->size = sizeof(JEntry);
>> + v->boolean = (hentry == JENTRY_ISFALSE) ? false : true;
>> + }
>> + else if (hentry == JENTRY_ISNUMERIC)
>> + {
>> + v->type = jbvNumeric;
>> + v->numeric = DatumGetNumeric(DirectFunctionCall3(numeric_recv, PointerGetDatum(buf),
>> + Int32GetDatum(0), Int32GetDatum(-1)));
>> +
>> + v->size = sizeof(JEntry) * 2 + VARSIZE_ANY(v->numeric);
>
> What's the *2 here?
>
>> +static void
>> +recvJsonb(StringInfo buf, JsonbValue *v, uint32 level, uint32 header)
>> +{
>> + uint32 hentry;
>> + uint32 i;
>
> This function and recvJsonbValue call each other recursively, afaics
> without any limit, shouldn't they check for the stack depth?
>
>> + hentry = header & JENTRY_TYPEMASK;
>> +
>> + v->size = 3 * sizeof(JEntry);
>
> *3?
>
>> + if (hentry == JENTRY_ISOBJECT)
>> + {
>> + v->type = jbvHash;
>> + v->hash.npairs = header & JB_COUNT_MASK;
>> + if (v->hash.npairs > 0)
>> + {
>> + v->hash.pairs = palloc(sizeof(*v->hash.pairs) * v->hash.npairs);
>> +
>
> Hm, if I understand correctly, we're just allocating JB_COUNT_MASK
> (which is 0x0FFFFFFF) * sizeof(*v->hash.pairs) bytes here, without any
> crosschecks about the actual length of the data? So with a few bytes the
> server can be coaxed to allocate a gigabyte of data?
> Since this immediately calls another input routine, this can be done in
> a nested fashion, quickly OOMing the server.
>
> I think this and several other places really need a bit more input
> sanity checking.
>
>> + for (i = 0; i < v->hash.npairs; i++)
>> + {
>> + recvJsonbValue(buf, &v->hash.pairs[i].key, level, pq_getmsgint(buf, 4));
>> + if (v->hash.pairs[i].key.type != jbvString)
>> + elog(ERROR, "jsonb's key could be only a string");
>
> Shouldn't that be an ereport(ERRCODE_DATATYPE_MISMATCH)? Similar in a
> few other places.
>
>> +char *
>> +JsonbToCString(StringInfo out, char *in, int estimated_len)
>> +{
>> + bool first = true;
>> + JsonbIterator *it;
>> + int type;
>> + JsonbValue v;
>> + int level = 0;
>> +
>> + if (out == NULL)
>> + out = makeStringInfo();
>
> Such a behaviour certainly deserves a documentary comment. Generally
> some more functions could use that.
>
>> + while ((type = JsonbIteratorGet(&it, &v, false)) != 0)
>> + {
>> +reout:
>> + switch (type)
>> + {
> ...
>> + {
>> + Assert(type == WJB_BEGIN_OBJECT || type == WJB_BEGIN_ARRAY);
>> + goto reout;
>
> Hrmpf.
>
>> +Datum
>> +jsonb_typeof(PG_FUNCTION_ARGS)
>> +{
> ...
>> +}
>
> Hm, shouldn't that be in jsonfuncs.c?
>
>> diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
>> index a19b222..f1eacc6 100644
>> --- a/src/backend/utils/adt/jsonfuncs.c
>> +++ b/src/backend/utils/adt/jsonfuncs.c
>> @@ -27,6 +27,7 @@
>> #include "utils/builtins.h"
>> #include "utils/hsearch.h"
>> #include "utils/json.h"
>> +#include "utils/jsonb.h"
>> #include "utils/jsonapi.h"
>> #include "utils/lsyscache.h"
>> #include "utils/memutils.h"
>> @@ -51,6 +52,7 @@ static inline Datum get_path_all(PG_FUNCTION_ARGS, bool as_text);
>> static inline text *get_worker(text *json, char *field, int elem_index,
>> char **tpath, int *ipath, int npath,
>> bool normalize_results);
>> +static inline Datum get_jsonb_path_all(PG_FUNCTION_ARGS, bool as_text);
>
> I don't see the point of using PG_FUNCTION_ARGS if you're manually
> calling it like
> + return get_jsonb_path_all(fcinfo, false);
>
> That just makes it harder if someday PG_FUNCTION_ARGS grows a second
> argument or something.
>
>
>> +Datum
>> +jsonb_object_keys(PG_FUNCTION_ARGS)
>> +{
>> + FuncCallContext *funcctx;
>> + OkeysState *state;
>> + int i;
>> +
>> + if (SRF_IS_FIRSTCALL())
>> + {
>> + MemoryContext oldcontext;
>> + Jsonb *jb = PG_GETARG_JSONB(0);
>> + bool skipNested = false;
>> + JsonbIterator *it;
>> + JsonbValue v;
>> + int r = 0;
>> +
>> + if (JB_ROOT_IS_SCALAR(jb))
>> + ereport(ERROR,
>> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> + errmsg("cannot call jsonb_object_keys on a scalar")));
>> + else if (JB_ROOT_IS_ARRAY(jb))
>> + ereport(ERROR,
>> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> + errmsg("cannot call jsonb_object_keys on an array")));
>> +
>> + funcctx = SRF_FIRSTCALL_INIT();
>> + oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
>
> This will detoast 'jb' into the expression context, since
> PG_GETARG_JSONB() is called before the MemoryContextSwitchTo. But that's
> ok since the percall code only deals with ->result, right?
>
>> - /* make these in a sufficiently long-lived memory context */
>> old_cxt = MemoryContextSwitchTo(rsi->econtext->ecxt_per_query_memory);
>
> wh remove that comment?
>
>> +#define JENTRY_ISCALAR (0x10000000 | 0x40000000)
>
> Isn't there an S missing here?
>
>> --- a/contrib/hstore/hstore_compat.c
>> +++ b/contrib/hstore/hstore_compat.c
>> +/*
>> + * New Old version (new not-nested version of hstore, v2 version)
>> + * V2 and v3 (nested) are upward binary compatible. But
>> + * framework was fully changed. Keep here old definitions (v2)
>> + */
>
> That's an, err, interesting sentence. I think referring to old new
> version and stuff is less than helpful. I realize lots of that is
> baggage from existing code, but yet another version doesn't make it
> easier.
>
> I lost my stomach (or maybe it was the glass of red) somewhere in the
> middle, but I think this needs a lot of work. Especially the io code
> doesn't seem ready to me. I'd consider ripping out the send/recv code
> for 9.4, that seems the biggest can of worms. It will still be usable
> without.

Not having type send/recv functions is somewhat dangerous; it can
cause problems for libraries that run everything through the binary
wire format. I'd give jsonb a pass on that, being a new type, but
would be concerned if hstore had that ability revoked.

offhand note: hstore_send seems pretty simply written and clean; it's
a simple nonrecursive iterator...

merlin

On 2014-02-03 09:22:52 -0600, Merlin Moncure wrote:
> > I lost my stomach (or maybe it was the glass of red) somewhere in the
> > middle, but I think this needs a lot of work. Especially the io code
> > doesn't seem ready to me. I'd consider ripping out the send/recv code
> > for 9.4, that seems the biggest can of worms. It will still be usable
> > without.
>
> Not having type send/recv functions is somewhat dangerous; it can
> cause problems for libraries that run everything through the binary
> wire format. I'd give jsonb a pass on that, being a new type, but
> would be concerned if hstore had that ability revoked.

Yea, removing it for hstore would be a compat problem...

> offhand note: hstore_send seems pretty simply written and clean; it's
> a simple nonrecursive iterator...

But a send function is pretty pointless without the corresponding recv
function... And imo recv simply is to dangerous as it's currently
written.
I am not saying that it cannot be made work, just that it's still nearly
as ugly as when I pointed out several of the dangers some weeks back.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 02/03/2014 07:27 AM, Andres Freund wrote:
> On 2014-02-03 09:22:52 -0600, Merlin Moncure wrote:
>>> I lost my stomach (or maybe it was the glass of red) somewhere in the
>>> middle, but I think this needs a lot of work. Especially the io code
>>> doesn't seem ready to me. I'd consider ripping out the send/recv code
>>> for 9.4, that seems the biggest can of worms. It will still be usable
>>> without.
>>
>> Not having type send/recv functions is somewhat dangerous; it can
>> cause problems for libraries that run everything through the binary
>> wire format. I'd give jsonb a pass on that, being a new type, but
>> would be concerned if hstore had that ability revoked.
>
> Yea, removing it for hstore would be a compat problem...
>
>> offhand note: hstore_send seems pretty simply written and clean; it's
>> a simple nonrecursive iterator...
>
> But a send function is pretty pointless without the corresponding recv
> function... And imo recv simply is to dangerous as it's currently
> written.
> I am not saying that it cannot be made work, just that it's still nearly
> as ugly as when I pointed out several of the dangers some weeks back.

Oleg, Teodor, any comments on the above?

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

On 02/03/2014 05:22 PM, Merlin Moncure wrote:
>> >I lost my stomach (or maybe it was the glass of red) somewhere in the
>> >middle, but I think this needs a lot of work. Especially the io code
>> >doesn't seem ready to me. I'd consider ripping out the send/recv code
>> >for 9.4, that seems the biggest can of worms. It will still be usable
>> >without.
> Not having type send/recv functions is somewhat dangerous; it can
> cause problems for libraries that run everything through the binary
> wire format. I'd give jsonb a pass on that, being a new type, but
> would be concerned if hstore had that ability revoked.

send/recv functions are also needed for binary-format COPY. IMHO jsonb
must have send/recv functions. All other built-in types have them,
except for types like 'smgr', 'aclitem' and 'any*' that no-one should be
using as column types.

- Heikki

Andrew provided us more information and we'll work on recv. What
people think about testing this stuff ? btw, we don't have any
regression test on this.

Oleg

On Wed, Feb 5, 2014 at 2:03 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 02/03/2014 07:27 AM, Andres Freund wrote:
>> On 2014-02-03 09:22:52 -0600, Merlin Moncure wrote:
>>>> I lost my stomach (or maybe it was the glass of red) somewhere in the
>>>> middle, but I think this needs a lot of work. Especially the io code
>>>> doesn't seem ready to me. I'd consider ripping out the send/recv code
>>>> for 9.4, that seems the biggest can of worms. It will still be usable
>>>> without.
>>>
>>> Not having type send/recv functions is somewhat dangerous; it can
>>> cause problems for libraries that run everything through the binary
>>> wire format. I'd give jsonb a pass on that, being a new type, but
>>> would be concerned if hstore had that ability revoked.
>>
>> Yea, removing it for hstore would be a compat problem...
>>
>>> offhand note: hstore_send seems pretty simply written and clean; it's
>>> a simple nonrecursive iterator...
>>
>> But a send function is pretty pointless without the corresponding recv
>> function... And imo recv simply is to dangerous as it's currently
>> written.
>> I am not saying that it cannot be made work, just that it's still nearly
>> as ugly as when I pointed out several of the dangers some weeks back.
>
> Oleg, Teodor, any comments on the above?
>
> --
> Josh Berkus
> PostgreSQL Experts Inc.
> http://pgexperts.com
>
>
> --
> 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

>> +static void
>> +recvJsonbValue(StringInfo buf, JsonbValue *v, uint32 level, int c)
>> + v->size = sizeof(JEntry) * 2 + VARSIZE_ANY(v->numeric);
>
> What's the *2 here?
Reservation for aligment. It's allowed to be v->size greater than it's actually
needed. Fixed.

> This function and recvJsonbValue call each other recursively, afaics
> without any limit, shouldn't they check for the stack depth?
added a check_stack_depth()

>
> *3?

Jentry + header + reservation for aligment

>> + v->hash.pairs = palloc(sizeof(*v->hash.pairs) * v->hash.npairs);
>> +
if (v->hash.npairs > (buf->len - buf->cursor) / (2 * sizeof(uint32)))
ereport(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE)
2 * sizeof(uint32) - minimal size of object element (key plus its value)

> Shouldn't that be an ereport(ERRCODE_DATATYPE_MISMATCH)? Similar in a
> few other places.
fixed

>> +char *
>> +JsonbToCString(StringInfo out, char *in, int estimated_len)
> Such a behaviour certainly deserves a documentary comment. Generally
> some more functions could use that.
add comment

>
>> + while ((type = JsonbIteratorGet(&it, &v, false)) != 0)
>> +reout:
>> + goto reout;
>
> Hrmpf.

:) commented

>
>> +Datum
>> +jsonb_typeof(PG_FUNCTION_ARGS)
>> +{
> ...
>> +}
>
> Hm, shouldn't that be in jsonfuncs.c?
No idea, i don't have an objection

send/recv for hstore is fixed too. Should I make new version of patch? Right now
it's placed on github. May be Andrew wants to change something?

--
Teodor Sigaev E-mail: teodor(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/

On Wed, Feb 5, 2014 at 12:44 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> send/recv functions are also needed for binary-format COPY. IMHO jsonb must
> have send/recv functions. All other built-in types have them, except for
> types like 'smgr', 'aclitem' and 'any*' that no-one should be using as
> column types.

Yes -- completely agree. I also consider the hstore functionality (in
particular, searching and access operators) to be essential
functionality.

I'm actually surprised we have an alternate binary wire format for
jsonb at all; json is explicitly text and I'm not sure what the use
case of sending the internal structure is. Meaning, maybe jsonb
send/recv should be a thin wrapper to sending the json string. The
hstore send/recv I think properly covers the case where client side
binary wire format actors would want to manage performance critical
cases that want to avoid parsing.

On Wed, Feb 5, 2014 at 1:21 AM, Oleg Bartunov <obartunov(at)gmail(dot)com> wrote:
> Andrew provided us more information and we'll work on recv. What
> people think about testing this stuff ? btw, we don't have any
> regression test on this.

I'm intensely interested in this work; I consider it to be transformative.

I've *lightly* tested the jsonb/hstore functionality and so far
everything is working.

I still have concerns about the API. Aside from the stuff I mentioned
upthread I find the API split between jsonb and hstore to be a little
odd; a lot of useful bits (for example, the @> operator) come via the
hstore type only. So these types are joined at the hip for real work
which makes the diverging incomplete behaviors in functions like
populate_record() disconcerting. Another point I'm struggling with is
what jsonb brings to the table that isn't covered either hstore or
json; working through a couple of cases I find myself not using the
jsonb functionality except as a 'hstore json formatter' which the json
type covers. I'm probably being obtuse, but we have to be cautious
before plonking a couple of dozen extra functions in the public
schema.

merlin

On 02/05/2014 10:48 AM, Merlin Moncure wrote:
> On Wed, Feb 5, 2014 at 12:44 AM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
>> send/recv functions are also needed for binary-format COPY. IMHO jsonb must
>> have send/recv functions. All other built-in types have them, except for
>> types like 'smgr', 'aclitem' and 'any*' that no-one should be using as
>> column types.
> Yes -- completely agree. I also consider the hstore functionality (in
> particular, searching and access operators) to be essential
> functionality.
>
> I'm actually surprised we have an alternate binary wire format for
> jsonb at all; json is explicitly text and I'm not sure what the use
> case of sending the internal structure is. Meaning, maybe jsonb
> send/recv should be a thin wrapper to sending the json string. The
> hstore send/recv I think properly covers the case where client side
> binary wire format actors would want to manage performance critical
> cases that want to avoid parsing.
>
>

The whole reason we have jsonb is to avoid reparsing where possible. In
fact, I'd rather have the send and recv functions in the jsonb code and
have hstore's functions call them, so we don't duplicate code.

cheers

andrew

On 02/05/2014 10:36 AM, Teodor Sigaev wrote:
>
>>
>>> +Datum
>>> +jsonb_typeof(PG_FUNCTION_ARGS)
>>> +{
>> ...
>>> +}
>>
>> Hm, shouldn't that be in jsonfuncs.c?
> No idea, i don't have an objection

No it shouldn't. The json equivalent function is in json.c, and needs to
be because it uses the parser internals that aren't exposed outside that
code.

>
> send/recv for hstore is fixed too. Should I make new version of patch?
> Right now it's placed on github. May be Andrew wants to change something?
>
>

I'll take a look, but I think we need to unify this so we use one set of
send/recv code for the two types if possible, as I just said to Merlin.

cheers

andrew

On Wed, Feb 5, 2014 at 10:22 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> I'm actually surprised we have an alternate binary wire format for
>> jsonb at all; json is explicitly text and I'm not sure what the use
>> case of sending the internal structure is. Meaning, maybe jsonb
>> send/recv should be a thin wrapper to sending the json string. The
>> hstore send/recv I think properly covers the case where client side
>> binary wire format actors would want to manage performance critical
>> cases that want to avoid parsing.
>
> The whole reason we have jsonb is to avoid reparsing where possible

Sure; but on the server side. The wire format is for handling client
concerns. For example, the case you're arguing for would be for libpq
client to extract as jsonb as binary, manipulate it on a binary level,
then send it back as binary. I find this case to be something of a
stretch.

That being said, for binary dump/restore perhaps there's a performance
case to be made.

> In fact, I'd rather have the send and recv functions in the jsonb code and have
> hstore's functions call them, so we don't duplicate code.

yeah. Agree that there needs to be two sets of routines, not three.
I think a case could be made for the jsonb type could take either
json's or hstore's depending on if the above. FWIW, either way is
fine.

merlin

Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> On Wed, Feb 5, 2014 at 10:22 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> The whole reason we have jsonb is to avoid reparsing where possible

> Sure; but on the server side. The wire format is for handling client
> concerns. For example, the case you're arguing for would be for libpq
> client to extract as jsonb as binary, manipulate it on a binary level,
> then send it back as binary. I find this case to be something of a
> stretch.

I'm with Merlin in thinking that the case for exposing a binary format
to clients is pretty weak, or at least a convincing use-case has not
been shown. Given the concerns upthread about security hazards in the
patch's existing recv code, and the fact that it's already February,
switching to "binary is the same as text" may well be the most prudent
path here.

regards, tom lane

On 02/05/2014 11:40 AM, Tom Lane wrote:
> Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
>> On Wed, Feb 5, 2014 at 10:22 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>>> The whole reason we have jsonb is to avoid reparsing where possible
>> Sure; but on the server side. The wire format is for handling client
>> concerns. For example, the case you're arguing for would be for libpq
>> client to extract as jsonb as binary, manipulate it on a binary level,
>> then send it back as binary. I find this case to be something of a
>> stretch.
> I'm with Merlin in thinking that the case for exposing a binary format
> to clients is pretty weak, or at least a convincing use-case has not
> been shown. Given the concerns upthread about security hazards in the
> patch's existing recv code, and the fact that it's already February,
> switching to "binary is the same as text" may well be the most prudent
> path here.
>
>

If we do that we're going to have to live with that forever, aren't we?
I don't see why there should be a convincing case for binary format for
nested hstore but not for jsonb.

If it were only for arbitrary libpq clietns I wouldn't bother so much.
To me the main case for binary format is that some people use COPY
BINARY for efficiency reasons, and I heard tell recently of someone
working on that as an option for pg_dump, which seems to me worth
considering.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 02/05/2014 11:40 AM, Tom Lane wrote:
>> switching to "binary is the same as text" may well be the most prudent
>> path here.

> If we do that we're going to have to live with that forever, aren't we?

Yeah, but the other side of that coin is that we'll have to live forever
with whatever binary format we pick, too. If it turns out to be badly
designed, that could be much worse than eating some parsing costs during
dump/restore.

If we had infinite time/manpower, this wouldn't really be an issue.
We don't, though, and so I suggest that this may be one of the better
things to toss overboard.

regards, tom lane

On 02/05/2014 12:48 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 02/05/2014 11:40 AM, Tom Lane wrote:
>>> switching to "binary is the same as text" may well be the most prudent
>>> path here.
>> If we do that we're going to have to live with that forever, aren't we?
> Yeah, but the other side of that coin is that we'll have to live forever
> with whatever binary format we pick, too. If it turns out to be badly
> designed, that could be much worse than eating some parsing costs during
> dump/restore.
>
> If we had infinite time/manpower, this wouldn't really be an issue.
> We don't, though, and so I suggest that this may be one of the better
> things to toss overboard.
>
>

The main reason I'm prepared to consider this is the JSON parser seems
to be fairly efficient (See Oleg's recent stats) and in fact we'd more
or less be parsing the binary format on input anyway, so there's no
proof that a binary format is going to be hugely faster (or possibly
even that it will be faster at all).

If anyone else has opinions on this sing out pretty darn soon (like the
next 24 hours or so, before I begin.)

cheers

andrew

On Wed, Feb 5, 2014 at 11:48 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> If we had infinite time/manpower, this wouldn't really be an issue.
> We don't, though, and so I suggest that this may be one of the better
> things to toss overboard.

The hstore send/recv functions have basically the same
(copy/pasted/name adjusted) implementation. Since hstore will
presumably remain (as the current hstore is) 'deep binary' and all of
Andres's gripes apply to the hstore as well, this change buys us
precisely zap from a time perspective; it comes down to which is
intrinsically the better choice.

merlin

On 02/05/2014 07:48 AM, Merlin Moncure wrote:
> Another point I'm struggling with is
> what jsonb brings to the table that isn't covered either hstore or
> json; working through a couple of cases I find myself not using the
> jsonb functionality except as a 'hstore json formatter' which the json
> type covers. I'm probably being obtuse, but we have to be cautious
> before plonking a couple of dozen extra functions in the public
> schema.

There's three reasons why it's worthwhile:

1) user-friendliness: telling users they need to do "::JSON" and
"::HSTORE2" all the time is sufficiently annoying -- and prone to
causing errors -- to be a blocker to adoption by a certain, very
numerous, class of user.

2) performance: to the extent that we can operate entirely in JSONB and
not transform back and forth to JSON and HSTORE, function calls (and
index lookups) will be much faster. And given the competition, speed is
important.

3) growth: 9.4's JSONB functions are a prerequisite to developing richer
JSON querying capabilities in 9.5 and later, which will go beyond "JSON
formatting for HSTORE".

Frankly, if it were entirely up to me HSTORE2 would be part of core and
its only interface would be JSONB. But it's not. So this is a compromise.

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

On 02/05/2014 02:03 PM, Josh Berkus wrote:

> Frankly, if it were entirely up to me HSTORE2 would be part of core and
> its only interface would be JSONB. But it's not. So this is a compromise.
>

You could only do that by inventing a new type. But hstore2 isn't a new
type, it's meant to be the existing hstore type with new capabilities.

Incidentally, some work is being done by one of my colleagues on an
extension of gin/gist operators for indexing jsonb similarly to hstore2.
Now that will possibly be something we can bring into 9.4, although
we'll have to check how we go about pg_upgrade for that case.

cheers

andrew

On Wed, Feb 5, 2014 at 1:03 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 02/05/2014 07:48 AM, Merlin Moncure wrote:
>> Another point I'm struggling with is
>> what jsonb brings to the table that isn't covered either hstore or
>> json; working through a couple of cases I find myself not using the
>> jsonb functionality except as a 'hstore json formatter' which the json
>> type covers. I'm probably being obtuse, but we have to be cautious
>> before plonking a couple of dozen extra functions in the public
>> schema.
>
> There's three reasons why it's worthwhile:
>
> 1) user-friendliness: telling users they need to do "::JSON" and
> "::HSTORE2" all the time is sufficiently annoying -- and prone to
> causing errors -- to be a blocker to adoption by a certain, very
> numerous, class of user.

That's a legitimate point of concern. But in and of itself I'm sure
sure it warrants exposing a separate API.

> 2) performance: to the extent that we can operate entirely in JSONB and
> not transform back and forth to JSON and HSTORE, function calls (and
> index lookups) will be much faster. And given the competition, speed is
> important.

Not following this. I do not see how the presence of jsonb helps at
all. Client to server communication will be text->binary (and vice
versa) and handling within the server itself will be in binary. This
is the crux of my point.

> 3) growth: 9.4's JSONB functions are a prerequisite to developing richer
> JSON querying capabilities in 9.5 and later, which will go beyond "JSON
> formatting for HSTORE".

I kind of get this point. But in lieu of a practical use case today,
what's the rush to implement? I fully anticipate I'm out on left
field on this one (I have a cot and mini fridge there). The question
on the table is: what use cases (performance included) does jsonb
solve that is not solve can't be solved without it? With the possible
limited exception of andrew's yet to be delivered enhanced
deserialization routines, I can't think of any. If presented with
reasonable evidence I'll shut my yap, pronto.

> Frankly, if it were entirely up to me HSTORE2 would be part of core and
> its only interface would be JSONB. But it's not. So this is a compromise.

I don't. To be pedantic: hstore is in core, but packaged as an
extension. That's a very important distinction.

In fact, I'll go further and say it seem wise for all SQL standard
type work to happen in extensions. As long as it's an in core contrib
extension, I see no stigma to that whatsoever. It's not clear at all
to me why the json type was put to the public schema and now we're
about to double down with jsonb. Having things extension packaged
greatly eases concerns about future API changes because if problems
emerge it's not impossible to imagine compatibility extensions to
appear to bridge the gap if certain critical functions change. That's
exactly the sort of thing that we may want to happen here, I think.

merlin

On 02/05/2014 03:15 PM, Merlin Moncure wrote:
> On Wed, Feb 5, 2014 at 1:03 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> On 02/05/2014 07:48 AM, Merlin Moncure wrote:
>>> Another point I'm struggling with is
>>> what jsonb brings to the table that isn't covered either hstore or
>>> json; working through a couple of cases I find myself not using the
>>> jsonb functionality except as a 'hstore json formatter' which the json
>>> type covers. I'm probably being obtuse, but we have to be cautious
>>> before plonking a couple of dozen extra functions in the public
>>> schema.
>> There's three reasons why it's worthwhile:
>>
>> 1) user-friendliness: telling users they need to do "::JSON" and
>> "::HSTORE2" all the time is sufficiently annoying -- and prone to
>> causing errors -- to be a blocker to adoption by a certain, very
>> numerous, class of user.
> That's a legitimate point of concern. But in and of itself I'm sure
> sure it warrants exposing a separate API.
>
>> 2) performance: to the extent that we can operate entirely in JSONB and
>> not transform back and forth to JSON and HSTORE, function calls (and
>> index lookups) will be much faster. And given the competition, speed is
>> important.
> Not following this. I do not see how the presence of jsonb helps at
> all. Client to server communication will be text->binary (and vice
> versa) and handling within the server itself will be in binary. This
> is the crux of my point.
>
>> 3) growth: 9.4's JSONB functions are a prerequisite to developing richer
>> JSON querying capabilities in 9.5 and later, which will go beyond "JSON
>> formatting for HSTORE".
> I kind of get this point. But in lieu of a practical use case today,
> what's the rush to implement? I fully anticipate I'm out on left
> field on this one (I have a cot and mini fridge there). The question
> on the table is: what use cases (performance included) does jsonb
> solve that is not solve can't be solved without it? With the possible
> limited exception of andrew's yet to be delivered enhanced
> deserialization routines, I can't think of any. If presented with
> reasonable evidence I'll shut my yap, pronto.
>
>> Frankly, if it were entirely up to me HSTORE2 would be part of core and
>> its only interface would be JSONB. But it's not. So this is a compromise.
> I don't. To be pedantic: hstore is in core, but packaged as an
> extension. That's a very important distinction.
>
> In fact, I'll go further and say it seem wise for all SQL standard
> type work to happen in extensions. As long as it's an in core contrib
> extension, I see no stigma to that whatsoever. It's not clear at all
> to me why the json type was put to the public schema and now we're
> about to double down with jsonb. Having things extension packaged
> greatly eases concerns about future API changes because if problems
> emerge it's not impossible to imagine compatibility extensions to
> appear to bridge the gap if certain critical functions change. That's
> exactly the sort of thing that we may want to happen here, I think.
>

The time for this discussion was months ago. I would not have spent many
many hours of my time if I thought it was going to be thrown away. I
find this attitude puzzling, to say the least. You were a major part of
the discussion when we said "OK, we'll leave json as it is (text based)
and add jsonb." That's exactly what we're doing.

And no, hstore is NOT in core. In core for a type means to me it's
builtin, with a fixed Oid.

cheers

andrew

On Wed, Feb 5, 2014 at 2:37 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> The time for this discussion was months ago. I would not have spent many
> many hours of my time if I thought it was going to be thrown away. I find
> this attitude puzzling, to say the least. You were a major part of the
> discussion when we said "OK, we'll leave json as it is (text based) and add
> jsonb." That's exactly what we're doing.

certainly. I'll shut my yap; I understand your puzzlement. At the
time though, I had assumed the API was going to incorporate more of
the hstore feature set than it did.

merlin

On 02/05/2014 03:45 PM, Merlin Moncure wrote:
> On Wed, Feb 5, 2014 at 2:37 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> The time for this discussion was months ago. I would not have spent many
>> many hours of my time if I thought it was going to be thrown away. I find
>> this attitude puzzling, to say the least. You were a major part of the
>> discussion when we said "OK, we'll leave json as it is (text based) and add
>> jsonb." That's exactly what we're doing.
> certainly. I'll shut my yap; I understand your puzzlement. At the
> time though, I had assumed the API was going to incorporate more of
> the hstore feature set than it did.
>

And we will. Specifically the indexing ops I mentioned upthread. We've
got done as much as could be done this cycle. That's how Postgres
development works.

One of the major complaints about json in 9.3 is that almost all the
functions and operators involve reparsing the json. The equivalent
operations for jsonb do not, and should accordingly be significantly
faster. That's what I have been spending my time on. I don't think
that's an inconsiderable advance.

cheers

andrew

Merlin,

> Not following this. I do not see how the presence of jsonb helps at
> all. Client to server communication will be text->binary (and vice
> versa) and handling within the server itself will be in binary. This
> is the crux of my point.

Except that handling it on the server, in binary, would require using
the HSTORE syntax. Otherwise you're converting from text JSON and back
whenever you want to nest functions.

> I kind of get this point. But in lieu of a practical use case today,
> what's the rush to implement? I fully anticipate I'm out on left
> field on this one (I have a cot and mini fridge there). The question
> on the table is: what use cases (performance included) does jsonb
> solve that is not solve can't be solved without it?

Indexed element extraction. JSON path queries. JSON manipulation.

If JSONB is in 9.4, then these are things we can build as extensions and
have available long before September 2015 -- in fact, we've already
started on a couple. If JSONB isn't in core as a data type, then we
have to wait for the 9.5 dev cycle to do anything.

> In fact, I'll go further and say it seem wise for all SQL standard
> type work to happen in extensions. As long as it's an in core contrib
> extension, I see no stigma to that whatsoever. It's not clear at all
> to me why the json type was put to the public schema and now we're
> about to double down with jsonb.

I'll agree that having hstore in contrib and json in core has been a
significant source of issues.

On 02/05/2014 12:45 PM, Merlin Moncure wrote:> certainly. I'll shut my
yap; I understand your puzzlement. At the
> time though, I had assumed the API was going to incorporate more of
> the hstore feature set than it did.

That was the original goal. However, Oleg and Teodor's late delivery of
Hstore2 limited what Andrew could do for JSONB before CF4 started.

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

On Wed, Feb 5, 2014 at 3:03 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> That was the original goal. However, Oleg and Teodor's late delivery of
> Hstore2 limited what Andrew could do for JSONB before CF4 started.

yeah. anyways, I'm good on this point.

merlin

On 02/05/2014 04:06 PM, Merlin Moncure wrote:
> On Wed, Feb 5, 2014 at 3:03 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> That was the original goal. However, Oleg and Teodor's late delivery of
>> Hstore2 limited what Andrew could do for JSONB before CF4 started.

I also had issues. But this is the sort of thing that happens. We get
done as much as we can.

cheers

andrew

On 02/05/2014 01:10 PM, Andrew Dunstan wrote:
>
> On 02/05/2014 12:48 PM, Tom Lane wrote:
>> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>> On 02/05/2014 11:40 AM, Tom Lane wrote:
>>>> switching to "binary is the same as text" may well be the most prudent
>>>> path here.
>>> If we do that we're going to have to live with that forever, aren't we?
>> Yeah, but the other side of that coin is that we'll have to live forever
>> with whatever binary format we pick, too. If it turns out to be badly
>> designed, that could be much worse than eating some parsing costs during
>> dump/restore.
>>
>> If we had infinite time/manpower, this wouldn't really be an issue.
>> We don't, though, and so I suggest that this may be one of the better
>> things to toss overboard.
>>
>>
>
>
> The main reason I'm prepared to consider this is the JSON parser seems
> to be fairly efficient (See Oleg's recent stats) and in fact we'd more
> or less be parsing the binary format on input anyway, so there's no
> proof that a binary format is going to be hugely faster (or possibly
> even that it will be faster at all).
>
> If anyone else has opinions on this sing out pretty darn soon (like
> the next 24 hours or so, before I begin.)

I got a slightly earlier start ;-) For people wanting to play along,
here's what this change looks like:
<https://github.com/feodor/postgres/commit/3fe899b3d7e8f806b14878da4a4e2331b0eb58e8>

I have a bit more cleanup to do and then I'll try to make new patches.

cheers

andrew

On 02/01/2014 05:20 PM, Andres Freund wrote:
>> diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
>> >index 1ae9fa0..fd93d9b 100644
>> >--- a/src/backend/utils/adt/Makefile
>> >+++ b/src/backend/utils/adt/Makefile
>> >@@ -32,7 +32,8 @@ OBJS = acl.o arrayfuncs.o array_selfuncs.o array_typanalyze.o \
>> > tsquery_op.o tsquery_rewrite.o tsquery_util.o tsrank.o \
>> > tsvector.o tsvector_op.o tsvector_parser.o \
>> > txid.o uuid.o windowfuncs.o xml.o rangetypes_spgist.o \
>> >- rangetypes_typanalyze.o rangetypes_selfuncs.o
>> >+ rangetypes_typanalyze.o rangetypes_selfuncs.o \
>> >+ jsonb.o jsonb_support.o
> Odd, most OBJS lines are kept in alphabetical order, but that doesn't
> seem to be the case here.

This whole list is a mess, and we don't even have all the range_types
files following each other.

Worth cleaning up?

I'm actually wondering if it might be worth having some subgroups of
object files and then combining them into $OBJS.

Or it could just be left more or less as is - it's hardly a breakthrough
advance.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 02/01/2014 05:20 PM, Andres Freund wrote:
>> Odd, most OBJS lines are kept in alphabetical order, but that doesn't
>> seem to be the case here.

> This whole list is a mess, and we don't even have all the range_types
> files following each other.

> Worth cleaning up?

+1. It's just neatnik-ism, but isn't compulsive neatnik-ism pretty
much a job requirement for programmers? It's hard enough dealing
with necessary complexities without having to wonder if some seemingly
arbitrary choice has hidden meanings.

> I'm actually wondering if it might be worth having some subgroups of
> object files and then combining them into $OBJS.

Nah, let's just alphabetize them and be done. The Makefile has no
reason to care about subgroups of those files.

regards, tom lane

Andrew Dunstan wrote:

> This whole list is a mess, and we don't even have all the
> range_types files following each other.
>
> Worth cleaning up?
>
> I'm actually wondering if it might be worth having some subgroups of
> object files and then combining them into $OBJS.

Doesn't the MSVC build stuff parse OBJS definitions?

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

On Feb 5, 2014, at 3:59 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

> I got a slightly earlier start ;-) For people wanting to play along, here's what this change looks like: <https://github.com/feodor/postgres/commit/3fe899b3d7e8f806b14878da4a4e2331b0eb58e8>

Man I love seeing all that read. :-)

D

On 02/06/2014 11:38 AM, Alvaro Herrera wrote:
> Andrew Dunstan wrote:
>
>> This whole list is a mess, and we don't even have all the
>> range_types files following each other.
>>
>> Worth cleaning up?
>>
>> I'm actually wondering if it might be worth having some subgroups of
>> object files and then combining them into $OBJS.
> Doesn't the MSVC build stuff parse OBJS definitions?
>

Good point. At least in some cases it does.

cheers

andrew

On 02/05/2014 10:36 AM, Teodor Sigaev wrote:
> Should I make new version of patch? Right now it's placed on github.
> May be Andrew wants to change something?
>

Attached are updated patches.

Apart from the things Teodor has fixed, this includes

* switching to using text representation in jsonb send/recv
* implementation of jsonb_array_elements_text that we need now we have
json_array_elements_text
* some code fixes requested in code reviews, plus some other tidying
and refactoring.

cheers

andrew

On Fri, Feb 7, 2014 at 1:18 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 02/01/2014 05:20 PM, Andres Freund wrote:
>>>
>>> diff --git a/src/backend/utils/adt/Makefile
>>> b/src/backend/utils/adt/Makefile
>>> >index 1ae9fa0..fd93d9b 100644
>>> >--- a/src/backend/utils/adt/Makefile
>>> >+++ b/src/backend/utils/adt/Makefile
>>> >@@ -32,7 +32,8 @@ OBJS = acl.o arrayfuncs.o array_selfuncs.o
>>> > array_typanalyze.o \
>>> > tsquery_op.o tsquery_rewrite.o tsquery_util.o tsrank.o \
>>> > tsvector.o tsvector_op.o tsvector_parser.o \
>>> > txid.o uuid.o windowfuncs.o xml.o rangetypes_spgist.o \
>>> >- rangetypes_typanalyze.o rangetypes_selfuncs.o
>>> >+ rangetypes_typanalyze.o rangetypes_selfuncs.o \
>>> >+ jsonb.o jsonb_support.o
>>
>> Odd, most OBJS lines are kept in alphabetical order, but that doesn't
>> seem to be the case here.
>
>
>
> This whole list is a mess, and we don't even have all the range_types files
> following each other.
>
> Worth cleaning up?
+1. Yes please.
--
Michael

On Fri, February 7, 2014 00:47, Andrew Dunstan wrote:
>
> Attached are updated patches.
>
> jsonb-10.patch.gz
> nested-hstore-10.patch.gz

Small changes to json documentation, mostly of typo caliber.

Thanks,

Erik Rijkers

On 02/06/2014 01:48 AM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 02/05/2014 11:40 AM, Tom Lane wrote:
>>> switching to "binary is the same as text" may well be the most prudent
>>> path here.
>
>> If we do that we're going to have to live with that forever, aren't we?
>
> Yeah, but the other side of that coin is that we'll have to live forever
> with whatever binary format we pick, too. If it turns out to be badly
> designed, that could be much worse than eating some parsing costs during
> dump/restore.
>
> If we had infinite time/manpower, this wouldn't really be an issue.
> We don't, though, and so I suggest that this may be one of the better
> things to toss overboard.

Can't we just reject attempts to transfer these via binary copy,
allowing only a text format? So rather than sending text when the binary
is requested, we just require clients to use text for this type.

That way it's possible to add the desired binary format later, without
rushed decisions.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 02/05/2014 06:48 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 02/05/2014 11:40 AM, Tom Lane wrote:
>>> switching to "binary is the same as text" may well be the most prudent
>>> path here.
>> If we do that we're going to have to live with that forever, aren't we?
> Yeah, but the other side of that coin is that we'll have to live forever
> with whatever binary format we pick, too. If it turns out to be badly
> designed, that could be much worse than eating some parsing costs during
> dump/restore.
The fastest and lowest parsing cost format for "JSON" is tnetstrings
http://tnetstrings.org/ why not use it as the binary wire format ?

It would be as binary as it gets and still be generally parse-able by
lots of different platforms, at leas by all of these we care about.

Cheers
Hannu

Hi,

On 2014-02-06 18:47:31 -0500, Andrew Dunstan wrote:
> * switching to using text representation in jsonb send/recv

> +/*
> + * jsonb type recv function
> + *
> + * the type is sent as text in binary mode, so this is almost the same
> + * as the input function.
> + */
> +Datum
> +jsonb_recv(PG_FUNCTION_ARGS)
> +{
> + StringInfo buf = (StringInfo) PG_GETARG_POINTER(0);
> + text *result = cstring_to_text_with_len(buf->data, buf->len);
> +
> + return deserialize_json_text(result);
> +}

> +/*
> + * jsonb type send function
> + *
> + * Just send jsonb as a string of text
> + */
> +Datum
> +jsonb_send(PG_FUNCTION_ARGS)
> +{
> + Jsonb *jb = PG_GETARG_JSONB(0);
> + StringInfoData buf;
> + char *out;
> +
> + out = JsonbToCString(NULL, (JB_ISEMPTY(jb)) ? NULL : VARDATA(jb), VARSIZE(jb));
> +
> + pq_begintypsend(&buf);
> + pq_sendtext(&buf, out, strlen(out));
> + PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
> +}

I'd suggest making the format discernible from possible different future
formats, to allow introducing a proper binary at some later time. Maybe
just send a int8 first, containing the format.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 02/10/2014 11:05 AM, Andres Freund wrote:
> Hi,
>
> On 2014-02-06 18:47:31 -0500, Andrew Dunstan wrote:
>> * switching to using text representation in jsonb send/recv
>> +/*
>> + * jsonb type recv function
>> + *
>> + * the type is sent as text in binary mode, so this is almost the same
>> + * as the input function.
>> + */
>> +Datum
>> +jsonb_recv(PG_FUNCTION_ARGS)
>> +{
>> + StringInfo buf = (StringInfo) PG_GETARG_POINTER(0);
>> + text *result = cstring_to_text_with_len(buf->data, buf->len);
>> +
>> + return deserialize_json_text(result);
>> +}
>> +/*
>> + * jsonb type send function
>> + *
>> + * Just send jsonb as a string of text
>> + */
>> +Datum
>> +jsonb_send(PG_FUNCTION_ARGS)
>> +{
>> + Jsonb *jb = PG_GETARG_JSONB(0);
>> + StringInfoData buf;
>> + char *out;
>> +
>> + out = JsonbToCString(NULL, (JB_ISEMPTY(jb)) ? NULL : VARDATA(jb), VARSIZE(jb));
>> +
>> + pq_begintypsend(&buf);
>> + pq_sendtext(&buf, out, strlen(out));
>> + PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
>> +}
> I'd suggest making the format discernible from possible different future
> formats, to allow introducing a proper binary at some later time. Maybe
> just send a int8 first, containing the format.
+10

Especially as this is one type where we may want add type-specific
compression options at some point

Cheers

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On 02/10/2014 05:05 AM, Andres Freund wrote:
> Hi,
>
> On 2014-02-06 18:47:31 -0500, Andrew Dunstan wrote:
>> * switching to using text representation in jsonb send/recv
>> +/*
>> + * jsonb type recv function
>> + *
>> + * the type is sent as text in binary mode, so this is almost the same
>> + * as the input function.
>> + */
>> +Datum
>> +jsonb_recv(PG_FUNCTION_ARGS)
>> +{
>> + StringInfo buf = (StringInfo) PG_GETARG_POINTER(0);
>> + text *result = cstring_to_text_with_len(buf->data, buf->len);
>> +
>> + return deserialize_json_text(result);
>> +}
>> +/*
>> + * jsonb type send function
>> + *
>> + * Just send jsonb as a string of text
>> + */
>> +Datum
>> +jsonb_send(PG_FUNCTION_ARGS)
>> +{
>> + Jsonb *jb = PG_GETARG_JSONB(0);
>> + StringInfoData buf;
>> + char *out;
>> +
>> + out = JsonbToCString(NULL, (JB_ISEMPTY(jb)) ? NULL : VARDATA(jb), VARSIZE(jb));
>> +
>> + pq_begintypsend(&buf);
>> + pq_sendtext(&buf, out, strlen(out));
>> + PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
>> +}
> I'd suggest making the format discernible from possible different future
> formats, to allow introducing a proper binary at some later time. Maybe
> just send a int8 first, containing the format.
>

Teodor privately suggested something similar. I was thinking of just
sending a version byte, which for now would be '\x01'. An int8 seems
like more future-proofing provision than we really need.

cheers

andrew

On 2014-02-10 07:27:59 -0500, Andrew Dunstan wrote:
> On 02/10/2014 05:05 AM, Andres Freund wrote:
> >I'd suggest making the format discernible from possible different future
> >formats, to allow introducing a proper binary at some later time. Maybe
> >just send a int8 first, containing the format.
> >
>
> Teodor privately suggested something similar. I was thinking of just
> sending a version byte, which for now would be '\x01'. An int8 seems like
> more future-proofing provision than we really need.

Hm. Isn't that just about the same? I was thinking of the c type int8,
not the 64bit type. It seems cleaner to do a pg_sendint(..., 1, 1) than
to do it manually inside the string.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 02/10/2014 07:39 AM, Andres Freund wrote:
> On 2014-02-10 07:27:59 -0500, Andrew Dunstan wrote:
>> On 02/10/2014 05:05 AM, Andres Freund wrote:
>>> I'd suggest making the format discernible from possible different future
>>> formats, to allow introducing a proper binary at some later time. Maybe
>>> just send a int8 first, containing the format.
>>>
>> Teodor privately suggested something similar. I was thinking of just
>> sending a version byte, which for now would be '\x01'. An int8 seems like
>> more future-proofing provision than we really need.
> Hm. Isn't that just about the same? I was thinking of the c type int8,
> not the 64bit type. It seems cleaner to do a pg_sendint(..., 1, 1) than
> to do it manually inside the string.

OK, works for me. I'm tied up for a couple of days, will do this when
I'm back on deck.

cheers

andrew

Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> On 02/06/2014 01:48 AM, Tom Lane wrote:
>>> switching to "binary is the same as text" may well be the most prudent
>>> path here.

> Can't we just reject attempts to transfer these via binary copy,
> allowing only a text format? So rather than sending text when the binary
> is requested, we just require clients to use text for this type.

That used to be the case, back when we didn't have send/recv functions for
all built-in types; and client-code authors complained bitterly about it.
It's pretty much unworkable if the text/binary choice is being made by
a code level that doesn't have complete understanding of the queries it's
transmitting. Consider "SELECT * FROM ..."; how are you going to know
which columns to request in binary and which in text? Even if you're
willing to do trial and error (ie, it's okay to cause transaction
rollbacks), the backend isn't very helpful about telling you exactly
which column(s) would need to be requested as text.

I think the downthread solution of prepending a type-specific format ID
byte is a better answer for giving us flexibility down the road.

regards, tom lane

On Mon, Feb 10, 2014 at 6:39 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-02-10 07:27:59 -0500, Andrew Dunstan wrote:
>> On 02/10/2014 05:05 AM, Andres Freund wrote:
>> >I'd suggest making the format discernible from possible different future
>> >formats, to allow introducing a proper binary at some later time. Maybe
>> >just send a int8 first, containing the format.
>> >
>>
>> Teodor privately suggested something similar. I was thinking of just
>> sending a version byte, which for now would be '\x01'. An int8 seems like
>> more future-proofing provision than we really need.
>
> Hm. Isn't that just about the same? I was thinking of the c type int8,
> not the 64bit type. It seems cleaner to do a pg_sendint(..., 1, 1) than
> to do it manually inside the string.

-1. Currently no other wire format types send version and it's not
clear why this one is special. We've changed the wire format versions
before and it's upon the client to deal with those changes. The
server version *is* the version basically. If a broader solution
exists I think it should be addressed broadly. Versioning one type
only IMNSHO is a complete hack.

merlin

Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> On Mon, Feb 10, 2014 at 6:39 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> On 2014-02-10 07:27:59 -0500, Andrew Dunstan wrote:
>>> Teodor privately suggested something similar. I was thinking of just
>>> sending a version byte, which for now would be '\x01'. An int8 seems like
>>> more future-proofing provision than we really need.

> -1. Currently no other wire format types send version and it's not
> clear why this one is special. We've changed the wire format versions
> before and it's upon the client to deal with those changes.

Really? How would you expect to do that, exactly? In particular,
how would you propose that a binary pg_dump file be reloadable if
we redefine the binary format down the road without having made
provision like this?

> Versioning one type only IMNSHO is a complete hack.

I don't feel a need for versioning int, or float8, or most other types;
and that includes the ones for which we've previously defined binary
format as equivalent to text (enums). In this case we know that we're not
totally satisfied with the binary format we're defining today, so I think
a type-specific escape hatch is a reasonable solution.

Moreover, I don't especially buy tying it to server version, even if we
had an information pathway that would provide that reliably in all
contexts. Granting the presumption that more than one data type would
want such versioning, it's still possible that different data types would
have different ideas about what they needed to do and where the cutover
points were.

regards, tom lane

On Mon, Feb 10, 2014 at 12:15 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
>> On Mon, Feb 10, 2014 at 6:39 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> On 2014-02-10 07:27:59 -0500, Andrew Dunstan wrote:
>>>> Teodor privately suggested something similar. I was thinking of just
>>>> sending a version byte, which for now would be '\x01'. An int8 seems like
>>>> more future-proofing provision than we really need.
>
>> -1. Currently no other wire format types send version and it's not
>> clear why this one is special. We've changed the wire format versions
>> before and it's upon the client to deal with those changes.
>
> Really? How would you expect to do that, exactly? In particular,
> how would you propose that a binary pg_dump file be reloadable if
> we redefine the binary format down the road without having made
> provision like this?
>
>> Versioning one type only IMNSHO is a complete hack.
>
> I don't feel a need for versioning int, or float8, or most other types;
> and that includes the ones for which we've previously defined binary
> format as equivalent to text (enums). In this case we know that we're not
> totally satisfied with the binary format we're defining today, so I think
> a type-specific escape hatch is a reasonable solution.
>
> Moreover, I don't especially buy tying it to server version, even if we
> had an information pathway that would provide that reliably in all
> contexts.

Why not? Furthermore what are we doing now? If we need a binary
format contract that needs to be separated from this discussion.

I've written (along with Andrew C) the only serious attempt to deal
with client side binary format handling (http://libpqtypes.esilo.com/)
and in all interesting cases it depends on the server version to
define binary parsing behaviors. I agree WRT float8, etc but other
types have changed in a couple of cases and it's always been with the
version. I find it highly unlikely that any compatibility behaviors
are going to be defined *for each and every returned datum* now, or
ever...so even if it's a few bytes lost, why do it? Intra-version
compatibility issues should they ever have to be handled would be more
likely handled at connection- or query- time.

Point being, if an escape hatch is needed, I'm near 100% certain this
is not the right place to do it. Binary wire format compatibility is
a complex topic and proposed solution ISTM is not at all fleshed out.

merlin

On 2014-02-10 11:59:53 -0600, Merlin Moncure wrote:
> On Mon, Feb 10, 2014 at 6:39 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2014-02-10 07:27:59 -0500, Andrew Dunstan wrote:
> >> On 02/10/2014 05:05 AM, Andres Freund wrote:
> >> >I'd suggest making the format discernible from possible different future
> >> >formats, to allow introducing a proper binary at some later time. Maybe
> >> >just send a int8 first, containing the format.
> >> >
> >>
> >> Teodor privately suggested something similar. I was thinking of just
> >> sending a version byte, which for now would be '\x01'. An int8 seems like
> >> more future-proofing provision than we really need.
> >
> > Hm. Isn't that just about the same? I was thinking of the c type int8,
> > not the 64bit type. It seems cleaner to do a pg_sendint(..., 1, 1) than
> > to do it manually inside the string.
>
> -1. Currently no other wire format types send version and it's not
> clear why this one is special. We've changed the wire format versions
> before and it's upon the client to deal with those changes. The
> server version *is* the version basically. If a broader solution
> exists I think it should be addressed broadly. Versioning one type
> only IMNSHO is a complete hack.

I don't find that very convincing. The entire reason jsonb exists is
because the parsing overhead of text json is significant, so it stands
to reason that soon somebody will try to work on a better wire protocol,
even if the current code cannot be made ready for 9.4. And I don't think
past instability of binary type's formats is a good reason for
*needlessly* breaking stuff like binary COPYs.
And it's not like one prefixed byte has any real-world relevant cost.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Feb 10, 2014 at 5:02 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-02-10 11:59:53 -0600, Merlin Moncure wrote:
>> On Mon, Feb 10, 2014 at 6:39 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > On 2014-02-10 07:27:59 -0500, Andrew Dunstan wrote:
>> >> On 02/10/2014 05:05 AM, Andres Freund wrote:
>> >> >I'd suggest making the format discernible from possible different future
>> >> >formats, to allow introducing a proper binary at some later time. Maybe
>> >> >just send a int8 first, containing the format.
>> >> >
>> >>
>> >> Teodor privately suggested something similar. I was thinking of just
>> >> sending a version byte, which for now would be '\x01'. An int8 seems like
>> >> more future-proofing provision than we really need.
>> >
>> > Hm. Isn't that just about the same? I was thinking of the c type int8,
>> > not the 64bit type. It seems cleaner to do a pg_sendint(..., 1, 1) than
>> > to do it manually inside the string.
>>
>> -1. Currently no other wire format types send version and it's not
>> clear why this one is special. We've changed the wire format versions
>> before and it's upon the client to deal with those changes. The
>> server version *is* the version basically. If a broader solution
>> exists I think it should be addressed broadly. Versioning one type
>> only IMNSHO is a complete hack.
>
> I don't find that very convincing. The entire reason jsonb exists is
> because the parsing overhead of text json is significant, so it stands
> to reason that soon somebody will try to work on a better wire protocol,
> even if the current code cannot be made ready for 9.4. And I don't think
> past instability of binary type's formats is a good reason for
> *needlessly* breaking stuff like binary COPYs.
> And it's not like one prefixed byte has any real-world relevant cost.

The point is, why does this one type get a version id? Imagine a
hypothetical program that sent/received the binary format for jsonb.
All you have to to is manage the version flag appropriately, right?

Wrong. You still need to have code that checks the server version and
see if it's supported (particularly for sending) and as there is *no
protocol negotiation of the formats at present it's all going to boil
down to if version = X do Y*. How does the server know which
'versions' are ok to send? It doesn't. Follow along with me here:
Suppose we don't introduce a version flag today and change the format
to some more exotic structure for 9.5. How has the version flag made
things easier for the client? It hasn't. The client goes "if version
= X do Y".

I guess you could argue that having a version flag could, say, allow
libpq clients to gracefully error out if, say, a old non-exotic-format
speaking libpq happens to connect to a newer sever -- assuming the
client actually bothered to check the flag. That's zero help to the
client though -- regardless the compatibility isn't established and
that's zero help to other binary formats that we have=, and probably
will continue to-, change. What about them? Are we now, at the
upteenth hour of the final commit fest, suddenly deciding that binary
wire formats going to be compatible across versions?

The kinda low effort way to deal with binary format compatibility is
to simply document the existing formats and document format changes in
some convenient place. The 'real' long term path to doing it IMO is
to abstract out a shared/client server type library with some protocol
negotiation features. Then, at connection time, the client/server
agree on what's the optimal way to send things -- perhaps the client
can signal things like 'want compression for long datums'.

The only case for a version flag at the data point level is if the
server is sending version X at this tuple and version Y at that tuple.
I don't think that's a makable case. Some might say, "what about a
compression bit based on compressibility/length?" and to that I'd
answer: why is that handling specific to the json type...are
text/bytea/arrays not worth that feature too?

merlin

On 2014-02-10 17:35:12 -0600, Merlin Moncure wrote:
> Wrong. You still need to have code that checks the server version and
> see if it's supported (particularly for sending) and as there is *no
> protocol negotiation of the formats at present it's all going to boil
> down to if version = X do Y*. How does the server know which
> 'versions' are ok to send? It doesn't. Follow along with me here:
> Suppose we don't introduce a version flag today and change the format
> to some more exotic structure for 9.5. How has the version flag made
> things easier for the client? It hasn't. The client goes "if version
> = X do Y".

think of binary COPY outputting data in 9.4 and then trying to import
that data into 9.5. That's the interesting case here.

> What about them? Are we now, at the
> upteenth hour of the final commit fest, suddenly deciding that binary
> wire formats going to be compatible across versions?

It has been a concern before.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Feb 10, 2014 at 5:38 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-02-10 17:35:12 -0600, Merlin Moncure wrote:
>> Wrong. You still need to have code that checks the server version and
>> see if it's supported (particularly for sending) and as there is *no
>> protocol negotiation of the formats at present it's all going to boil
>> down to if version = X do Y*. How does the server know which
>> 'versions' are ok to send? It doesn't. Follow along with me here:
>> Suppose we don't introduce a version flag today and change the format
>> to some more exotic structure for 9.5. How has the version flag made
>> things easier for the client? It hasn't. The client goes "if version
>> = X do Y".
>
> think of binary COPY outputting data in 9.4 and then trying to import
> that data into 9.5. That's the interesting case here.

right, json could be made work, but any other format change introduced
to any other already existing type will break. That's not a real
solution unless we decree henceforth that no formats will change from
here on in, in which case I withdraw my objection.

I think COPY binary has exactly the same set of considerations as the
client side. If you want to operate cleanly between versions (which
has never been promised in the past), you have to encode in a header
the kinds of things the server would need to parse it properly.
Starting with, but not necessarily limited to, the encoding server's
version.

merlin

On 2014-02-10 17:48:32 -0600, Merlin Moncure wrote:
> On Mon, Feb 10, 2014 at 5:38 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2014-02-10 17:35:12 -0600, Merlin Moncure wrote:
> >> Wrong. You still need to have code that checks the server version and
> >> see if it's supported (particularly for sending) and as there is *no
> >> protocol negotiation of the formats at present it's all going to boil
> >> down to if version = X do Y*. How does the server know which
> >> 'versions' are ok to send? It doesn't. Follow along with me here:
> >> Suppose we don't introduce a version flag today and change the format
> >> to some more exotic structure for 9.5. How has the version flag made
> >> things easier for the client? It hasn't. The client goes "if version
> >> = X do Y".
> >
> > think of binary COPY outputting data in 9.4 and then trying to import
> > that data into 9.5. That's the interesting case here.
>
> right, json could be made work, but any other format change introduced
> to any other already existing type will break. That's not a real
> solution unless we decree henceforth that no formats will change from
> here on in, in which case I withdraw my objection.

Sure, it's not a full solution. But it's better than nothing, and it's
likely that we'll see breakage soonish. I don't think there's been much
recent mucking around with incompatible binary formats?

> I think COPY binary has exactly the same set of considerations as the
> client side. If you want to operate cleanly between versions (which
> has never been promised in the past), you have to encode in a header
> the kinds of things the server would need to parse it properly.
> Starting with, but not necessarily limited to, the encoding server's
> version.

It works in enough cases atm that it's worthwile trying to keep it
working. Sure, it could be better, but it's what we have right now. Atm
it's e.g. the only realistic way to copy larger amounts of bytea between
servers without copying the entire cluster.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Feb 10, 2014 at 5:52 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> It works in enough cases atm that it's worthwile trying to keep it
> working. Sure, it could be better, but it's what we have right now. Atm
> it's e.g. the only realistic way to copy larger amounts of bytea between
> servers without copying the entire cluster.

That's the thing -- it might work today, but what about tomorrow?
We'd be sending the wrong signals. People start building processes
around all of this and now we've painted ourselves into a box. Better
in my mind to simply educate users that this practice is dangerous and
unsupported, as we used to do. I guess until now. It seems completely
odd to me that we're attaching a case to the jsonb type, in the wrong
way -- something that we've never attached to any other type before.
For example, why didn't we attach a version code to the json type send
function? Wasn't the whole point of this is that jsonb send/recv be
more spiritually closer to json? If we want to introduce alternative
type formats in the 9.5 cycle, why can't we attach version based
encoding handling to *that* problem?

The more angles I look at this from the more it looks messy and rushed.

Notwithstanding all the above, I figure here enough smart people
disagree (once again, heh) to call it consensus.

merlin

On 2014-02-10 18:16:15 -0600, Merlin Moncure wrote:
> On Mon, Feb 10, 2014 at 5:52 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > It works in enough cases atm that it's worthwile trying to keep it
> > working. Sure, it could be better, but it's what we have right now. Atm
> > it's e.g. the only realistic way to copy larger amounts of bytea between
> > servers without copying the entire cluster.
>
> That's the thing -- it might work today, but what about tomorrow?
> We'd be sending the wrong signals. People start building processes
> around all of this and now we've painted ourselves into a box.

That ship has sailed.

> Better in my mind to simply educate users that this practice is dangerous and
> unsupported, as we used to do.

But we don't have any alternatives for such scenarios, so that just
amounts to "screw you". If there are good reason for just breaking
binary protocol compatibility, I can live with that, but that's really
not the case here. The additional amount of code is *miniscule*, even
after adding a real binary protocol format since all the code has to be
there for the plain send/recv functions anyway.

The amount of interesting and acceptable binary protocol changes has
gotten lower in step with the acceptance of on-disk compatibility
changes, which isn't particularly surprising.

> I guess until now. It seems completely
> odd to me that we're attaching a case to the jsonb type, in the wrong
> way -- something that we've never attached to any other type before.
> For example, why didn't we attach a version code to the json type send
> function? Wasn't the whole point of this is that jsonb send/recv be
> more spiritually closer to json? If we want to introduce alternative
> type formats in the 9.5 cycle, why can't we attach version based
> encoding handling to *that* problem?

That doesn't make any sense to me. jsonb is a separate type because it
behaves differently than json. So I don't see how that plays a role
here.

And if we add a new format version in 9.5 we need to make it discernible
from the 9.4 format. Without space for a format indicator we'd have to
resort to ugly tricks like defining the high bit in the first byte set
indicates the new version. I don't see the improvement here.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> right, json could be made work, but any other format change introduced
> to any other already existing type will break. That's not a real
> solution unless we decree henceforth that no formats will change from
> here on in, in which case I withdraw my objection.

Well, I don't recall that we've made a practice of changing binary formats
a lot. Doing so would break existing dumps, which is something we
strenuously avoid.

Even granting that sometime in the future we invent infrastructure to do
the kind of protocol negotiation you're talking about, one byte per JSON
value seems like a cheap and worthwhile cross-check that both ends came
to the same conclusion about what to send.

regards, tom lane

On Mon, Feb 10, 2014 at 6:24 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> And if we add a new format version in 9.5 we need to make it discernible
> from the 9.4 format. Without space for a format indicator we'd have to
> resort to ugly tricks like defining the high bit in the first byte set
> indicates the new version. I don't see the improvement here.

Point being: a 9.5 binary format reading server could look for a magic
token in the beginning of the file which would indicate the presence
of a header. The server could then make intelligent decisions about
reading data inside the file which would be follow exactly the same
kinds of decisions binary format consuming client code would make.
Perhaps it would be a simple check on version, or something more
complex that would involve a negotiation. The 'format' indicator,
should version not be precise enough, needs to be in the header, not
passed with every instance of the data type, and certainly not for one
type in the absence of others.

merlin

Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> On Mon, Feb 10, 2014 at 6:24 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> And if we add a new format version in 9.5 we need to make it discernible
>> from the 9.4 format. Without space for a format indicator we'd have to
>> resort to ugly tricks like defining the high bit in the first byte set
>> indicates the new version. I don't see the improvement here.

> Point being: a 9.5 binary format reading server could look for a magic
> token in the beginning of the file which would indicate the presence
> of a header. The server could then make intelligent decisions about
> reading data inside the file which would be follow exactly the same
> kinds of decisions binary format consuming client code would make.
> Perhaps it would be a simple check on version, or something more
> complex that would involve a negotiation. The 'format' indicator,
> should version not be precise enough, needs to be in the header, not
> passed with every instance of the data type, and certainly not for one
> type in the absence of others.

Basically, you want to move the goalposts to somewhere that's not only
out of reach today, but probably a few counties away from the stadium.
I don't see this happening at all frankly, because nobody has been
interested enough to work on something like it up to now. And I
definitely don't see it as appropriate to block improvement of jsonb
until this happens.

regards, tom lane

On Mon, Feb 10, 2014 at 6:39 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
>> On Mon, Feb 10, 2014 at 6:24 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> And if we add a new format version in 9.5 we need to make it discernible
>>> from the 9.4 format. Without space for a format indicator we'd have to
>>> resort to ugly tricks like defining the high bit in the first byte set
>>> indicates the new version. I don't see the improvement here.
>
>> Point being: a 9.5 binary format reading server could look for a magic
>> token in the beginning of the file which would indicate the presence
>> of a header. The server could then make intelligent decisions about
>> reading data inside the file which would be follow exactly the same
>> kinds of decisions binary format consuming client code would make.
>> Perhaps it would be a simple check on version, or something more
>> complex that would involve a negotiation. The 'format' indicator,
>> should version not be precise enough, needs to be in the header, not
>> passed with every instance of the data type, and certainly not for one
>> type in the absence of others.
>
> Basically, you want to move the goalposts to somewhere that's not only
> out of reach today, but probably a few counties away from the stadium.
> I don't see this happening at all frankly, because nobody has been
> interested enough to work on something like it up to now. And I
> definitely don't see it as appropriate to block improvement of jsonb
> until this happens.

That's completely unfair. I'm arguing *not* to attach version
dependency expectations to the jsonb type, at all, not the other way
around. If you want to do that, fine, but do it *later* as in, 9.5,
or beyond. I just gave an example of how binary format changes could
be worked in later.

merlin

On 2014-02-10 19:01:48 -0600, Merlin Moncure wrote:
> On Mon, Feb 10, 2014 at 6:39 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> >> On Mon, Feb 10, 2014 at 6:24 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >>> And if we add a new format version in 9.5 we need to make it discernible
> >>> from the 9.4 format. Without space for a format indicator we'd have to
> >>> resort to ugly tricks like defining the high bit in the first byte set
> >>> indicates the new version. I don't see the improvement here.
> >
> >> Point being: a 9.5 binary format reading server could look for a magic
> >> token in the beginning of the file which would indicate the presence
> >> of a header. The server could then make intelligent decisions about
> >> reading data inside the file which would be follow exactly the same
> >> kinds of decisions binary format consuming client code would make.
> >> Perhaps it would be a simple check on version, or something more
> >> complex that would involve a negotiation. The 'format' indicator,
> >> should version not be precise enough, needs to be in the header, not
> >> passed with every instance of the data type, and certainly not for one
> >> type in the absence of others.
> >
> > Basically, you want to move the goalposts to somewhere that's not only
> > out of reach today, but probably a few counties away from the stadium.
> > I don't see this happening at all frankly, because nobody has been
> > interested enough to work on something like it up to now. And I
> > definitely don't see it as appropriate to block improvement of jsonb
> > until this happens.
>
> That's completely unfair. I'm arguing *not* to attach version
> dependency expectations to the jsonb type, at all, not the other way
> around. If you want to do that, fine, but do it *later* as in, 9.5,
> or beyond. I just gave an example of how binary format changes could
> be worked in later.