Re: Calculate total_table_pages after set_base_rel_sizes() - Mailing list pgsql-hackers

From Edmund Horner
Subject Re: Calculate total_table_pages after set_base_rel_sizes()
Date
Msg-id 153782775738.25845.3844327947842330303.pgcf@coridan.postgresql.org
Whole thread Raw
In response to Calculate total_table_pages after set_base_rel_sizes()  (David Rowley <david.rowley@2ndquadrant.com>)
Responses Re: Calculate total_table_pages after set_base_rel_sizes()  (David Rowley <david.rowley@2ndquadrant.com>)
List pgsql-hackers
David Rowley said:
> I believe that we should be delaying the PlannerInfo's
> total_table_pages calculation until after constraint exclusion and
> partition pruning have taken place. Doing this calculation before we
> determine which relations we don't need to scan can lead to
> incorrectly applying random_page_cost to too many pages processed
> during an Index Scan.
> 
> We already don't count relations removed by join removals from this
> calculation, so counting pruned partitions seems like an omission.
> 
> The attached patch moves the calculation to after set_base_rel_sizes()
> is called and before set_base_rel_pathlists() is called, where the
> information is actually used.
> 
> I am considering this a bug fix, but I'm proposing this for PG12 only
> as I don't think destabilising plans in the back branches is a good
> idea. I'll add this to the September commitfest.

Hi David, I had a quick look at this.  (I haven't tested it so this isn't a full review.)

It looks like a fairly straightforward code move.  And I think it's correct to exclude the pages from partitions that
won'tbe read.
 

I have a small tweak.  In make_one_rel, we currently have:

    /*
     * Compute size estimates and consider_parallel flags for each base rel,
     * then generate access paths.
     */
    set_base_rel_sizes(root);
    set_base_rel_pathlists(root);

Your patch inserts code between the two lines.  I think the comment should be split too.

    /* Compute size estimates and consider_parallel flags for each base rel. */
    set_base_rel_sizes(root);

    // NEW CODE

    /* Generate access paths. */
    set_base_rel_pathlists(root);

Cheers,
Edmund

pgsql-hackers by date:

Previous
From: Lukas Fittl
Date:
Subject: Re: auto_explain: Include JIT output if applicable
Next
From: Joe Wildish
Date:
Subject: Re: Implementing SQL ASSERTION