Re: speeding up planning with partitions

Thanks for taking a look.

On 2019/04/02 2:34, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> writes:
>> On 2019/03/30 0:29, Tom Lane wrote:
>>> That seems like probably an independent patch --- do you want to write it?
>
>> Here is that patch.
>> It revises get_relation_constraints() such that the partition constraint
>> is loaded in only the intended cases.
>
> So I see the problem you're trying to solve here, but I don't like this
> patch a bit, because it depends on root->inhTargetKind which IMO is a
> broken bit of junk that we need to get rid of. Here is an example of
> why, with this patch applied:
>
> regression=# create table p (a int) partition by list (a);
> CREATE TABLE
> regression=# create table p1 partition of p for values in (1);
> CREATE TABLE
> regression=# set constraint_exclusion to on;
> SET
> regression=# explain select * from p1 where a = 2;
> QUERY PLAN
> ------------------------------------------
> Result (cost=0.00..0.00 rows=0 width=0)
> One-Time Filter: false
> (2 rows)
>
> So far so good, but watch what happens when we include the same case
> in an UPDATE on some other partitioned table:
>
> regression=# create table prtab (a int, b int) partition by list (a);
> CREATE TABLE
> regression=# create table prtab2 partition of prtab for values in (2);
> CREATE TABLE
> regression=# explain update prtab set b=b+1 from p1 where prtab.a=p1.a and p1.a=2;
> QUERY PLAN
> ---------------------------------------------------------------------------
> Update on prtab (cost=0.00..82.30 rows=143 width=20)
> Update on prtab2
> -> Nested Loop (cost=0.00..82.30 rows=143 width=20)
> -> Seq Scan on p1 (cost=0.00..41.88 rows=13 width=10)
> Filter: (a = 2)
> -> Materialize (cost=0.00..38.30 rows=11 width=14)
> -> Seq Scan on prtab2 (cost=0.00..38.25 rows=11 width=14)
> Filter: (a = 2)
> (8 rows)
>
> No constraint exclusion, while in v10 you get
>
> Update on prtab (cost=0.00..0.00 rows=0 width=0)
> -> Result (cost=0.00..0.00 rows=0 width=0)
> One-Time Filter: false
>
> The reason is that this logic supposes that root->inhTargetKind describes
> *all* partitioned tables in the query, which is obviously wrong.
>
> Now maybe we could make it work by doing something like
>
> if (rel->reloptkind == RELOPT_BASEREL &&
> (root->inhTargetKind == INHKIND_NONE ||
> rel->relid != root->parse->resultRelation))

Ah, you're right. inhTargetKind has to be checked in conjunction with
checking whether the relation is the target relation.

> but I find that pretty messy, plus it's violating the concept that we
> shouldn't be allowing messiness from inheritance_planner to leak into
> other places.

I'm afraid that we'll have to live with this particular hack as long as we
have inheritance_planner(), but we maybe could somewhat reduce the extent
to which the hack is spread into other planner files.

How about we move the part of get_relation_constraints() that loads the
partition constraint to its only caller
relation_excluded_by_constraints()? If we do that, all uses of
root->inhTargetKind will be confined to one place. Attached updated patch
does that.

> What I'd rather do is have this test just read
>
> if (rel->reloptkind == RELOPT_BASEREL)
>
> Making it be that way causes some changes in the partition_prune results,
> as attached, which suggest that removing the enable_partition_pruning
> check as you did wasn't such a great idea either. However, if I add
> that back in, then it breaks the proposed new regression test case.
>
> I'm not at all clear on what we think the interaction between
> enable_partition_pruning and constraint_exclusion ought to be,
> so I'm not sure what the appropriate resolution is here. Thoughts?

Prior to 428b260f87 (that is, in PG 11), partition pruning for UPDATE and
DELETE queries is realized by applying constraint exclusion to the
partition constraint of the target partition. The conclusion of the
discussion when adding the enable_partition_pruning GUC [1] was that
whether or not constraint exclusion is carried out (to facilitate
partition pruning) should be governed by the new GUC, not
constraint_exclusion, if only to avoid confusing users.

428b260f87 has obviated the need to check enable_partition_pruning in
relation_excluded_by_constraints(), because inheritance_planner() now runs
the query as if it were SELECT, which does partition pruning using
partprune.c, governed by the setting of enable_partition_pruning. So,
there's no need to check it again in relation_excluded_by_constraints(),
because we won't be consulting the partition constraint again; well, at
least after applying the proposed patch.

> BTW, just about all the other uses of root->inhTargetKind seem equally
> broken from here; none of them are accounting for whether the rel in
> question is the query target.

There's only one other use of its value, AFAICS:

switch (constraint_exclusion)
{
case CONSTRAINT_EXCLUSION_OFF:

/*
* Don't prune if feature turned off -- except if the relation is
* a partition. While partprune.c-style partition pruning is not
* yet in use for all cases (update/delete is not handled), it
* would be a UI horror to use different user-visible controls
* depending on such a volatile implementation detail. Therefore,
* for partitioned tables we use enable_partition_pruning to
* control this behavior.
*/
if (root->inhTargetKind == INHKIND_PARTITIONED)
break;

Updated patch removes it though. Which other uses are there?

Attached patch is only for HEAD this time. I'll post one for PG 11 (if
you'd like) once we reach consensus on the best thing to do here is.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/flat/CAFjFpRcwq7G16J_w%2Byy_xiE7daD0Bm6iYTnhz81f79yrSOn4DA%40mail.gmail.com