Обсуждение: json (b) and null fields

I should have been paying a bit more attention to the recent work on 
adding an ignore_nulls option to row_to_json(). Here are some belated 
thought. I apologize to Pavel and Stephen for not having commented earlier.

I think this is really a bandaid, and it will fail to catch lots of 
cases. Several examples:
 * it doesn't apply recursively, so if the row has a nested composite,   or an array of composites, none of those will
havenulls ignored,   only the top level will. * it doesn't apply to other json generators, notably json_agg().   That's
avery big omission. * it does nothing to allow us to strip nulls from existing json/jsonb   data.
 

I think a much more comprehensive solution would be preferable. What I 
have in mind is something like
    json_strip_null_fields(json) -> json

and a similar function for jsonb.

These would operate recursively. There is a downside, in that they would 
be required to reprocess the json/jsonb. But adding an option like this 
to all the json generator functions would be seriously ugly, especially 
since they are mostly aggregate functions or variadic functions. At 
least in the jsonb case the cost of reprocessing is likely to be fairly low.

cheers

andrew

Andrew, all,

* Andrew Dunstan (andrew@dunslane.net) wrote:
> I should have been paying a bit more attention to the recent work on
> adding an ignore_nulls option to row_to_json(). Here are some
> belated thought. I apologize to Pavel and Stephen for not having
> commented earlier.

No problem at all and thanks for continuing to think about it!  We
certainly still have quite a bit of time til 9.5 to get this right.

> I think this is really a bandaid, and it will fail to catch lots of
> cases. Several examples:

As discussed on IRC- I agree.  I tend to think of JSON objects as
relatively simple hstore-like structures and so hadn't considered the
complex structure case (as I'm guessing Pavel hadn't either).

> I think a much more comprehensive solution would be preferable. What
> I have in mind is something like
>
>     json_strip_null_fields(json) -> json
>
> and a similar function for jsonb.

Right, this makes sense to me.

> These would operate recursively. There is a downside, in that they
> would be required to reprocess the json/jsonb. But adding an option
> like this to all the json generator functions would be seriously
> ugly, especially since they are mostly aggregate functions or
> variadic functions. At least in the jsonb case the cost of
> reprocessing is likely to be fairly low.

Yeah, I don't see adding this option to all json generator functions as
making a lot of sense but rather just to the select few things which it
really makes sense for and then having a function which can be used by
users to do the same for results from other operations.
Thanks!
    Stephen

On 09/27/2014 08:00 AM, Stephen Frost wrote:
> Andrew, all,
>
> * Andrew Dunstan (andrew@dunslane.net) wrote:
>> I should have been paying a bit more attention to the recent work on
>> adding an ignore_nulls option to row_to_json(). Here are some
>> belated thought. I apologize to Pavel and Stephen for not having
>> commented earlier.
> No problem at all and thanks for continuing to think about it!  We
> certainly still have quite a bit of time til 9.5 to get this right.
>
>> I think this is really a bandaid, and it will fail to catch lots of
>> cases. Several examples:
> As discussed on IRC- I agree.  I tend to think of JSON objects as
> relatively simple hstore-like structures and so hadn't considered the
> complex structure case (as I'm guessing Pavel hadn't either).
>
>> I think a much more comprehensive solution would be preferable. What
>> I have in mind is something like
>>
>>      json_strip_null_fields(json) -> json
>>
>> and a similar function for jsonb.
> Right, this makes sense to me.
>
>> These would operate recursively. There is a downside, in that they
>> would be required to reprocess the json/jsonb. But adding an option
>> like this to all the json generator functions would be seriously
>> ugly, especially since they are mostly aggregate functions or
>> variadic functions. At least in the jsonb case the cost of
>> reprocessing is likely to be fairly low.
> Yeah, I don't see adding this option to all json generator functions as
> making a lot of sense but rather just to the select few things which it
> really makes sense for and then having a function which can be used by
> users to do the same for results from other operations.
>
>     


I guess I'm questioning the wisdom of keeping it for row_to_json given 
that it doesn't operate recursively anyway (and making it do so would be 
difficult and ugly).

The counter argument for this is that nested composites and arrays of 
composites are relatively rare in records, so providing a fast 
non-recursive stripping of nulls for the common case is reasonable.

If we're going to keep this, I think we also need to provide it 
(non-recursive) for json_agg via an optional second argument. This 
should be a fairly simple change: just steer the result via 
composite_to_json if it's a record, rather than to datum_to_json.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 09/27/2014 08:00 AM, Stephen Frost wrote:
>> Yeah, I don't see adding this option to all json generator functions as
>> making a lot of sense but rather just to the select few things which it
>> really makes sense for and then having a function which can be used by
>> users to do the same for results from other operations.

