Weird special case in jsonb_concat()

I wrote:

However, further experimentation found a case that fails:
regression=# select '3'::jsonb || '{}'::jsonb;
ERROR: invalid concatenation of jsonb objects
I wonder what is the point of this weird exception, and whether
whoever devised it can provide a concise explanation of what
they think the full behavior of "jsonb || jsonb" is. Why isn't
'[3, {}]' a reasonable result here, if the cases above are OK?

Here is a proposed patch for that. It turns out that the third
else-branch in IteratorConcat() already does the right thing, if
we just remove its restrictive else-condition and let it handle
everything except the two-objects and two-arrays cases. But it
seemed to me that trying to handle both the object || array
and array || object cases in that one else-branch was poorly
thought out: only one line of code can actually be shared, and it
took several extra lines of infrastructure to support the sharing.
So I split those cases into separate else-branches.

This also addresses the inadequate documentation that was the
original complaint.

Thoughts? Should we back-patch this? The existing behavior
seems to me to be inconsistent enough to be arguably a bug,
but we've not had field complaints saying "this should work".

regards, tom lane

so 19. 12. 2020 v 21:35 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

I wrote:

However, further experimentation found a case that fails:
regression=# select '3'::jsonb || '{}'::jsonb;
ERROR: invalid concatenation of jsonb objects
I wonder what is the point of this weird exception, and whether
whoever devised it can provide a concise explanation of what
they think the full behavior of "jsonb || jsonb" is. Why isn't
'[3, {}]' a reasonable result here, if the cases above are OK?

Here is a proposed patch for that. It turns out that the third
else-branch in IteratorConcat() already does the right thing, if
we just remove its restrictive else-condition and let it handle
everything except the two-objects and two-arrays cases. But it
seemed to me that trying to handle both the object || array
and array || object cases in that one else-branch was poorly
thought out: only one line of code can actually be shared, and it
took several extra lines of infrastructure to support the sharing.
So I split those cases into separate else-branches.

This also addresses the inadequate documentation that was the
original complaint.

Thoughts? Should we back-patch this? The existing behavior
seems to me to be inconsistent enough to be arguably a bug,
but we've not had field complaints saying "this should work".

+1

Pavel

On Sat, Dec 19, 2020, at 21:35, Tom Lane wrote:

Here is a proposed patch for that.

I've tested the patch and "All 202 tests passed".

In addition, I've tested it on a json intensive project,
which passes all its own tests.

I haven't studied the jsonfuncs.c code in detail,
but the new code looks much cleaner, nice.

This also addresses the inadequate documentation that was the
original complaint.

Looks good.

In addition, to the user wondering how to append a json array-value "as is",
I think it would be useful to provide an example on how to do this
in the documentation.

I think there is a risk users will attempt much more fragile
hacks to achieve this, if we don't provide guidance
in the documentation.

Suggestion:

         <literal>'["a", "b"]'::jsonb || '["a", "d"]'::jsonb</literal>
         <returnvalue>["a", "b", "a", "d"]</returnvalue>
        </para>
+       <para>
+        <literal>'["a", "b"]'::jsonb || jsonb_build_array('["a", "d"]'::jsonb)</literal>
+        <returnvalue>["a", "b", ["a", "d"]]</returnvalue>
+       </para>
        <para>
         <literal>'{"a": "b"}'::jsonb || '{"c": "d"}'::jsonb</literal>
         <returnvalue>{"a": "b", "c": "d"}</returnvalue>

Thoughts? Should we back-patch this? The existing behavior
seems to me to be inconsistent enough to be arguably a bug,
but we've not had field complaints saying "this should work".

+1 back-patch, I think it's a bug.

Best regards,

Joel

Hi,
w.r.t. the patch,

+select '[3]'::jsonb || '{}'::jsonb;
+ ?column?
+----------
+ [3, {}]
+(1 row)
+
+select '3'::jsonb || '[]'::jsonb;

Should cases where the empty array precedes non-empty jsonb be added ?

select '[]'::jsonb || '3'::jsonb;
select '{}'::jsonb || '[3]'::jsonb;

Cheers

"Joel Jacobson" <joel@compiler.org> writes:

On Sat, Dec 19, 2020, at 21:35, Tom Lane wrote:

Here is a proposed patch for that.

In addition, to the user wondering how to append a json array-value "as is",
I think it would be useful to provide an example on how to do this
in the documentation.

Done in v13 and HEAD; the older table format doesn't really have room
for more examples.

+1 back-patch, I think it's a bug.

I'm not quite sure it's a bug, but it does seem like fairly unhelpful
behavior to throw an error instead of doing something useful, so
back-patched.

regards, tom lane