public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andrew Dunstan <[email protected]>
To: Jacob Champion <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: WIP Incremental JSON Parser
Date: Thu, 7 Mar 2024 22:42:06 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAOYmi+kNPw+3hjjQarC8bm-iDa=CRmTB1TMb35zbydRLK+7+1A@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<[email protected]>
<CAOYmi+mPoQyUjjp01U_x1U02k3QkFyoZ6Z4sm5oxF8HhsLrGzg@mail.gmail.com>
<[email protected]>
<CAOYmi+mZ6ZVDHsP6Fcx+3vGYRSY+7fXmQP7wNuk1KTOVtwK2Pw@mail.gmail.com>
<CAOYmi+mZER=D1gEDOt7Td8tB214eHFbViYvDvEjF4uiJ3bFv=A@mail.gmail.com>
<[email protected]>
<CAOYmi+kLQmvpGzMsry67B5=1JYhm04rv8J7QX-qh3Xy-kceGJw@mail.gmail.com>
<[email protected]>
<CAOYmi+nHV55Uhz+o-HKq0GNiWn2L5gMcuyRQEz_fqpGY=pFxKA@mail.gmail.com>
<CAOYmi+=DAS_SpWM8Qd-C2Cv0yWUkENbsHcjEYt4vdCAp_BG_Jw@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAOYmi+kOY47od5NuB+379XSnAfTknocLkYV6qz+Gk1XRPwG1iQ@mail.gmail.com>
<CAOYmi+kNPw+3hjjQarC8bm-iDa=CRmTB1TMb35zbydRLK+7+1A@mail.gmail.com>
On 2024-03-07 Th 10:28, Jacob Champion wrote:
> Some more observations as I make my way through the patch:
>
> In src/common/jsonapi.c,
>
>> +#define JSON_NUM_NONTERMINALS 6
> Should this be 5 now?
Yep.
>
>> + res = pg_parse_json_incremental(&(incstate->lex), &(incstate->sem),
>> + chunk, size, is_last);
>> +
>> + expected = is_last ? JSON_SUCCESS : JSON_INCOMPLETE;
>> +
>> + if (res != expected)
>> + json_manifest_parse_failure(context, "parsing failed");
> This leads to error messages like
>
> pg_verifybackup: error: could not parse backup manifest: parsing failed
>
> which I would imagine is going to lead to confused support requests in
> the event that someone does manage to corrupt their manifest. Can we
> make use of json_errdetail() and print the line and column numbers?
> Patch 0001 over at [1] has one approach to making json_errdetail()
> workable in frontend code.
Looks sound on a first look. Maybe we should get that pushed ASAP so we
can take advantage of it.
>
> Top-level scalars like `false` or `12345` do not parse correctly if
> the chunk size is too small; instead json_errdetail() reports 'Token
> "" is invalid'. With small chunk sizes, json_errdetail() additionally
> segfaults on constructions like `[tru]` or `12zz`.
Ugh. Will investigate.
>
> For my local testing, I'm carrying the following diff in
> 001_test_json_parser_incremental.pl:
>
>> - ok($stdout =~ /SUCCESS/, "chunk size $size: test succeeds");
>> - ok(!$stderr, "chunk size $size: no error output");
>> + like($stdout, qr/SUCCESS/, "chunk size $size: test succeeds");
>> + is($stderr, "", "chunk size $size: no error output");
> This is particularly helpful when a test fails spuriously due to code
> coverage spray on stderr.
>
Makes sense, thanks.
I'll have a fresh patch set soon which will also take care of the bitrot.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
view thread (44+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: WIP Incremental JSON Parser
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox