Message-ID: <682c8fff-355c-a04f-57ac-81055c4ccda8@dunslane.net>
Date: Thu, 7 Mar 2024 22:42:06 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Content-Language: en-US
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Peter Smith <smithpb2250@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <7b0a51d6-0d9d-7366-3a1a-f74397a02f55@dunslane.net>
 <99332c8b-0bea-1ef4-efa6-406635eb34b7@dunslane.net>
 <8cc158a4-5d51-737c-16a1-8885d04e4cab@dunslane.net>
 <CAOYmi+mPoQyUjjp01U_x1U02k3QkFyoZ6Z4sm5oxF8HhsLrGzg@mail.gmail.com>
 <3892d996-0ec8-3a56-1a6c-b397a3c31516@dunslane.net>
 <CAOYmi+mZ6ZVDHsP6Fcx+3vGYRSY+7fXmQP7wNuk1KTOVtwK2Pw@mail.gmail.com>
 <CAOYmi+mZER=D1gEDOt7Td8tB214eHFbViYvDvEjF4uiJ3bFv=A@mail.gmail.com>
 <3efe8333-3285-1d9d-5ad3-0f9784d5c1c4@dunslane.net>
 <CAOYmi+kLQmvpGzMsry67B5=1JYhm04rv8J7QX-qh3Xy-kceGJw@mail.gmail.com>
 <1778cdac-bb93-5bbd-1505-5684e95b0094@dunslane.net>
 <CAOYmi+nHV55Uhz+o-HKq0GNiWn2L5gMcuyRQEz_fqpGY=pFxKA@mail.gmail.com>
 <CAOYmi+=DAS_SpWM8Qd-C2Cv0yWUkENbsHcjEYt4vdCAp_BG_Jw@mail.gmail.com>
 <1b30469a-9d29-c021-f28a-74e248760aa4@dunslane.net>
 <ec7d0be8-79f3-05ea-d5ed-323e496556dc@dunslane.net>
 <CAOYmi+kOY47od5NuB+379XSnAfTknocLkYV6qz+Gk1XRPwG1iQ@mail.gmail.com>
 <CAOYmi+kNPw+3hjjQarC8bm-iDa=CRmTB1TMb35zbydRLK+7+1A@mail.gmail.com>
From: Andrew Dunstan <andrew@dunslane.net>
Subject: Re: WIP Incremental JSON Parser
In-Reply-To: <CAOYmi+kNPw+3hjjQarC8bm-iDa=CRmTB1TMb35zbydRLK+7+1A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/682c8fff-355c-a04f-57ac-81055c4ccda8%40dunslane.net>
Precedence: bulk


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