Обсуждение: selecting from partitions and constraint exclusion
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
Вложения
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
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
Вложения
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
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
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
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
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