Horton

Thread

Commits

  1. Clean up handling of constraint_exclusion and enable_partition_pruning.

  1. selecting from partitions and constraint exclusion

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-03-20T04:37:13Z

    Hi,

While looking at a partition pruning bug [1], I noticed something that
started to feel like a regression:

Setup:

create table p (a int) partition by list (a);
create table p1 partition of p for values in (1);

In PG 10:

set constraint_exclusion to on;
explain select * from p1 where a = 2;
                QUERY PLAN
──────────────────────────────────────────
 Result  (cost=0.00..0.00 rows=0 width=4)
   One-Time Filter: false
(2 rows)

In PG 11 (and HEAD):

set constraint_exclusion to on;
explain select * from p1 where a = 2;
                     QUERY PLAN
────────────────────────────────────────────────────
 Seq Scan on p1  (cost=0.00..41.88 rows=13 width=4)
   Filter: (a = 2)
(2 rows)

That's because get_relation_constraints() no longer (as of PG 11) includes
the partition constraint for SELECT queries.  But that's based on an
assumption that partitions are always accessed via parent, so partition
pruning would make loading the partition constraint unnecessary.  That's
not always true, as shown in the above example.

Should we fix that?  I'm attaching a patch here.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/00e601d4ca86$932b8bc0$b982a340$@lab.ntt.co.jp

  2. Re: selecting from partitions and constraint exclusion

    David Rowley <david.rowley@2ndquadrant.com> — 2019-03-20T10:41:53Z

    On Wed, 20 Mar 2019 at 17:37, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> That's because get_relation_constraints() no longer (as of PG 11) includes
> the partition constraint for SELECT queries.  But that's based on an
> assumption that partitions are always accessed via parent, so partition
> pruning would make loading the partition constraint unnecessary.  That's
> not always true, as shown in the above example.
>
> Should we fix that?  I'm attaching a patch here.

Perhaps we should. The constraint_exclusion documents [1] just mention:

> Controls the query planner's use of table constraints to optimize queries.

and I'm pretty sure you could class the partition constraint as a
table constraint.

As for the patch:

