Re: Bug in jsonb minus operator

On 05/18/2015 10:52 PM, Peter Geoghegan wrote:
> On Mon, May 18, 2015 at 7:11 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
>> Here's an patch along those lines. It seems to do the trick, at least for
>> your test case, and it has the merit of being very small, so small I'd like
>> to backpatch it - accepting jbvBinary as is in pushJsonbValue seems like a
>> bug to me.
> Isn't that for the benefit of raw scalar pseudo arrays? The existing
> comments above pushJsonbValue() acknowledge such callers.


Umm, no, the raw scalar pseudo arrays are of type jbvArray, not 
jbvBinary. And they are pushed with WJB_BEGIN_ARRAY, not with WJB_ELEM 
or WJB_VALUE, as the comment notes. See this fragment of code from 
JsonbValueToJsonb:
        scalarArray.type = jbvArray;        scalarArray.val.array.rawScalar = true;        scalarArray.val.array.nElems
=1;
 
        pushJsonbValue(&pstate, WJB_BEGIN_ARRAY, &scalarArray);


I tested this by removing the assert test for jbvBinary in the WJB_ELEM 
and WJB_VALUE switch beranches and then running the regression suite. No 
assertion failure was triggered. While that's not guaranteed to be a 
perfect test, it doesn't seem like a bad one. Can you pose a counter 
example where this will break?


>
> Why are you passing the skipNested variable to JsonbIteratorNext()
> within jsonb_delete()? I'm not seeing a need for that.
>

If you don't the logic gets more complex, as you need to keep track of 
what level of the object you are at. The virtue of this change is that 
it will simplify a lot of such processing by removing the unnecessary 
restriction on passing jbvBinary values to pushJsonbValue().

If you have a better fix for the bug you complained about I'll be happy 
to take a look.

cheers

andrew