Re: jsonb contains behaviour weirdness

Hi!

Let's consider some examples.

# select '[1,2]'::jsonb @> '[1,2,2]'::jsonb;
?column?
----------
f
(1 row)

One may think it's because second jsonb array contain two "2". So, contains
takes care about count of equal elements.

# select '[1,1,2]'::jsonb @> '[1,2,2]'::jsonb;
?column?
----------
t
(1 row)

But, it's not. Jsonb contains takes care only about length of array.

# select '[[1,2]]'::jsonb @> '[[1,2,2]]'::jsonb;
?column?
----------
t
(1 row)

Even more weird :)
The reason why jsonb contains behaves so is check in the beginning
of jsonb_contains. It makes fast check of jsonb type and elements count
before calling JsonbDeepContains.

if (JB_ROOT_COUNT(val) < JB_ROOT_COUNT(tmpl) ||
JB_ROOT_IS_OBJECT(val) != JB_ROOT_IS_OBJECT(tmpl))
PG_RETURN_BOOL(false);

It's likely that "JB_ROOT_COUNT(val) < JB_ROOT_COUNT(tmpl)" should be
checked only for objects, not arrays. Also, should JsonbDeepContains does
same fast check when it deals with nested objects?

------
With best regards,
Alexander Korotkov.

On Fri, Sep 12, 2014 at 6:40 AM, Alexander Korotkov
<aekorotkov(at)gmail(dot)com> wrote:
> Even more weird :)

Agreed.

> The reason why jsonb contains behaves so is check in the beginning of
> jsonb_contains. It makes fast check of jsonb type and elements count before
> calling JsonbDeepContains.
>
> if (JB_ROOT_COUNT(val) < JB_ROOT_COUNT(tmpl) ||
> JB_ROOT_IS_OBJECT(val) != JB_ROOT_IS_OBJECT(tmpl))
> PG_RETURN_BOOL(false);
>
> It's likely that "JB_ROOT_COUNT(val) < JB_ROOT_COUNT(tmpl)" should be
> checked only for objects, not arrays. Also, should JsonbDeepContains does
> same fast check when it deals with nested objects?

I think this is due to commit 364ddc. That removed the extra step that
had arrays sorted (and then de-duped) ahead of time, which made arrays
behave like objects at the top level. I think that this sort + de-dup
step was mischaracterized as purely a performance thing (possibly by
me). Basically, JsonbDeepContains() is consistent with the previous
behavior at the top level, but not the current (inadvertently altered)
behavior. I think the fix is probably a return to the previous
behavior. I'll take a closer look.

--
Peter Geoghegan

Peter Geoghegan <pg(at)heroku(dot)com> writes:
> I think this is due to commit 364ddc. That removed the extra step that
> had arrays sorted (and then de-duped) ahead of time, which made arrays
> behave like objects at the top level. I think that this sort + de-dup
> step was mischaracterized as purely a performance thing (possibly by
> me). Basically, JsonbDeepContains() is consistent with the previous
> behavior at the top level, but not the current (inadvertently altered)
> behavior. I think the fix is probably a return to the previous
> behavior. I'll take a closer look.

I'm confused. Are you proposing to return to sort + de-dup of JSON
arrays? Surely that is completely broken. Arrays are ordered.

regards, tom lane

On Fri, Sep 12, 2014 at 11:09 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I'm confused. Are you proposing to return to sort + de-dup of JSON
> arrays? Surely that is completely broken. Arrays are ordered.

Sorry, my earlier remarks were premature. In fact, that alteration
only applied to existence, not containment. However, arrays are
ordered for the purposes of equality, but not containment.

--
Peter Geoghegan

On 09/12/2014 06:40 AM, Alexander Korotkov wrote:
> Hi!
>
> Let's consider some examples.
>
> # select '[1,2]'::jsonb @> '[1,2,2]'::jsonb;
> ?column?
> ----------
> f
> (1 row)
>
> One may think it's because second jsonb array contain two "2". So,
> contains takes care about count of equal elements.

JSONB arrays are allowed to have repleating elements. It's keys which
are not allowed to repeat.

>
> # select '[1,1,2]'::jsonb @> '[1,2,2]'::jsonb;
> ?column?
> ----------
> t
> (1 row)
>
> But, it's not. Jsonb contains takes care only about length of array.

OK, now, that's messed up.

>
> # select '[[1,2]]'::jsonb @> '[[1,2,2]]'::jsonb;
> ?column?
> ----------
> t
> (1 row)
>
> Even more weird :)
> The reason why jsonb contains behaves so is check in the beginning
> of jsonb_contains. It makes fast check of jsonb type and elements count
> before calling JsonbDeepContains.
>
> if (JB_ROOT_COUNT(val) < JB_ROOT_COUNT(tmpl) ||
> JB_ROOT_IS_OBJECT(val) != JB_ROOT_IS_OBJECT(tmpl))
> PG_RETURN_BOOL(false);
>
> It's likely that "JB_ROOT_COUNT(val) < JB_ROOT_COUNT(tmpl)" should be
> checked only for objects, not arrays.

