Re: [HACKERS] 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 9erthali...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Jsonb: jbvBinary usage in the convertJsonbValue?
On Thu, May 29, 2014 at 7:34 AM, Dmitry Dolgov 9erthali...@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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Jsonb: jbvBinary usage in the convertJsonbValue?
On 06/02/2014 10:02 AM, Robert Haas wrote: On Thu, May 29, 2014 at 7:34 AM, Dmitry Dolgov 9erthali...@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? cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 9erthali...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Jsonb: jbvBinary usage in the convertJsonbValue?
On Mon, Jun 2, 2014 at 7:22 AM, Andrew Dunstan and...@dunslane.net wrote: 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. Well, I guess the latter condition should be moved, since clearly the jbvBinary case isn't handled within convertJsonbScalar(). It would be mildly preferable to have a more accurate error message (the convertJsonbValue() error) if that can't happen condition should ever happen. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
