Re: Jsonb: jbvBinary usage in the convertJsonbValue?

On 06/02/2014 10:22 AM, Andrew Dunstan wrote:
>
> On 06/02/2014 10:02 AM, Robert Haas wrote:
>> On Thu, May 29, 2014 at 7:34 AM, Dmitry Dolgov 
>> <9erthalion6@gmail.com> wrote:
>>> I'm little confused by the convertJsonbValue functon at jsonb_utils.c
>>> Maybe I misunderstood something, so I need help =)
>>>
>>>>>> if (IsAJsonbScalar(val) || val->type == jbvBinary)
>>>>>>      convertJsonbScalar(buffer, header, val);
>>> As I can see, the convertJsonbScalar function is used for scalar and 
>>> binary
>>> jsonb values. But this function doesn't handle the jbvBinary type.
>> There definitely seems to be an oversight here of some kind. Either
>> convertJsonbValue() shouldn't be calling convertJsonbScalar() with an
>> object of type jbvBinary, or convertJsonbScalar() should know how to
>> handle that case.
>>
>
>
> Yes, I've just been looking at that. I think this is probably a 
> hangover from when these routines were recast to some extent. Given 
> that we're not seeing any errors from it, I'd be inclined to remove 
> the the "|| val->type == jbvBinary" part. One of the three call sites 
> to convertJsonbValue asserts that this can't be true, and doing so 
> doesn't result in a regression failure.
>
> Peter and Teodor, comments?
>
>

Thinking about it a bit more, ISTM this should be ok, since we convert a 
JsonbValue to Jsonb in a depth-first recursive pattern. We should 
certainly add some comments along these lines to explain why the 
jbvbinary case shouldn't arise.

cheers

andrew