Thread: Should total_pages be calculated after partition pruning andconstraint exclusion?

It seems quite strange to me that we calculate total_pages before any
partition pruning and constraint exclusion is performed during
set_base_rel_sizes(). Wouldn't it be better to wait until after that's
done so we don't mistakenly count relations we're not going to scan?

Delaying the calculation until after set_base_rel_sizes() means it's
still done in time before it's needed in set_base_rel_pathlists().

The attached patch implements the change.

There are no visible plan changes in the regression tests, but the
change can affect the plans for larger partitioned tables.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Attachment
David Rowley <david.rowley@2ndquadrant.com> writes:
> It seems quite strange to me that we calculate total_pages before any
> partition pruning and constraint exclusion is performed during
> set_base_rel_sizes(). Wouldn't it be better to wait until after that's
> done so we don't mistakenly count relations we're not going to scan?

> Delaying the calculation until after set_base_rel_sizes() means it's
> still done in time before it's needed in set_base_rel_pathlists().

I think that when this was first done, there was not a separation between
those passes over the rangetable, so that such a simple fix would have
been impossible.

> The attached patch implements the change.

Please add to next CF.  It's a bit late to be doing such things in v11,
IMO.

            regards, tom lane


On 16 May 2018 at 08:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Rowley <david.rowley@2ndquadrant.com> writes:
>> The attached patch implements the change.
>
> Please add to next CF.  It's a bit late to be doing such things in v11,
> IMO.

If I do that, it'll go under bug fix. It seems strange to delay fixing
this until v12. We've still got 1 week before beta.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


David Rowley <david.rowley@2ndquadrant.com> writes:
> On 16 May 2018 at 08:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> David Rowley <david.rowley@2ndquadrant.com> writes:
>> Please add to next CF.  It's a bit late to be doing such things in v11,
>> IMO.

> If I do that, it'll go under bug fix.

No, it should go under "planner improvement".  If this were a bug fix,
it'd be a candidate for back-patch, which IMO it's not --- if only
because of the risk of plan destabilization.  But for a more direct
analogy, if you'd submitted some other improvement in selectivity
or cost estimation, that was not a bug fix in the sense of "corrects
outright wrong answers", would you expect it to escape feature freeze
at this point?

Beta or no beta, there has to be a pretty good argument why changes
should go into v11 and not v12 once we're past feature freeze.

            regards, tom lane


On 16 May 2018 at 11:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Rowley <david.rowley@2ndquadrant.com> writes:
>> On 16 May 2018 at 08:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> David Rowley <david.rowley@2ndquadrant.com> writes:
>>> Please add to next CF.  It's a bit late to be doing such things in v11,
>>> IMO.
>
>> If I do that, it'll go under bug fix.
>
> No, it should go under "planner improvement".  If this were a bug fix,
> it'd be a candidate for back-patch, which IMO it's not --- if only
> because of the risk of plan destabilization.  But for a more direct
> analogy, if you'd submitted some other improvement in selectivity
> or cost estimation, that was not a bug fix in the sense of "corrects
> outright wrong answers", would you expect it to escape feature freeze
> at this point?

I'm not going to make a fuss over it, but I guess we must differ in
opinion as I would count tracking relation sizes of relations we're
actually not going to scan as a bug. I wouldn't have pushed a
backpatch due to possible plan destabilisation. People may have pushed
effective_cache_size beyond their machines RAM size to work around it.

I'll add to the commitfest before it opens., if it's going in as
"planner improvement" I'll probably hold off until I'm ready with the
other improvements that I'm working on.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


David Rowley <david.rowley@2ndquadrant.com> writes:
> On 16 May 2018 at 11:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> No, it should go under "planner improvement".  If this were a bug fix,
>> it'd be a candidate for back-patch, which IMO it's not --- if only
>> because of the risk of plan destabilization.

> I'm not going to make a fuss over it, but I guess we must differ in
> opinion as I would count tracking relation sizes of relations we're
> actually not going to scan as a bug.

Dunno, the way in which total_pages is used is so squishy/heuristic
that it's really hard to say that changing the way we calculate it
is necessarily going to lead to better plans.  I agree that this is
*probably* an improvement, but I don't think it's absolutely clear.

If there were some way to paint this as connected to other stuff
already done for v11, I'd be happier about pushing it in now --- but
it doesn't seem to be.

            regards, tom lane