Bug in jsonb minus operator

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. As it says of conversion
from in memory to disk representation (at the top of json_util.c):

/*
* Turn an in-memory JsonbValue into a Jsonb for on-disk storage.
*
* There isn't a JsonbToJsonbValue(), because generally we find it more
* convenient to directly iterate through the Jsonb representation and only
* really convert nested scalar values. JsonbIteratorNext() does this, so that
* clients of the iteration code don't have to directly deal with the binary
* representation (JsonbDeepContains() is a notable exception, although all
* exceptions are internal to this module). In general, functions that accept
* a JsonbValue argument are concerned with the manipulation of scalar values,
* or simple containers of scalar values, where it would be inconvenient to
* deal with a great amount of other state.
*/

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?

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. 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.

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). 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.
--
Peter Geoghegan