> I guess I'm questioning the wisdom of keeping it for row_to_json given 
> that it doesn't operate recursively anyway (and making it do so would be 
> difficult and ugly).

IMO, adding it to row_to_json was really ugly to start with, independently
of whether it's difficult or not.  That function had one too many optional
arguments already, and in its current form it's nothing but a loaded gun
pointed at users' feet.  (Quick, which bool argument is which?)

> If we're going to keep this, I think we also need to provide it 
> (non-recursive) for json_agg via an optional second argument. This 
> should be a fairly simple change: just steer the result via 
> composite_to_json if it's a record, rather than to datum_to_json.

Unfortunately, any such thing will fall foul of an established project
policy.  I quote the opr_sanity regression test that will complain:

-- Check that there are not aggregates with the same name and different
-- numbers of arguments.  While not technically wrong, we have a project policy
-- to avoid this because it opens the door for confusion in connection with
-- ORDER BY: novices frequently put the ORDER BY in the wrong place.
-- See the fate of the single-argument form of string_agg() for history.

So my vote is for a separate function and no optional arguments.
        regards, tom lane

On 09/27/2014 06:27 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 09/27/2014 08:00 AM, Stephen Frost wrote:
>>> Yeah, I don't see adding this option to all json generator functions as
>>> making a lot of sense but rather just to the select few things which it
>>> really makes sense for and then having a function which can be used by
>>> users to do the same for results from other operations.
>> I guess I'm questioning the wisdom of keeping it for row_to_json given
>> that it doesn't operate recursively anyway (and making it do so would be
>> difficult and ugly).
> IMO, adding it to row_to_json was really ugly to start with, independently
> of whether it's difficult or not.  That function had one too many optional
> arguments already, and in its current form it's nothing but a loaded gun
> pointed at users' feet.  (Quick, which bool argument is which?)
>
>> If we're going to keep this, I think we also need to provide it
>> (non-recursive) for json_agg via an optional second argument. This
>> should be a fairly simple change: just steer the result via
>> composite_to_json if it's a record, rather than to datum_to_json.
> Unfortunately, any such thing will fall foul of an established project
> policy.  I quote the opr_sanity regression test that will complain:
>
> -- Check that there are not aggregates with the same name and different
> -- numbers of arguments.  While not technically wrong, we have a project policy
> -- to avoid this because it opens the door for confusion in connection with
> -- ORDER BY: novices frequently put the ORDER BY in the wrong place.
> -- See the fate of the single-argument form of string_agg() for history.
>
> So my vote is for a separate function and no optional arguments.
>
>             



You mean like row_to_json_no_nulls() and json_agg_no_nulls()?

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 09/27/2014 06:27 PM, Tom Lane wrote:
>> So my vote is for a separate function and no optional arguments.

> You mean like row_to_json_no_nulls() and json_agg_no_nulls()?

I thought you were proposing that we should revert the committed patch
lock-stock-n-barrel, and instead invent json_strip_null_fields().
That's instead, not in addition to.  Even if you weren't saying that
exactly, that's where my vote goes.
        regards, tom lane

On 09/27/2014 10:52 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 09/27/2014 06:27 PM, Tom Lane wrote:
>>> So my vote is for a separate function and no optional arguments.
>> You mean like row_to_json_no_nulls() and json_agg_no_nulls()?
> I thought you were proposing that we should revert the committed patch
> lock-stock-n-barrel, and instead invent json_strip_null_fields().
> That's instead, not in addition to.  Even if you weren't saying that
> exactly, that's where my vote goes.


I was just exploring alternatives. But I think that's where my vote goes 
too.

cheers

andrew

On 09/27/2014 10:52 PM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 09/27/2014 06:27 PM, Tom Lane wrote:

So my vote is for a separate function and no optional arguments.

You mean like row_to_json_no_nulls() and json_agg_no_nulls()?

I thought you were proposing that we should revert the committed patch
lock-stock-n-barrel, and instead invent json_strip_null_fields().
That's instead, not in addition to.  Even if you weren't saying that
exactly, that's where my vote goes.

I was just exploring alternatives. But I think that's where my vote goes too.

