Обсуждение: Question about duplicate JSONTYPE_JSON check

While exploring the jsonb code, I noticed that in
datum_to_jsonb_internal, the tcategory checks compares against
JSONTYPE_JSON twice. There's no reason for that, right?

...

Ok, so, to try to answer my own question, I went looking at the
history, and this comes from "Unify JSON categorize type API and
export for external use" [0]. Specifically, the change was

-            (tcategory == JSONBTYPE_ARRAY ||
-             tcategory == JSONBTYPE_COMPOSITE ||
-             tcategory == JSONBTYPE_JSON ||
-             tcategory == JSONBTYPE_JSONB ||
-             tcategory == JSONBTYPE_JSONCAST))
+            (tcategory == JSONTYPE_ARRAY ||
+             tcategory == JSONTYPE_COMPOSITE ||
+             tcategory == JSONTYPE_JSON ||
+             tcategory == JSONTYPE_JSONB ||
+             tcategory == JSONTYPE_JSON))

So "JSONBTYPE_JSONCAST" turned into "JSONTYPE_JSON". Should that have
been "JSONTYPE_CAST" (that seems to be the corresponding value in the
new enum) instead?

Thanks,
Maciek

[0]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3c152a27b06313fe27bd47079658f928e291986b

I'm adding the author/committer and reviewer of 3c152a2, since I think
this may be a bug (my apologies if I'm misunderstanding this). See my
previous e-mail quoted below:

On Mon, Mar 10, 2025 at 5:11 PM Maciek Sakrejda <maciek@pganalyze.com> wrote:
>
> While exploring the jsonb code, I noticed that in
> datum_to_jsonb_internal, the tcategory checks compares against
> JSONTYPE_JSON twice. There's no reason for that, right?
>
> ...
>
> Ok, so, to try to answer my own question, I went looking at the
> history, and this comes from "Unify JSON categorize type API and
> export for external use" [0]. Specifically, the change was
>
> -            (tcategory == JSONBTYPE_ARRAY ||
> -             tcategory == JSONBTYPE_COMPOSITE ||
> -             tcategory == JSONBTYPE_JSON ||
> -             tcategory == JSONBTYPE_JSONB ||
> -             tcategory == JSONBTYPE_JSONCAST))
> +            (tcategory == JSONTYPE_ARRAY ||
> +             tcategory == JSONTYPE_COMPOSITE ||
> +             tcategory == JSONTYPE_JSON ||
> +             tcategory == JSONTYPE_JSONB ||
> +             tcategory == JSONTYPE_JSON))
>
> So "JSONBTYPE_JSONCAST" turned into "JSONTYPE_JSON". Should that have
> been "JSONTYPE_CAST" (that seems to be the corresponding value in the
> new enum) instead?
>
> Thanks,
> Maciek
>
> [0]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3c152a27b06313fe27bd47079658f928e291986b

On Wed, Mar 12, 2025 at 10:00 AM Tender Wang <tndrwang@gmail.com> wrote:
> Maciek Sakrejda <maciek@pganalyze.com> 于2025年3月11日周二 08:12写道：
>> While exploring the jsonb code, I noticed that in
>> datum_to_jsonb_internal, the tcategory checks compares against
>> JSONTYPE_JSON twice. There's no reason for that, right?
>
> Yeah, the second JSONTYPE_JSON seems redundant.
>>
>> Ok, so, to try to answer my own question, I went looking at the
>> history, and this comes from "Unify JSON categorize type API and
>> export for external use" [0]. Specifically, the change was
>>
>> -            (tcategory == JSONBTYPE_ARRAY ||
>> -             tcategory == JSONBTYPE_COMPOSITE ||
>> -             tcategory == JSONBTYPE_JSON ||
>> -             tcategory == JSONBTYPE_JSONB ||
>> -             tcategory == JSONBTYPE_JSONCAST))
>> +            (tcategory == JSONTYPE_ARRAY ||
>> +             tcategory == JSONTYPE_COMPOSITE ||
>> +             tcategory == JSONTYPE_JSON ||
>> +             tcategory == JSONTYPE_JSONB ||
>> +             tcategory == JSONTYPE_JSON))
>>
>> So "JSONBTYPE_JSONCAST" turned into "JSONTYPE_JSON". Should that have
>> been "JSONTYPE_CAST" (that seems to be the corresponding value in the
>> new enum) instead?
>
> The below else branch has code if (tcategory == JSONTYPE_CAST). I guess here the
> second JSONTYPE_JSON may just be removed.
> @Amit Langote please check out this.

Looks like a copy-paste bug on my part.  Will fix, thanks for the report.

--
Thanks, Amit Langote

