Re: Jsonb: jbvBinary usage in the convertJsonbValue?

On 06/02/2014 11:38 AM, Andrew Dunstan wrote:
>
> 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.
>

Nobody has commented further that I have noticed, so I have committed this.

cheers

andrew