On 09/27/2014 11:58 PM, Stephen Frost wrote:
> All,
>
> On Saturday, September 27, 2014, Andrew Dunstan <andrew@dunslane.net 
> <mailto:andrew@dunslane.net>> wrote:
>
>
>     On 09/27/2014 10:52 PM, Tom Lane wrote:
>
>         Andrew Dunstan <andrew@dunslane.net> writes:
>
>             On 09/27/2014 06:27 PM, Tom Lane wrote:
>
>                 So my vote is for a separate function and no optional
>                 arguments.
>
>             You mean like row_to_json_no_nulls() and json_agg_no_nulls()?
>
>         I thought you were proposing that we should revert the
>         committed patch
>         lock-stock-n-barrel, and instead invent json_strip_null_fields().
>         That's instead, not in addition to.  Even if you weren't
>         saying that
>         exactly, that's where my vote goes.
>
>
>
>     I was just exploring alternatives. But I think that's where my
>     vote goes too.
>
>
> I'm fine with that. I'd like the strip-Nulls capability, but seems 
> like it'd be better off as an independent function (or functions) 
> instead.
>
>

Unlike the row_to_json stuff, json{b}_strip_null_fields() can almost 
certainly be done as a small extension. One advantage of that is that it 
would be used with 9.4.

cheers

andrew

On Sat, Sep 27, 2014 at 11:00 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
> On 09/27/2014 10:52 PM, Tom Lane wrote:
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>> On 09/27/2014 06:27 PM, Tom Lane wrote:
>>>>
>>>> So my vote is for a separate function and no optional arguments.
>>>
>>> You mean like row_to_json_no_nulls() and json_agg_no_nulls()?
>>
>> I thought you were proposing that we should revert the committed patch
>> lock-stock-n-barrel, and instead invent json_strip_null_fields().
>> That's instead, not in addition to.  Even if you weren't saying that
>> exactly, that's where my vote goes.
>
> I was just exploring alternatives. But I think that's where my vote goes
> too.

+1.  I am sort of surprised that anyone things this null-stripping
behavior is something that ought to be part of core PostgreSQL instead
of, I don't know, relegated to an extension somewhere far from the
bright center of the galaxy.  I've never heard of that requirement
anywhere but here.  But if other people feel we should have it, that's
fine - but certainly making it a separate function makes a lot more
sense.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas@gmail.com) wrote:
> +1.  I am sort of surprised that anyone things this null-stripping
> behavior is something that ought to be part of core PostgreSQL instead
> of, I don't know, relegated to an extension somewhere far from the
> bright center of the galaxy.  I've never heard of that requirement
> anywhere but here.  But if other people feel we should have it, that's
> fine - but certainly making it a separate function makes a lot more
> sense.

I'm not at all surprised by the request, and I do believe it to be
worthwhile to have in core as it's a pretty common pattern with JSON.
That said, making it part of an operator function wasn't the right
approach and it should be an independent function.

Once I've caught up on things this morning, I'll revert the original
commit and ping Pavel about an alternative implementation, with an
independent function, which also handles complex JSON (unless anyone
else wants to volunteer to work on it..).
Thanks,
    Stephen

On 09/29/2014 10:38 AM, Robert Haas wrote:
> On Sat, Sep 27, 2014 at 11:00 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>> On 09/27/2014 10:52 PM, Tom Lane wrote:
>>> Andrew Dunstan <andrew@dunslane.net> writes:
>>>> On 09/27/2014 06:27 PM, Tom Lane wrote:
>>>>> So my vote is for a separate function and no optional arguments.
>>>> You mean like row_to_json_no_nulls() and json_agg_no_nulls()?
>>> I thought you were proposing that we should revert the committed patch
>>> lock-stock-n-barrel, and instead invent json_strip_null_fields().
>>> That's instead, not in addition to.  Even if you weren't saying that
>>> exactly, that's where my vote goes.
>> I was just exploring alternatives. But I think that's where my vote goes
>> too.
> +1.  I am sort of surprised that anyone things this null-stripping
> behavior is something that ought to be part of core PostgreSQL instead
> of, I don't know, relegated to an extension somewhere far from the
> bright center of the galaxy.  I've never heard of that requirement
> anywhere but here.  But if other people feel we should have it, that's
> fine - but certainly making it a separate function makes a lot more
> sense.
>


Fetching data from a missing field should return null, so stripping null 
fields can give you in effect the same result, depending on what your 
app does, without actually having to store the key and the null. And if 
we're producing JSON for an external processor from a table that is 
fairly sparsely populated, this could reduce the size of the JSON 
enormously.

