Question about duplicate JSONTYPE_JSON check

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:

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 <amitlangote09@gmail.com> please check out this.

--
Thanks,
Tender Wang

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.

On Wed, Mar 12, 2025 at 21:40 Álvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

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.

Thanks (Maciek, Tender too) for the review.

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 :-)

Ah, no problem, fixed it. I prefer to see URLs in that way too, but somehow
didn’t notice what I had done.