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(at)dunslane(dot)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