That said, doing this as an extension is probably a good way to go, as I 
suggested upthread, since we could then make it available for 9.4, 
rather than making people wait until 9.5.

cheers

andrew

On Mon, Sep 29, 2014 at 9:45 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> +1.  I am sort of surprised that anyone things this null-stripping
>> behavior is something that ought to be part of core PostgreSQL instead
>> of, I don't know, relegated to an extension somewhere far from the
>> bright center of the galaxy.  I've never heard of that requirement
>> anywhere but here.  But if other people feel we should have it, that's
>> fine - but certainly making it a separate function makes a lot more
>> sense.
>
> I'm not at all surprised by the request, and I do believe it to be
> worthwhile to have in core as it's a pretty common pattern with JSON.
> That said, making it part of an operator function wasn't the right
> approach and it should be an independent function.

Are you defining 'core' as 'supported by the core project' (in which
case I agree) or 'not an extension' (in which case I disagree).

merlin

* Andrew Dunstan (andrew@dunslane.net) wrote:
> That said, doing this as an extension is probably a good way to go,
> as I suggested upthread, since we could then make it available for
> 9.4, rather than making people wait until 9.5.

Two points on this- having it in 9.5 doesn't preclude someone from
extracting it into an extension for 9.4 (indeed, that makes it far more
likely for such an extension to actually happen, imv..), and having it
in core means it's actually generally available and a function which can
be depended upon, which is far from the case for an extension.  Things
are a bit better if it's in contrib, though we'd want to have more than
one function provided in such a contrib extension.

Perhaps there are other functions related to JSON which should go into
such a contrib extension which would make it more worthwhile..  Would
hate to have an extension that ends up being "yeah, to actually use
JSON in PG you have to have this extension" either though.

I realize that can possibly be a "slippery slope", where we end up with
more in core than really belongs there, but this strikes me as a common
enough case that we should cover it.  If I didn't feel it would be a
frequently used capability, I wouldn't have supported adding it in the
first place..
Thanks,
    Stephen

Merlin,

* Merlin Moncure (mmoncure@gmail.com) wrote:
> Are you defining 'core' as 'supported by the core project' (in which
> case I agree) or 'not an extension' (in which case I disagree).

Which means you're suggesting it as an extension which lives in
contrib..?  Otherwise, I'm not following.

If that is the suggestion, then I'd want to have something more than
just this one function for that contrib extension...  Suggestions?
Thanks,
    Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Andrew Dunstan (andrew@dunslane.net) wrote:
>> That said, doing this as an extension is probably a good way to go,
>> as I suggested upthread, since we could then make it available for
>> 9.4, rather than making people wait until 9.5.

> Two points on this- having it in 9.5 doesn't preclude someone from
> extracting it into an extension for 9.4 (indeed, that makes it far more
> likely for such an extension to actually happen, imv..), and having it
> in core means it's actually generally available and a function which can
> be depended upon, which is far from the case for an extension.

I seem to recall that we've run into practical difficulties with moving
extensions into core.  It might be OK for a functions-only extension
though.
        regards, tom lane

On Sep 29, 2014, at 9:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I seem to recall that we've run into practical difficulties with moving
> extensions into core.  It might be OK for a functions-only extension
> though.

It does make upgrading difficult, though, as I’ve learned the hard way with when upgrading from 9.2 with
json_enhancementsto 9.3 without. We had to do some selective dropping and re-creating of functions, views, and triggers
toget it all to work properly. 

Best,

David

On 09/29/2014 03:23 PM, Pavel Stehule wrote:
>
>
> 2014-09-27 14:00 GMT+02:00 Stephen Frost <sfrost@snowman.net 
> <mailto:sfrost@snowman.net>>:
>
>     Andrew, all,
>
>     * Andrew Dunstan (andrew@dunslane.net
>     <mailto:andrew@dunslane.net>) wrote:
>     > I should have been paying a bit more attention to the recent work on
>     > adding an ignore_nulls option to row_to_json(). Here are some
>     > belated thought. I apologize to Pavel and Stephen for not having
>     > commented earlier.
>
>     No problem at all and thanks for continuing to think about it!  We
>     certainly still have quite a bit of time til 9.5 to get this right.
>
>     > I think this is really a bandaid, and it will fail to catch lots of
>     > cases. Several examples:
>
>     As discussed on IRC- I agree.  I tend to think of JSON objects as
>     relatively simple hstore-like structures and so hadn't considered the
>     complex structure case (as I'm guessing Pavel hadn't either).
>
>     > I think a much more comprehensive solution would be preferable. What
>     > I have in mind is something like
>     >
>     >     json_strip_null_fields(json) -> json
>     >
>     > and a similar function for jsonb.
>
>     Right, this makes sense to me.
>
>
> It is better than nothing, but it is not nice for JSON due 2x parsing. 
> Probably it is not issue for jsonb.
>
> It is not nice, but I have not better .. and it will be faster than 
> any custom solution in plpgsql.
>
> Personally, I am not sure, maybe is better to fix row_to_json
>
>

