Horton

Thread

Commits

  1. Fix handling of "Subplans Removed" field in EXPLAIN output.

  2. Doc: Fix list of storage parameters available for ALTER TABLE

  1. BUG #16171: Potential malformed JSON in explain output

    The Post Office <noreply@postgresql.org> — 2019-12-18T10:28:43Z

    The following bug has been logged on the website:

Bug reference:      16171
Logged by:          Mahadevan Ramachandran
Email address:      mahadevan@rapidloop.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 ..}
    ]

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

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

    Daniel Gustafsson <daniel@yesql.se> — 2019-12-18T15:15:29Z

    > On 18 Dec 2019, at 11:28, PG Bug reporting form <noreply@postgresql.org> wrote:
> 
> The following bug has been logged on the website:
> 
> Bug reference:      16171
> Logged by:          Mahadevan Ramachandran
> Email address:      mahadevan@rapidloop.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

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

    Hamid Akhtar <hamid.akhtar@gmail.com> — 2020-01-24T09:28:50Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

I've reviewed and verified this patch and IMHO, this is ready to be committed.


So I have verified this patch against the tip of REL_12_STABLE branch (commit c4c76d19).
- git apply <patch> works without issues.

Although the patch description mentions that it fixes a malformed JSON in explain output by creating a "Plan" group, this also fixes the same malformation issue for XML and YAML formats. It does not impact the text output though.

