Re: BUG #16171: Potential malformed JSON in explain output

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Hamid Akhtar <hamid(dot)akhtar(at)gmail(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org, Daniel Gustafsson <daniel(at)yesql(dot)se>
Subject: Re: BUG #16171: Potential malformed JSON in explain output
Date: 2020-02-01 19:37:55
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-bugs pgsql-hackers

Hamid Akhtar <hamid(dot)akhtar(at)gmail(dot)com> writes:
> I've reviewed and verified this patch and IMHO, this is ready to be committed.

I took a look at this and I don't think it's really going in the right
direction. ISTM the clear intent of this code was to attach the "Subplans
Removed" item as a field of the parent [Merge]Append node, but the author
forgot about the intermediate "Plans" array. So I think that, rather than
doubling down on a mistake, we ought to move where the field is generated
so that it *is* a field of the parent node.

Another failure to follow the design conventions for EXPLAIN output is
that in non-text formats, the schema for each node type ought to be fixed;
that is, if a given field can appear for a particular node type and
EXPLAIN options, it should appear always, not be omitted just because it's

So that leads me to propose 0001 attached. This does lead to some field
order rearrangement in text mode, as per the regression test changes,
but I think that's not a big deal. (A change can only happen if there
are initplan(s) attached to the parent node.)

Also, I wondered whether we had any other violations of correct formatting
in this code, which led me to the idea of running auto_explain in JSON
mode and having it feed its result to json_in. This isn't a complete
test because it won't whine about duplicate field names, but it did
correctly find the current bug --- and I couldn't find any others while
running the core regression tests with various auto_explain options.
0002 attached isn't committable, because nobody would want the overhead
in production, but it seems like a good trick to keep up our sleeves.


regards, tom lane

Attachment Content-Type Size
0001-place-subnodes-removed-correctly.patch text/x-diff 8.1 KB
0002-check-json-validity.patch text/x-diff 687 bytes

In response to


Browse pgsql-bugs by date

  From Date Subject
Next Message Jack Plasterer 2020-02-01 21:33:41 Unable to trigger createdb
Previous Message David Steele 2020-02-01 05:31:40 Re: Don't try fetching future segment of a TLI.

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2020-02-01 20:00:53 Re: Created feature for to_date() conversion using patterns 'YYYY-WW', 'YYYY-WW-D', 'YYYY-MM-W' and 'YYYY-MM-W-D'
Previous Message Pavel Stehule 2020-02-01 18:00:34 Re: [Proposal] Global temporary tables