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

> On 18 Dec 2019, at 11:28, PG Bug reporting form <noreply(at)postgresql(dot)org> wrote:
>
> The following bug has been logged on the website:
>
> Bug reference: 16171
> Logged by: Mahadevan Ramachandran
> Email address: mahadevan(at)rapidloop(dot)com
> PostgreSQL version: 12.1
> Operating system: any
> Description:
>
> Refer src/backend/commands/explain.c, version 12.1.
>
> When a plan node has children, the function ExplainNode starts a JSON array
> with the key "Plans" (line 1955), like so:
>
> "Plans": [
>
> with the intention of creating an array of "Plan" objects, one for each
> child:
>
> "Plans": [
> { .. a child plan goes here ..},
> { .. a child plan goes here ..}
> ]
>
> However, if the node (the current, parent one) is of a certain type (see
> switch at line 1975), then ExplainMemberNodes is called, which does this
> (lines 3335-6):
>
> if (nsubnodes < nplans)
> ExplainPropertyInteger("Subplans Removed", NULL, nplans - nsubnodes,
> es);
>
> This can potentially cause a malformed JSON output like this:
>
> "Plans": [
> { .. a child plan goes here ..},
> "Subplans Removed": 5,
> { .. a child plan goes here ..}
> ]

Nice catch! That seems like a correct analysis to me. The same error is
present in YAML output as well AFAICT.

> I don't have a sample explain output that exhibits this error, this was
> found while reviewing the code.

A tip for when you're struggling to get the output you want for testing
something: grep for it in src/test/regress. Chances are there is already a
test covering the precise output you're interested in. For the example at
hand, the partition_prune.sql suite contains quite a few such queries.

Looking at the output from one of them, in text as well as JSON exemplifies the
bug clearly:

QUERY PLAN
--------------------------------------------------------
Append (actual rows=1 loops=1)
InitPlan 1 (returns $0)
-> Result (actual rows=1 loops=1)
Subplans Removed: 2
-> Seq Scan on mc3p1 mc3p_1 (actual rows=1 loops=1)
Filter: ((a = $1) AND (abs(b) < $0))
(6 rows)

QUERY PLAN
------------------------------------------------------
[ +
{ +
"Plan": { +
"Node Type": "Append", +
"Parallel Aware": false, +
"Actual Rows": 2, +
"Actual Loops": 1, +
"Plans": [ +
{ +
"Node Type": "Result", +
"Parent Relationship": "InitPlan", +
"Subplan Name": "InitPlan 1 (returns $0)",+
"Parallel Aware": false, +
"Actual Rows": 1, +
"Actual Loops": 1 +
}, +
"Subplans Removed": 1, +
{ +
"Node Type": "Seq Scan", +
"Parent Relationship": "Member", +
"Parallel Aware": false, +
"Relation Name": "mc3p0", +
"Alias": "mc3p_1", +
"Actual Rows": 1, +
"Actual Loops": 1, +
"Filter": "((a <= $1) AND (abs(b) < $0))",+
"Rows Removed by Filter": 0 +
}, +
{ +
"Node Type": "Seq Scan", +
"Parent Relationship": "Member", +
"Parallel Aware": false, +
"Relation Name": "mc3p1", +
"Alias": "mc3p_2", +
"Actual Rows": 1, +
"Actual Loops": 1, +
"Filter": "((a <= $1) AND (abs(b) < $0))",+
"Rows Removed by Filter": 0 +
} +
] +
}, +
"Triggers": [ +
] +
} +
]
(1 row)

Moving the "Subplans Removed" into a Plan group seems like the least bad option
to clearly identify it while keeping the formatting legal. The attached patch
generates the following output for JSON instead:

"Plans": [ +
{ +
"Node Type": "Result", +
"Parent Relationship": "InitPlan", +
"Subplan Name": "InitPlan 1 (returns $0)",+
"Parallel Aware": false, +
"Actual Rows": 1, +
"Actual Loops": 1 +
}, +
{ +
"Subplans Removed": 2 +
}, +
{ +
"Node Type": "Seq Scan", +
"Parent Relationship": "Member", +
"Parallel Aware": false, +
"Relation Name": "mc3p1", +
"Alias": "mc3p_1", +
"Actual Rows": 1, +
"Actual Loops": 1, +
"Filter": "((a = $1) AND (abs(b) < $0))", +
"Rows Removed by Filter": 0 +
} +

cheers ./daniel