No. There are several reasons for not doing this, starting with the ones 
in my original email on the topic and Tom's objection to the use of 
multiple default options.

This is a non-starter.

JSON parsing is actually pretty darn fast. Every json (as opposed to 
jsonb) function reparses the json. It's true that this is not nearly as 
fast as processing jsonb, but I think for this purpose it's probably not 
too bad.

Frankly, row_to_json is not the most useful case where we could do this 
anyway, so I don't think we should be looking for a heroic effort there. 
I think the consensus for separate functions is the way to go.

I have made a start on coding strip_nulls functions. I'll post a patch 
before too long.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 09/29/2014 03:23 PM, Pavel Stehule wrote:
>> It is better than nothing, but it is not nice for JSON due 2x parsing. 

> JSON parsing is actually pretty darn fast. Every json (as opposed to 
> jsonb) function reparses the json. It's true that this is not nearly as 
> fast as processing jsonb, but I think for this purpose it's probably not 
> too bad.

More to the point, the way to fix any concerns about double parsing is to
create row_to_jsonb(), not to plaster a bunch of options on row_to_json().

One of the reasons I didn't like the extra option for row_to_json is that
having any such options would inevitably create confusion when we do
invent row_to_jsonb, because that will certainly not have a "pretty"
option.  So row_to_json('...', true) and row_to_jsonb('...', true) would
both be accepted but they'd interpret their second arguments entirely
differently.  This is not really the fault of the "ignore_null_fields"
flag, but of the "pretty" flag, which arguably should not have been there
in the first place.  But looking at that precedent doesn't exactly fill
one with confidence that adding more optional arguments to row_to_json
isn't going to create even more problems down the line.

Breaking it out as a separate function seems to me to be a much better
long-term design.
        regards, tom lane

On 09/29/2014 04:14 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 09/29/2014 03:23 PM, Pavel Stehule wrote:
>>> It is better than nothing, but it is not nice for JSON due 2x parsing.
>> JSON parsing is actually pretty darn fast. Every json (as opposed to
>> jsonb) function reparses the json. It's true that this is not nearly as
>> fast as processing jsonb, but I think for this purpose it's probably not
>> too bad.
> More to the point, the way to fix any concerns about double parsing is to
> create row_to_jsonb(), not to plaster a bunch of options on row_to_json().


row_to_jsonb would be completely redundant with to_jsonb() in my recent 
patch.

And I don't want to add options like this there for the same reasons I 
didn't want them in row_to_json().


cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 09/29/2014 04:14 PM, Tom Lane wrote:
>> More to the point, the way to fix any concerns about double parsing is to
>> create row_to_jsonb(), not to plaster a bunch of options on row_to_json().

> row_to_jsonb would be completely redundant with to_jsonb() in my recent 
> patch.

Right, which raises the question of whether we shouldn't just be
deprecating both array_to_json() and row_to_json()...

> And I don't want to add options like this there for the same reasons I 
> didn't want them in row_to_json().

Agreed.  IMO the place to have put the "pretty" functionality was in some
sort of json-to-text conversion function; it never belonged in an input
conversion function.
        regards, tom lane

On 09/29/2014 04:32 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 09/29/2014 04:14 PM, Tom Lane wrote:
>>> More to the point, the way to fix any concerns about double parsing is to
>>> create row_to_jsonb(), not to plaster a bunch of options on row_to_json().
>> row_to_jsonb would be completely redundant with to_jsonb() in my recent
>> patch.
> Right, which raises the question of whether we shouldn't just be
> deprecating both array_to_json() and row_to_json()...
>
>> And I don't want to add options like this there for the same reasons I
>> didn't want them in row_to_json().
> Agreed.  IMO the place to have put the "pretty" functionality was in some
> sort of json-to-text conversion function; it never belonged in an input
> conversion function.
>
>             


Yes, if we had pretty printing functions we could deprecate and 
eventually remove row_to_json and array_to_json. That's probably worth 
putting on the TODO list.

cheers

andrew