Re: Question about duplicate JSONTYPE_JSON check

On Wed, Mar 12, 2025 at 12:07 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Wed, Mar 12, 2025 at 10:00 AM Tender Wang <tndrwang(at)gmail(dot)com> wrote:
> > Maciek Sakrejda <maciek(at)pganalyze(dot)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