Re: [Maria-developers] MDEV-23766:Optimizer tracing code is prone to producing invalid JSON
Hi Serg, Ok the patch moving in the right direction but it's not there, yet. Please find the input below. First, input on the commit comment. Please consider it as a request applying to ALL further commits to the MariaDB codebase.
MDEV-23766: implemented requested debug checks
Imagine somebody looking at this in a few years. Will they know what checks were requested? They might get a clue by looking at the Jira text, but the idea to make the commit comments concise and self-contained descriptions. Something like this: Line #1: one-line description of the patch:
MDEV-23766: Make attempts to write invalid JSON cause assertion failures
Subsequent lines:
Make JSON writing API (class Json_writer) check the produced JSON document is valid. The following checks are made: - JSON objects must contain named members - JSON arrays must contain unnamed members. - (TODO: add other restrictions we're enforcing).
It is not clear how the checker interacts with Single_line_formatting_helper. The code like one the one I'm quoting below shows some non-trivial overlap:
@@ -28,7 +36,16 @@ void Json_writer::append_indent()
void Json_writer::start_object() { +#ifndef NDEBUG + DBUG_ASSERT(got_name == named_item_expected()); + named_items_expectation.push_back(true); + size_t depth= named_items_expectation.size(); +#endif fmt_helper.on_start_object(); +#ifndef NDEBUG + if (depth != named_items_expectation.size()) + named_items_expectation.pop_back(); +#endif
if (!element_started) start_element();
This looks very cryptic. Can you elaborate about this? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
Hi Serg, This is review input for commit 8fb54fa006367e3a9cbe32dc1734c980cc5d5096. Item #1: I've tried adding some tests and neither of them fails. Can you check what's going on? I'm attaching the patch with tests to this email. Item #2: please improve the commit comment as was requested in my previous e-mail: On Thu, Oct 28, 2021 at 04:58:35PM +0300, Sergey Petrunia wrote:
First, input on the commit comment. Please consider it as a request applying to ALL further commits to the MariaDB codebase.
MDEV-23766: implemented requested debug checks
Imagine somebody looking at this in a few years. Will they know what checks were requested? They might get a clue by looking at the Jira text, but the idea to make the commit comments concise and self-contained descriptions.
Something like this:
Line #1: one-line description of the patch:
MDEV-23766: Make attempts to write invalid JSON cause assertion failures
Subsequent lines:
Make JSON writing API (class Json_writer) check the produced JSON document is valid. The following checks are made: - JSON objects must contain named members - JSON arrays must contain unnamed members. - (TODO: add other restrictions we're enforcing).
BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
participants (1)
-
Sergey Petrunia