Message-ID: <d42146aa-1a03-70ec-72be-d368de2a4ba1@dunslane.net>
Date: Thu, 25 Jan 2024 15:25:28 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: More new SQL/JSON item methods
Content-Language: en-US
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Jeevan Chalke <jeevan.chalke@enterprisedb.com>,
 Peter Eisentraut <peter@eisentraut.org>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: 
 <CAM2+6=XjTyqrrqHAOj80r0wVQxJSxc0iyib9bPC55uFO9VKatg@mail.gmail.com>
 <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>
 <439811.1706211069@sss.pgh.pa.us>
From: Andrew Dunstan <andrew@dunslane.net>
In-Reply-To: <439811.1706211069@sss.pgh.pa.us>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/d42146aa-1a03-70ec-72be-d368de2a4ba1%40dunslane.net>
Precedence: bulk


On 2024-01-25 Th 14:31, Tom Lane wrote:
> 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
>
> ==00:00:00:05.637 435530== Conditional jump or move depends on uninitialised value(s)
> ==00:00:00:05.637 435530==    at 0x8FD131: executeItemOptUnwrapTarget (jsonpath_exec.c:1547)
> ==00:00:00:05.637 435530==    by 0x8FED03: executeItem (jsonpath_exec.c:626)
> ==00:00:00:05.637 435530==    by 0x8FED03: executeNextItem (jsonpath_exec.c:1604)
> ==00:00:00:05.637 435530==    by 0x8FCA58: executeItemOptUnwrapTarget (jsonpath_exec.c:956)
> ==00:00:00:05.637 435530==    by 0x8FFDE4: executeItem (jsonpath_exec.c:626)
> ==00:00:00:05.637 435530==    by 0x8FFDE4: executeJsonPath.constprop.30 (jsonpath_exec.c:612)
> ==00:00:00:05.637 435530==    by 0x8FFF8C: jsonb_path_query_internal (jsonpath_exec.c:438)
>
> It's fairly obviously right about that:
>
>                  JsonbValue    jbv;
>                  ...
>                  jb = &jbv;
>                  Assert(tmp != NULL);    /* We must have set tmp above */
>                  jb->val.string.val = (jb->type == 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 = (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 = &jbv;
>                  Assert(tmp != NULL);    /* We must have set tmp above */
> -               jb->val.string.val = (jb->type == jbvString) ? tmp : pstrdup(tmp);
> +               jb->val.string.val = tmp;
>                  jb->val.string.len = strlen(jb->val.string.val);
>                  jb->type = jbvString;
>   
> and that quieted valgrind for this particular query and still
> passes regression.


Your fix looks sane. I also don't see why we need the pstrdup.


>
> (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.)
>
> 			


I don't think so. AIUI The first call deals with the '$' and the second 
one deals with the '.string()', which is why we see the error on the 
second call.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com