Horton

Thread

Commits

  1. Remove "invalid concatenation of jsonb objects" error case.

  1. Weird special case in jsonb_concat()

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-12-18T17:20:33Z

    The discussion in [1] pointed out that the existing documentation
for the "jsonb || jsonb" concatenation operator is far short of
reality: it fails to acknowledge that the operator will accept
any cases other than two jsonb array inputs or two jsonb object
inputs.

I'd about concluded that other cases were handled as if by
wrapping non-array inputs in one-element arrays and then
proceeding as for two arrays.  That works for most scenarios, eg

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

regression=# select '3'::jsonb || '[]'::jsonb;
 ?column? 
----------
 [3]
(1 row)

regression=# select '3'::jsonb || '4'::jsonb;
 ?column? 
----------
 [3, 4]
(1 row)

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?

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/0d72b76d-ca2b-4263-8888-d6dfca861c51%40www.fastmail.com

  2. Re: Weird special case in jsonb_concat()

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-12-19T20:35:28Z

    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

  3. Re: Weird special case in jsonb_concat()

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-12-20T06:32:48Z

    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


>                         regards, tom lane
>
>

  4. Re: Weird special case in jsonb_concat()

    Joel Jacobson <joel@compiler.org> — 2020-12-20T07:33:38Z

    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

  5. Re: Weird special case in jsonb_concat()

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-12-21T18:15:37Z

    "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