I'm sharing the problematic part of output for an unpatched version for JSON, XML, and YAML formats:
-- JSON
       "Plans": [
         "Subplans Removed": 1,
         {
           "Node Type": "Seq Scan",

-- XML
       <Plans>
         <Subplans-Removed>1</Subplans-Removed>
         <Plan>

-- YAML
     Plans:
       Subplans Removed: 1
       - Node Type: "Seq Scan"

The patched version gives the following and correct output:
-- JSON
       "Plans": [
         {
           "Subplans Removed": 1
         }, 
         {
           "Node Type": "Seq Scan",

-- XML
       <Plans>
         <Plan>
           <Subplans-Removed>1</Subplans-Removed>
         </Plan>
         <Plan>

-- YAML
     Plans:
       - Subplans Removed: 1
       - Node Type: "Seq Scan"

Following is the query that I used for validating the output. I picked it up (and simplified) from "src/test/regress/sql/partition_prune.sql". You can probably come up with a simpler query, but this does the job. The query below gives the output in JSON format:
----
create table ab (a int not null, b int not null) partition by list (a);
create table ab_a2 partition of ab for values in(2) partition by list (b);
create table ab_a2_b1 partition of ab_a2 for values in (1);
create table ab_a1 partition of ab for values in(1) partition by list (b);
create table ab_a1_b1 partition of ab_a1 for values in (2);

-- Disallow index only scans as concurrent transactions may stop visibility
-- bits being set causing "Heap Fetches" to be unstable in the EXPLAIN ANALYZE
-- output.
set enable_indexonlyscan = off;

prepare ab_q1 (int, int, int) as
select * from ab where a between $1 and $2 and b <= $3;

-- Execute query 5 times to allow choose_custom_plan
-- to start considering a generic plan.
execute ab_q1 (1, 8, 3);
execute ab_q1 (1, 8, 3);
execute ab_q1 (1, 8, 3);
execute ab_q1 (1, 8, 3);
execute ab_q1 (1, 8, 3);

explain (format json, analyze, costs off, summary off, timing off) execute ab_q1 (2, 2, 3);

deallocate ab_q1;
drop table ab;
----

The new status of this patch is: Ready for Committer

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-01T19:37:55Z

    Hamid Akhtar <hamid.akhtar@gmail.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
zero.

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.

Thoughts?

			regards, tom lane

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

    Daniel Gustafsson <daniel@yesql.se> — 2020-02-02T12:08:07Z

    > On 1 Feb 2020, at 20:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Hamid Akhtar <hamid.akhtar@gmail.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.

Right, that makes sense; +1 on the attached 0001 patch.

> 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.)

Does that prevent backpatching this, or are we Ok with EXPLAIN text output not
being stable across minors?  AFAICT Pg::Explain still works fine with this
change, but mileage may vary for other parsers.

> 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.

Thats a neat trick, I wonder if it would be worth maintaining a curated list of
these tricks in a README under src/test to help others avoid/reduce wheel
reinventing?

cheers ./daniel

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-02T16:48:32Z

    [ cc'ing depesz to see what he thinks about this ]

Daniel Gustafsson <daniel@yesql.se> writes:
> On 1 Feb 2020, at 20:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.)

> Does that prevent backpatching this, or are we Ok with EXPLAIN text output not
> being stable across minors?  AFAICT Pg::Explain still works fine with this
> change, but mileage may vary for other parsers.

I'm not sure about that either.  It should be a clear win for parsers
of the non-text formats, because now we're generating valid
JSON-or-whatever where we were not before.  But it's not too hard to
imagine that someone's ad-hoc parser of text output would break,
depending on how much it relies on field order rather than indentation
to make sense of things.

In the background of all this is that cases where it matters must be
pretty thin on the ground so far, else we'd have gotten complaints
sooner.  So we shouldn't really assume that everyone's parser handles
such cases at all yet.

I'm a little bit inclined to back-patch, on the grounds that JSON
output is hopelessly broken without this, and any text-mode parsers
that need work would need work by September anyway.  But I could
easily be argued into not back-patching.

Another approach we could consider is putting your patch in the
back branches and mine in HEAD.  I'm not sure that's a good idea;
it trades short-term stability of the text format for a long-term
mess in the non-text formats.  But maybe somebody will argue for it.

Thoughts?

			regards, tom lane

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

    hubert depesz lubaczewski <depesz@depesz.com> — 2020-02-03T11:40:22Z

    On Sun, Feb 02, 2020 at 11:48:32AM -0500, Tom Lane wrote:
> > Does that prevent backpatching this, or are we Ok with EXPLAIN text output not
> > being stable across minors?  AFAICT Pg::Explain still works fine with this
> > change, but mileage may vary for other parsers.
> I'm not sure about that either.  It should be a clear win for parsers
> of the non-text formats, because now we're generating valid
> JSON-or-whatever where we were not before.  But it's not too hard to
> imagine that someone's ad-hoc parser of text output would break,
> depending on how much it relies on field order rather than indentation
> to make sense of things.

Change looks reasonable to me.

Interestingly Pg::Explain doesn't handle either current JSON output in
this case (as it's not a valid JSON), nor the new one - but this can be
fixed easily.

Best regards,

depesz

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

    Daniel Gustafsson <daniel@yesql.se> — 2020-02-03T12:55:19Z

    > On 2 Feb 2020, at 17:48, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Thoughts?

Keeping TEXT explain stable across minor versions is very appealing, but more
so from a policy standpoint than a technical one.  The real-world implication
is probably quite small, but that's a very unscientific guess (searching Github
didn't turn up anything but thats far from conclusive).  Having broken JSON is
however a clear negative, and so is having a different JSON format in back-
branches for something which has never worked in the first place.

I guess I'm leaning towards backpatching, but it's not entirely clear-cut.

cheers ./daniel

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-03T14:15:42Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> I guess I'm leaning towards backpatching, but it's not entirely clear-cut.

That's where I stand too.  I'll wait a day or so to see if anyone
else comments; but if not, I'll back-patch.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-03T15:56:09Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> On 1 Feb 2020, at 20:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.

> Thats a neat trick, I wonder if it would be worth maintaining a curated list of
> these tricks in a README under src/test to help others avoid/reduce wheel
> reinventing?

It occurred to me that as long as this is an uncommittable hack anyway,
we could feed the EXPLAIN data to jsonb_in and then hot-wire the jsonb
code to whine about duplicate keys.  So attached, for the archives' sake,
is an improved version that does that.  I still don't find any problems
(other than the one we're fixing here); though no doubt if I reverted
100136849 it'd complain about that.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-04T18:08:46Z

    I wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
>> I guess I'm leaning towards backpatching, but it's not entirely clear-cut.

> That's where I stand too.  I'll wait a day or so to see if anyone
> else comments; but if not, I'll back-patch.

Hearing no objections, done that way.

			regards, tom lane