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
