Re: WIP Incremental JSON Parser
Andrew Dunstan <andrew@dunslane.net>
From: Andrew Dunstan <andrew@dunslane.net>
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>
Date: 2024-03-08T03:42:06Z
Lists: pgsql-hackers
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
Commits
-
Post review fixes for test_json_parser test module
- ba3e6e2bca97 17.0 landed
-
Shrink test file for test_json_parser module
- b8a7bfa33324 17.0 landed
-
Add support for incrementally parsing backup manifests
- ea7b4e9a2a7c 17.0 landed
-
Introduce a non-recursive JSON parser
- 3311ea86edc7 17.0 landed
-
Use incremental parsing of backup manifests.
- 222e11a10ae9 17.0 landed