On 05/19/2015 07:11 PM, Andrew Dunstan wrote:
>
> 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.
>
>
OK, here's a slightly updated version of the patch. The current
behaviour in which pushJsonbValue() happily pushes a jbvBinary value,
only to have it fail later when we try to get a Jsonb out of it, seems
like a plain bug. The fact that none of our present 9.4 code exercises
that bug is lucky, but since this is an exposed API, and has been the
cause of past complaint, I propose, in the absence of further objections
or comments, to apply it to both master and 9.4, once the new releases
are out of the way.
cheers
andrew
