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.
Thosevalues will be produced* by convertJsonbArray and convertJsonbObject, the results of which will* not be passed
backto 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
generallywe find it more* convenient to directly iterate through the Jsonb representation and only* really convert
nestedscalar values.  JsonbIteratorNext() does this, so that* clients of the iteration code don't have to directly deal
withthe binary* representation (JsonbDeepContains() is a notable exception, although all* exceptions are internal to
thismodule).  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