Horton

Re: More new SQL/JSON item methods

Andrew Dunstan <andrew@dunslane.net>

View thread
From: Andrew Dunstan <andrew@dunslane.net>
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>
Date: 2024-01-25T19:41:07Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Rationalize and improve error messages for some jsonpath items

  2. Clean up a bug in sql/json items commit 66ea94e8e6

  3. Implement various jsonpath methods

  4. Reorganise jsonpath operators and methods

  5. Add numeric_int8_opt_error() to optionally suppress errors

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


Argh! Will look, thanks.


cheers


andrew


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