From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andrew Dunstan <andrew@dunslane.net>
cc: Jeevan Chalke <jeevan.chalke@enterprisedb.com>,
        Peter Eisentraut <peter@eisentraut.org>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: More new SQL/JSON item methods
In-reply-to: <5d37428e-0753-e721-8e77-81cc4d0dc9f1@dunslane.net>
References: 
 <CAM2+6=XjTyqrrqHAOj80r0wVQxJSxc0iyib9bPC55uFO9VKatg@mail.gmail.com>
 <bc03e76e-15d6-744e-db53-2e4417b5fe05@dunslane.net>
 <CAM2+6=UCSp74devvHYpMx5NvpyWbCO3gYvgpEYHtNYqF2j6Z9Q@mail.gmail.com>
 <c3158517-940d-278e-d9ea-846c65430a14@dunslane.net>
 <CAM2+6=UrDfcsz0xn+oUmomOnPYazuP8dAM_Wrx5YMsWxXjWyKA@mail.gmail.com>
 <d37da0bc-204a-125a-dfeb-c3d7cc639d69@dunslane.net>
 <CAM2+6=UMCtXVSk1NmGHzsYYY6xGdnaMMKxVaKUtG7o0jkiuyQg@mail.gmail.com>
 <e913dde5-0209-441c-8040-dd2ace6415e0@eisentraut.org>
 <fa454906-7539-4070-b51a-2e7dcfb6b741@eisentraut.org>
 <CAM2+6=XPHNJnGypXy2C_5TUWgCxCktz1GrwuzsSZSqb_R+pMXQ@mail.gmail.com>
 <CAM2+6=WYbL4gxpmEJcN5BvLYuMZUpruLQRZxFeKTBNNpksgN=g@mail.gmail.com>
 <cc883de1-ae89-438b-a97e-926ad332f5ba@eisentraut.org>
 <CAM2+6=WyoinYoiuf3msxqH-hsPBoUz33MmrCpD_4Ceo6Nq1qOg@mail.gmail.com>
 <1d092c02-8249-4e56-8b8e-ae4db95b8fde@eisentraut.org>
 <CAM2+6=Wys-tgwA15WkgW+8uR0SKncFLZuj0B+ukjvxbrebz9qQ@mail.gmail.com>
 <5d37428e-0753-e721-8e77-81cc4d0dc9f1@dunslane.net>
Comments: In-reply-to Andrew Dunstan <andrew@dunslane.net>
	message dated "Thu, 25 Jan 2024 10:40:26 -0500"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <439810.1706211069.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 25 Jan 2024 14:31:09 -0500
Message-ID: <439811.1706211069@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/439811.1706211069%40sss.pgh.pa.us>
Precedence: bulk

Andrew Dunstan <andrew@dunslane.net> writes:
> Thanks, I have pushed this.

The buildfarm is pretty widely unhappy, mostly failing on

select jsonb_path_query('1.23', '$.string()');

On a guess, I tried running that under valgrind, and behold it said

=3D=3D00:00:00:05.637 435530=3D=3D Conditional jump or move depends on uni=
nitialised value(s)
=3D=3D00:00:00:05.637 435530=3D=3D    at 0x8FD131: executeItemOptUnwrapTar=
get (jsonpath_exec.c:1547)
=3D=3D00:00:00:05.637 435530=3D=3D    by 0x8FED03: executeItem (jsonpath_e=
xec.c:626)
=3D=3D00:00:00:05.637 435530=3D=3D    by 0x8FED03: executeNextItem (jsonpa=
th_exec.c:1604)
=3D=3D00:00:00:05.637 435530=3D=3D    by 0x8FCA58: executeItemOptUnwrapTar=
get (jsonpath_exec.c:956)
=3D=3D00:00:00:05.637 435530=3D=3D    by 0x8FFDE4: executeItem (jsonpath_e=
xec.c:626)
=3D=3D00:00:00:05.637 435530=3D=3D    by 0x8FFDE4: executeJsonPath.constpr=
op.30 (jsonpath_exec.c:612)
=3D=3D00:00:00:05.637 435530=3D=3D    by 0x8FFF8C: jsonb_path_query_intern=
al (jsonpath_exec.c:438)

It's fairly obviously right about that:

                JsonbValue    jbv;
                ...
                jb =3D &jbv;
                Assert(tmp !=3D NULL);    /* We must have set tmp above */
                jb->val.string.val =3D (jb->type =3D=3D jbvString) ? tmp :=
 pstrdup(tmp);
                                      ^^^^^^^^^^^^^^^^^^^^^

Presumably, this is a mistaken attempt to test the type
of the thing previously pointed to by "jb".

On the whole, what I'd be inclined to do here is get rid
of this test altogether and demand that every path through
the preceding "switch" deliver a value that doesn't need
pstrdup().  The only path that doesn't do that already is

                    case jbvBool:
                        tmp =3D (jb->val.boolean) ? "true" : "false";
                        break;

and TBH I'm not sure that we really need a pstrdup there
either.  The constants are immutable enough.  Is something
likely to try to pfree the pointer later?  I tried

@@ -1544,7 +1544,7 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt,=
 JsonPathItem *jsp,
 =

                jb =3D &jbv;
                Assert(tmp !=3D NULL);    /* We must have set tmp above */
-               jb->val.string.val =3D (jb->type =3D=3D jbvString) ? tmp :=
 pstrdup(tmp);
+               jb->val.string.val =3D tmp;
                jb->val.string.len =3D strlen(jb->val.string.val);
                jb->type =3D jbvString;
 =

and that quieted valgrind for this particular query and still
passes regression.

(The reported crashes seem to be happening later during a
recursive invocation, seemingly because JsonbType(jb) is
returning garbage.  So there may be another bug after this one.)

			regards, tom lane