Yeah, agreed.

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

Peter Geoghegan <pg(at)heroku(dot)com> writes:
> On Fri, Sep 12, 2014 at 11:09 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm confused. Are you proposing to return to sort + de-dup of JSON
>> arrays? Surely that is completely broken. Arrays are ordered.

> Sorry, my earlier remarks were premature. In fact, that alteration
> only applied to existence, not containment. However, arrays are
> ordered for the purposes of equality, but not containment.

My remarks were also premature, because looking back at the referenced
commit, I see what it removed was a sort and de-dup as a preliminary step
in containment comparisons, not as a generic alteration of array contents.
So that was sane enough, though I concur with Heikki's opinion that it
likely failed to be a performance win.

regards, tom lane

On Fri, Sep 12, 2014 at 11:21 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> # select '[1,1,2]'::jsonb @> '[1,2,2]'::jsonb;
>> ?column?
>> ----------
>> t
>> (1 row)
>>
>> But, it's not. Jsonb contains takes care only about length of array.
>
> OK, now, that's messed up.

To be clear: I don't think that this example is messed up (in
isolation). I think it's the correct behavior. What I find
objectionable is the inconsistency. I believe that this is Alexander's
concern too. Alexander's first example exhibits broken behavior.

--
Peter Geoghegan

On 09/12/2014 11:38 AM, Peter Geoghegan wrote:
> To be clear: I don't think that this example is messed up (in
> isolation). I think it's the correct behavior. What I find
> objectionable is the inconsistency. I believe that this is Alexander's
> concern too. Alexander's first example exhibits broken behavior.

Hmmm, oh. Yeah, I see what you mean; PostgreSQL's SQL array behavior is
that @> is true if array A contains all of the elements of array B
regardless of ordering or repetition.

jsonic=# select array[1,2,2] @> array[1,1,2]

;
?column?
----------
t

That's consistent with our docs and past behavior.

However, this better become a FAQ item, because it's not necessarily the
behavior that folks used to JSON but not Postgres will expect.

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

On Fri, Sep 12, 2014 at 10:38 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:

> On Fri, Sep 12, 2014 at 11:21 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> >> # select '[1,1,2]'::jsonb @> '[1,2,2]'::jsonb;
> >> ?column?
> >> ----------
> >> t
> >> (1 row)
> >>
> >> But, it's not. Jsonb contains takes care only about length of array.
> >
> > OK, now, that's messed up.
>
> To be clear: I don't think that this example is messed up (in
> isolation). I think it's the correct behavior. What I find
> objectionable is the inconsistency. I believe that this is Alexander's
> concern too. Alexander's first example exhibits broken behavior.
>

Agree. I just tried to explain how current behaviour could look for user
who sees it for the first time.

------
With best regards,
Alexander Korotkov.

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> However, this better become a FAQ item, because it's not necessarily the
> behavior that folks used to JSON but not Postgres will expect.

No, it's a bug, not a documentation deficiency.

regards, tom lane

On Fri, Sep 12, 2014 at 6:40 AM, Alexander Korotkov
<aekorotkov(at)gmail(dot)com> wrote:
> It's likely that "JB_ROOT_COUNT(val) < JB_ROOT_COUNT(tmpl)" should be
> checked only for objects, not arrays. Also, should JsonbDeepContains does
> same fast check when it deals with nested objects?

Attached patch implements something similar to what you describe here,
fixing your example.

I haven't added the optimization to JsonbDeepContains(). I think that
if anything, we should remove the optimization entirely, which is what
I've done -- an rhs "is it contained within?" value is hardly ever
going to be an object that has more pairs than the object we're
checking it is contained within. It's almost certainly going to have
far fewer pairs. Apart from only applying to objects, that
optimization just isn't an effective way of eliminating jsonb values
from consideration quickly. I'd rather not bother at all, rather than
having a complicated comment about why the optimization applies to
objects and not arrays.

--
Peter Geoghegan

On 09/12/2014 01:33 PM, Tom Lane wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> However, this better become a FAQ item, because it's not necessarily the
>> behavior that folks used to JSON but not Postgres will expect.
>
> No, it's a bug, not a documentation deficiency.

Hmmm? Are you proposing that we should change how ARRAY @> ARRAY works
for non-JSON data?

Or are you proposing that JSONARRAY @> JSONARRAY should work differently
from ARRAY @> ARRAY?

Or are you agreeing with Peter that it's the first case that's a bug,
and the 2nd two tests are correct, and just being unclear?

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

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> On 09/12/2014 01:33 PM, Tom Lane wrote:
>> No, it's a bug, not a documentation deficiency.

> Hmmm? Are you proposing that we should change how ARRAY @> ARRAY works
> for non-JSON data?

No.