On Wed, Mar 12, 2025 at 12:07 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Wed, Mar 12, 2025 at 10:00 AM Tender Wang <tndrwang@gmail.com> wrote:
> > Maciek Sakrejda <maciek@pganalyze.com> 于2025年3月11日周二 08:12写道：
> >> While exploring the jsonb code, I noticed that in
> >> datum_to_jsonb_internal, the tcategory checks compares against
> >> JSONTYPE_JSON twice. There's no reason for that, right?
> >
> > Yeah, the second JSONTYPE_JSON seems redundant.
> >>
> >> Ok, so, to try to answer my own question, I went looking at the
> >> history, and this comes from "Unify JSON categorize type API and
> >> export for external use" [0]. Specifically, the change was
> >>
> >> -            (tcategory == JSONBTYPE_ARRAY ||
> >> -             tcategory == JSONBTYPE_COMPOSITE ||
> >> -             tcategory == JSONBTYPE_JSON ||
> >> -             tcategory == JSONBTYPE_JSONB ||
> >> -             tcategory == JSONBTYPE_JSONCAST))
> >> +            (tcategory == JSONTYPE_ARRAY ||
> >> +             tcategory == JSONTYPE_COMPOSITE ||
> >> +             tcategory == JSONTYPE_JSON ||
> >> +             tcategory == JSONTYPE_JSONB ||
> >> +             tcategory == JSONTYPE_JSON))
> >>
> >> So "JSONBTYPE_JSONCAST" turned into "JSONTYPE_JSON". Should that have
> >> been "JSONTYPE_CAST" (that seems to be the corresponding value in the
> >> new enum) instead?
> >
> > The below else branch has code if (tcategory == JSONTYPE_CAST). I guess here the
> > second JSONTYPE_JSON may just be removed.
> > @Amit Langote please check out this.
>
> Looks like a copy-paste bug on my part.  Will fix, thanks for the report.

I was able to construct a test case that crashes due to this bug:

CREATE TYPE mood AS ENUM ('happy', 'sad', 'neutral');
CREATE FUNCTION mood_to_json(mood) RETURNS json AS $$
  SELECT to_json($1::text);
$$ LANGUAGE sql IMMUTABLE;
CREATE CAST (mood AS json) WITH FUNCTION mood_to_json(mood) AS IMPLICIT;

SELECT JSON_OBJECT('happy'::mood: '123'::jsonb);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

Attached patch adds the one-line fix and the above test case.

Will push tomorrow.

--
Thanks, Amit Langote

On 2025-Mar-12, Amit Langote wrote:

> I was able to construct a test case that crashes due to this bug:
> 
> CREATE TYPE mood AS ENUM ('happy', 'sad', 'neutral');
> CREATE FUNCTION mood_to_json(mood) RETURNS json AS $$
>   SELECT to_json($1::text);
> $$ LANGUAGE sql IMMUTABLE;
> CREATE CAST (mood AS json) WITH FUNCTION mood_to_json(mood) AS IMPLICIT;
> 
> SELECT JSON_OBJECT('happy'::mood: '123'::jsonb);
> server closed the connection unexpectedly

Good reaction time :-)  I see that that line shows as not even uncovered
in the report, but as non-existant (no background color as opposed to
red):

https://coverage.postgresql.org/src/backend/utils/adt/jsonb.c.gcov.html#660

Evidently the compiler must be optimizing it out as dead code.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Update: super-fast reaction on the Postgres bugs mailing list. The report
was acknowledged [...], and a fix is under discussion.
The wonders of open-source !"
             https://twitter.com/gunnarmorling/status/1596080409259003906

On Wed, Mar 12, 2025 at 7:09 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2025-Mar-12, Amit Langote wrote:
>
> > I was able to construct a test case that crashes due to this bug:
> >
> > CREATE TYPE mood AS ENUM ('happy', 'sad', 'neutral');
> > CREATE FUNCTION mood_to_json(mood) RETURNS json AS $$
> >   SELECT to_json($1::text);
> > $$ LANGUAGE sql IMMUTABLE;
> > CREATE CAST (mood AS json) WITH FUNCTION mood_to_json(mood) AS IMPLICIT;
> >
> > SELECT JSON_OBJECT('happy'::mood: '123'::jsonb);
> > server closed the connection unexpectedly
>
> Good reaction time :-)  I see that that line shows as not even uncovered
> in the report, but as non-existant (no background color as opposed to
> red):
>
> https://coverage.postgresql.org/src/backend/utils/adt/jsonb.c.gcov.html#660
>
> Evidently the compiler must be optimizing it out as dead code.

Ah, I did wonder about the coverage, thanks for pointing it out.

Patch look good for committing?

--
Thanks, Amit Langote

On 2025-Mar-12, Amit Langote wrote:

> Patch look good for committing?

Ah sorry, I should have said so -- yes, it looks good to me.  I feel a
slight dislike for using URL-escaped characters in the mailing list link
you added, because it means I cannot directly copy/paste the message-id
string into my email client program.  Not a huge issue for sure, and it
seems a majority of links in the source tree are already like that
anyway, but this seems an inclusive, safe, welcoming nitpicking space :-)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

Thanks for the quick fix. I was able to reproduce the assertion
failure and to confirm that it's resolved with the patch. Looks good
to me.