+ if ((root->parse->commandType == CMD_SELECT && !IS_OTHER_REL(rel)) ||

Shouldn't this really be checking rel->reloptkind == RELOPT_BASEREL
instead of !IS_OTHER_REL(rel) ?

For the comments:

+ * For selects, we only need those if the partition is directly mentioned
+ * in the query, that is not via parent.  In case of the latter, partition
+ * pruning, which uses the parent table's partition bound descriptor,
+ * ensures that we only consider partitions whose partition constraint
+ * satisfy the query quals (or, the two don't contradict each other), so
+ * loading them is pointless.
+ *
+ * For updates and deletes, we always need those for performing partition
+ * pruning using constraint exclusion, but, only if pruning is enabled.

You mention "the latter", normally you'd only do that if there was a
former, but in this case there's not.

How about just making it:

/*
 * Append partition predicates, if any.
 *
 * For selects, partition pruning uses the parent table's partition bound
 * descriptor, so there's no need to include the partition constraint for
 * this case.  However, if the partition is referenced directly in the query
 * then no partition pruning will occur, so we'll include it in the case.
 */
if ((root->parse->commandType != CMD_SELECT && enable_partition_pruning) ||
    (root->parse->commandType == CMD_SELECT && rel->reloptkind ==
RELOPT_BASEREL))

For the tests, it seems excessive to create some new tables for this.
Won't the tables in the previous test work just fine?

[1] https://www.postgresql.org/docs/devel/runtime-config-query.html#GUC-CONSTRAINT-EXCLUSION

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

  3. Re: selecting from partitions and constraint exclusion

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-03-22T08:17:25Z

    Hi David,

Thanks for checking.

On 2019/03/20 19:41, David Rowley wrote:
> On Wed, 20 Mar 2019 at 17:37, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> That's because get_relation_constraints() no longer (as of PG 11) includes
>> the partition constraint for SELECT queries.  But that's based on an
>> assumption that partitions are always accessed via parent, so partition
>> pruning would make loading the partition constraint unnecessary.  That's
>> not always true, as shown in the above example.
>>
>> Should we fix that?  I'm attaching a patch here.
> 
> Perhaps we should. The constraint_exclusion documents [1] just mention:
> 
>> Controls the query planner's use of table constraints to optimize queries.
> 
> and I'm pretty sure you could class the partition constraint as a
> table constraint.

Yes.

> As for the patch:
> 
> + if ((root->parse->commandType == CMD_SELECT && !IS_OTHER_REL(rel)) ||
> 
> Shouldn't this really be checking rel->reloptkind == RELOPT_BASEREL
> instead of !IS_OTHER_REL(rel) ?

Hmm, thought I'd use the macro if we have one, but I'll change as you
suggest if that's what makes the code easier to follow.  As you might
know, we can only get "simple" relations here.

> For the comments:
> 
> + * For selects, we only need those if the partition is directly mentioned
> + * in the query, that is not via parent.  In case of the latter, partition
> + * pruning, which uses the parent table's partition bound descriptor,
> + * ensures that we only consider partitions whose partition constraint
> + * satisfy the query quals (or, the two don't contradict each other), so
> + * loading them is pointless.
> + *
> + * For updates and deletes, we always need those for performing partition
> + * pruning using constraint exclusion, but, only if pruning is enabled.
> 
> You mention "the latter", normally you'd only do that if there was a
> former, but in this case there's not.

I was trying to go for "accessing partition directly" as the former and
"accessing it via the parent" as the latter, but maybe the sentence as
written cannot be read that way.

> How about just making it:
> 
> /*
>  * Append partition predicates, if any.
>  *
>  * For selects, partition pruning uses the parent table's partition bound
>  * descriptor, so there's no need to include the partition constraint for
>  * this case.  However, if the partition is referenced directly in the query
>  * then no partition pruning will occur, so we'll include it in the case.
>  */
> if ((root->parse->commandType != CMD_SELECT && enable_partition_pruning) ||
>     (root->parse->commandType == CMD_SELECT && rel->reloptkind ==
> RELOPT_BASEREL))

OK, I will use this text.

> For the tests, it seems excessive to create some new tables for this.
> Won't the tables in the previous test work just fine?

OK, I have revised the tests to use existing tables.

I'll add this to July fest to avoid forgetting about this.

Thanks,
Amit

  4. Re: selecting from partitions and constraint exclusion

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-03-25T00:31:41Z

    On 2019/03/22 17:17, Amit Langote wrote:
> I'll add this to July fest to avoid forgetting about this.

I'd forgotten to do this, but done today. :)

Thanks,
Amit

  5. Re: selecting from partitions and constraint exclusion

    Robert Haas <robertmhaas@gmail.com> — 2019-03-25T15:21:32Z

    On Wed, Mar 20, 2019 at 12:37 AM Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> That's because get_relation_constraints() no longer (as of PG 11) includes
> the partition constraint for SELECT queries.

What commit made that change?

This sounds to me like maybe it should be an open item.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  6. Re: selecting from partitions and constraint exclusion

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-03-26T00:36:39Z

    On 2019/03/26 0:21, Robert Haas wrote:
> On Wed, Mar 20, 2019 at 12:37 AM Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> That's because get_relation_constraints() no longer (as of PG 11) includes
>> the partition constraint for SELECT queries.
> 
> What commit made that change?

That would be 9fdb675fc5d2 (faster partition pruning) that got into PG 11.

> This sounds to me like maybe it should be an open item.

I've added this under Older Bugs.

Thanks,
Amit

  7. Re: selecting from partitions and constraint exclusion

    Thibaut Madelaine <thibaut.madelaine@dalibo.com> — 2019-04-05T16:12:12Z

    Le 25/03/2019 à 01:31, Amit Langote a écrit :
> On 2019/03/22 17:17, Amit Langote wrote:
>> I'll add this to July fest to avoid forgetting about this.
> I'd forgotten to do this, but done today. :)
>
> Thanks,
> Amit

Hello Amit,

Just a quick information that your last patch does not apply on head:

$ git apply
~/Téléchargements/v2-0001-Fix-planner-to-load-partition-constraint-in-some-.patch
error: patch failed: src/test/regress/expected/partition_prune.out:3637
error: src/test/regress/expected/partition_prune.out: patch does not apply

Manually applying it on top of Hosoya's last 2 patches, It corrects the
different cases we found so far.
I will keep on testing next week.

Cordialement,

Thibaut

  8. Re: selecting from partitions and constraint exclusion

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-04-08T04:45:35Z

    Hi Thibaut,

On 2019/04/06 1:12, Thibaut wrote:
> Le 25/03/2019 à 01:31, Amit Langote a écrit :
>> On 2019/03/22 17:17, Amit Langote wrote:
>>> I'll add this to July fest to avoid forgetting about this.
>> I'd forgotten to do this, but done today. :)
>>
>> Thanks,
>> Amit
> 
> Hello Amit,
> 
> Just a quick information that your last patch does not apply on head:
> 
> $ git apply
> ~/Téléchargements/v2-0001-Fix-planner-to-load-partition-constraint-in-some-.patch
> error: patch failed: src/test/regress/expected/partition_prune.out:3637
> error: src/test/regress/expected/partition_prune.out: patch does not apply
> 
> Manually applying it on top of Hosoya's last 2 patches, It corrects the
> different cases we found so far.
> I will keep on testing next week.

Thanks for the heads up.

We are discussing this and another related matter on a different thread
(titled "speeding up planning with partitions" [1]).  Maybe, the problem
originally reported here will get resolved there once we reach consensus
first on what to do in the HEAD branch and what's back-patchable as a
bug-fix to the PG 11 branch.

[1]
https://www.postgresql.org/message-id/50415da6-0258-d135-2ba4-197041b57c5b%40lab.ntt.co.jp