> Or are you proposing that JSONARRAY @> JSONARRAY should work differently
> from ARRAY @> ARRAY?

And no. It's a bug that jsonb array containment works differently from
regular array containment. We understand the source of the bug, ie a
mistaken optimization. I don't see why there's much need for discussion
about anything except whether removing the optimization altogether
(as Peter proposed) is the best fix, or whether we want to retain
some weaker form of it.

Personally I'd think that we should retain it for objects; Peter's
main argument against that was that the comment would be too complicated,
but that seems a bit silly from here.

regards, tom lane

On Mon, Sep 15, 2014 at 11:12 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Personally I'd think that we should retain it for objects; Peter's
> main argument against that was that the comment would be too complicated,
> but that seems a bit silly from here.

I just don't see any point to it. My argument against the complexity
of explaining why the optimization is only used with objects is based
on the costs and the benefits. I think the benefits are very close to
nil.

--
Peter Geoghegan

On Mon, Sep 15, 2014 at 2:18 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Mon, Sep 15, 2014 at 11:12 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Personally I'd think that we should retain it for objects; Peter's
>> main argument against that was that the comment would be too complicated,
>> but that seems a bit silly from here.
>
> I just don't see any point to it. My argument against the complexity
> of explaining why the optimization is only used with objects is based
> on the costs and the benefits. I think the benefits are very close to
> nil.

That seems pessimistic to me; I'm with Tom.

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

Peter Geoghegan <pg(at)heroku(dot)com> writes:
> On Mon, Sep 15, 2014 at 11:12 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Personally I'd think that we should retain it for objects; Peter's
>> main argument against that was that the comment would be too complicated,
>> but that seems a bit silly from here.

> I just don't see any point to it. My argument against the complexity
> of explaining why the optimization is only used with objects is based
> on the costs and the benefits. I think the benefits are very close to
> nil.

It might be that the benefit is very close to nil; that would depend a lot
on workload, so it's hard to be sure. I'd say though that the cost is
also very close to nil, in the sense that we're considering two additional
compare-and-branch instructions in a function that will surely expend
hundreds or thousands of instructions if there's no such short-circuit.

I've certainly been on the side of "that optimization isn't worth its
keep" many times before, but I don't think the case is terribly clear cut
here. Since somebody (possibly you) thought it was worth having to begin
with, I'm inclined to follow that lead.

regards, tom lane

On Mon, Sep 15, 2014 at 11:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> It might be that the benefit is very close to nil; that would depend a lot
> on workload, so it's hard to be sure. I'd say though that the cost is
> also very close to nil, in the sense that we're considering two additional
> compare-and-branch instructions in a function that will surely expend
> hundreds or thousands of instructions if there's no such short-circuit.

Fair enough.

--
Peter Geoghegan

On 09/15/2014 11:12 AM, Tom Lane wrote:
>> Or are you proposing that JSONARRAY @> JSONARRAY should work differently
>> from ARRAY @> ARRAY?
>
> And no. It's a bug that jsonb array containment works differently from
> regular array containment. We understand the source of the bug, ie a
> mistaken optimization. I don't see why there's much need for discussion
> about anything except whether removing the optimization altogether
> (as Peter proposed) is the best fix, or whether we want to retain
> some weaker form of it.

Right, so I was just saying that after we fix this behavior, the
behavior of JSONARRAY @> JSONARRAY should be commented somewhere because
that comparison may not work the way users who are not long-time
postgres users expect. Heck, I've personally done very little ARRAY @>
ARRAY myself in 12 years of using PostgreSQL arrays; I had to test it to
verify the current behavior.

Not sure exactly where this note should go, mind you.

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

This was never committed...

On Sat, Sep 13, 2014 at 4:31 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Fri, Sep 12, 2014 at 6:40 AM, Alexander Korotkov
> <aekorotkov(at)gmail(dot)com> wrote:
>> It's likely that "JB_ROOT_COUNT(val) < JB_ROOT_COUNT(tmpl)" should be
>> checked only for objects, not arrays. Also, should JsonbDeepContains does
>> same fast check when it deals with nested objects?
>
> Attached patch implements something similar to what you describe here,
> fixing your example.
>
> I haven't added the optimization to JsonbDeepContains(). I think that
> if anything, we should remove the optimization entirely, which is what
> I've done -- an rhs "is it contained within?" value is hardly ever
> going to be an object that has more pairs than the object we're
> checking it is contained within. It's almost certainly going to have
> far fewer pairs. Apart from only applying to objects, that
> optimization just isn't an effective way of eliminating jsonb values
> from consideration quickly. I'd rather not bother at all, rather than
> having a complicated comment about why the optimization applies to
> objects and not arrays.

--
Peter Geoghegan

Peter Geoghegan <pg(at)heroku(dot)com> writes:
> This was never committed...

Yeah ... the discussion trailed off and I think we forgot to actually
commit a fix. Will go do so.

regards, tom lane