Re: Bug in jsonb minus operator

On 05/16/2015 08:04 PM, Peter Geoghegan wrote:
> I'm seeing the following problem on the master branch:
>
> postgres=# select '{"foo":5}'::jsonb - 'bar'; -- okay
>    ?column?
> ------------
>   {"foo": 5}
> (1 row)
>
> postgres=# select '{"foo":{"bar":5}}'::jsonb - 'foo'; -- okay
>   ?column?
> ----------
>   {}
> (1 row)
>
> postgres=# select '{"foo":{"bar":5}}'::jsonb - 'rion'; -- spurious error
> ERROR:  XX000: unknown type of jsonb container to convert
> LOCATION:  convertJsonbValue, jsonb_util.c:1430
>
> This is an elog() - a "can't happen" error - due to this restriction
> within convertJsonbValue():
>
> /*
>   * A JsonbValue passed as val should never have a type of jbvBinary, and
>   * neither should any of its sub-components. Those values will be produced
>   * by convertJsonbArray and convertJsonbObject, the results of which will
>   * not be passed back to this function as an argument.
>   */
>
> This call to convertJsonbValue() actually originates from the final
> line of the new jsonb_delete() function, here:
>
> #5  0x0000000000877e10 in jsonb_delete (fcinfo=0x160e060) at jsonfuncs.c:3389
> 3389 PG_RETURN_JSONB(JsonbValueToJsonb(res));
>
> I explored writing a fix for this bug. I went back and forth on it,
> but I think that the most promising approach might be to simply teach
> convertJsonbValue() to care about jbvBinary-typed JsonbValue
> variables. That way, the jsonb_delete() function could actually expect
> this to work. I can't remember why I thought it was a good idea to
> have convertJsonbValue() reject jbvBinary values, but I believe the
> reason was that it simply wasn't necessary.


Sure. I thought we'd covered this but it's possible that we didn't, or 
that it got rebroken. There have been complaints about the limitation on 
values containing jbvBinary, so let's just remove it if that can be done 
simply, as it seems to be a not uncommonly encountered problem.

Are you going to submit a patch for that?


> jsonb_delete() is unusual in that it converts from a JsonbValue back
> to the on-disk Jsonb varlena format, but with a nested jbvBinary-typed
> value (in the presence of a nested object or array) -- it seems like
> it wouldn't be too hard to just roll with that. jsonb_delete() makes
> the mistake of not expecting to see jbvBinary values (since it doesn't
> always recurse into the json structure). We shouldn't really deal with
> jbvBinary-typed values specially from outside specialized utility
> routines like JsonbDeepContains() as noted in the above comment,
> though (so offhand I don't think we should teach jsonb_delete()
> anything new, as that would be a modularity violation). Thoughts?


Assuming the above fix we won't have to teach it anything new, right?

>
> Note that the existence related operators (that, like the minus
> operator should only work at the top level) don't go through
> JsonbValue conversion of the lhs value as part of their search at all.
> I don't think that their findJsonbValueFromContainer() routine (or a
> similar routine) is the right way of approaching this, though - that's
> a specialized routine, that doesn't care if an array value (which is,
> you'll recall, a key for the purposes of existence checking) appears
> once or multiple times.

Seems reasonable.

> On that topic, I think it's sloppy that "Table
> 9-41. Additional jsonb Operators" isn't clear about the fact that the
> "operator - text" op matches things on the same basis as the existence
> operators -- notice how the existence operator notes with emphasis
> that it cares about array *string* values only.

OK, please submit a patch that you think clears it up.

>
> Finally, I don't think an operator implementation function that is
> jsonb-only belongs anywhere other than jsonb_ops.c (for the same
> reason, replacePath() should live in jsonb_util.c).


The heading on jsonb_op.c (note spelling) suggests that it's for indexed 
operations. I don't mind rearranging the code layout to something people 
think more logical, but I also don't want to engage in unnecessary code 
churn. I'm not that religious about the organization.


> And I'm
> disappointed that the jsonb tests can no longer be run atomically with
> 'make installcheck TESTS="jsonb"' - I liked being able to do that.

It was always known that some cases would not work with TESTS - that was 
part of Tom's reservation about the whole thing. You can say
   make check-tests TESTS="create_table copy jsonb"


and while create_table will fail on an irrelevant test, copy and jsonb 
will succeed.

cheers

andrew