Thanks for taking a look.
On 2019/04/02 2:34, Tom Lane wrote:
> Amit Langote <langote_amit...@lab.ntt.co.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
diff --git a/src/backend/optimizer/plan/planner.c
b/src/backend/optimizer/plan/planner.c
index 3a1b846217..de655892f6 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1513,8 +1513,9 @@ inheritance_planner(PlannerInfo *root)
parent_rte->securityQuals = NIL;
/*
- * Mark whether we're planning a query to a partitioned table
or an
- * inheritance parent.
+ * HACK: setting this to a value other than INHKIND_NONE
signals to
+ * relation_excluded_by_constraints() to process the result
relation as
+ * a partition; see that function for more details.
*/
subroot->inhTargetKind =
(rootRelation != 0) ? INHKIND_PARTITIONED :
INHKIND_INHERITED;
diff --git a/src/backend/optimizer/util/plancat.c
b/src/backend/optimizer/util/plancat.c
index 31a3784536..0698bafd04 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -66,7 +66,7 @@ static void get_relation_foreign_keys(PlannerInfo *root,
RelOptInfo *rel,
static bool infer_collation_opclass_match(InferenceElem *elem, Relation idxRel,
List *idxExprs);
static List *get_relation_constraints(PlannerInfo *root,
- Oid relationObjectId,
RelOptInfo *rel,
+ Relation relation, RelOptInfo
*rel,
bool include_notnull);
static List *build_index_tlist(PlannerInfo *root, IndexOptInfo *index,
Relation heapRelation);
@@ -1155,19 +1155,13 @@ get_relation_data_width(Oid relid, int32 *attr_widths)
*/
static List *
get_relation_constraints(PlannerInfo *root,
- Oid relationObjectId,
RelOptInfo *rel,
+ Relation relation, RelOptInfo
*rel,
bool include_notnull)
{
List *result = NIL;
Index varno = rel->relid;
- Relation relation;
TupleConstr *constr;
- /*
- * We assume the relation has already been safely locked.
- */
- relation = table_open(relationObjectId, NoLock);
-
constr = relation->rd_att->constr;
if (constr != NULL)
{
@@ -1247,38 +1241,6 @@ get_relation_constraints(PlannerInfo *root,
}
}
- /*
- * Append partition predicates, if any.
- *
- * For selects, partition pruning uses the parent table's partition
bound
- * descriptor, instead of constraint exclusion which is driven by the
- * individual partition's partition constraint.
- */
- if (enable_partition_pruning && root->parse->commandType != CMD_SELECT)
- {
- List *pcqual = RelationGetPartitionQual(relation);
-
- if (pcqual)
- {
- /*
- * Run the partition quals through const-simplification
similar to
- * check constraints. We skip canonicalize_qual,
though, because
- * partition quals should be in canonical form already;
also,
- * since the qual is in implicit-AND format, we'd have
to
- * explicitly convert it to explicit-AND format and
back again.
- */
- pcqual = (List *) eval_const_expressions(root, (Node *)
pcqual);
-
- /* Fix Vars to have the desired varno */
- if (varno != 1)
- ChangeVarNodes((Node *) pcqual, 1, varno, 0);
-
- result = list_concat(result, pcqual);
- }
- }
-
- table_close(relation, NoLock);
-
return result;
}
@@ -1385,6 +1347,7 @@ relation_excluded_by_constraints(PlannerInfo *root,
List *constraint_pred;
List *safe_constraints;
ListCell *lc;
+ Relation relation;
/* As of now, constraint exclusion works only with simple relations. */
Assert(IS_SIMPLE_REL(rel));
@@ -1415,26 +1378,14 @@ relation_excluded_by_constraints(PlannerInfo *root,
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;
return false;
case CONSTRAINT_EXCLUSION_PARTITION:
/*
* When constraint_exclusion is set to 'partition' we
only handle
- * OTHER_MEMBER_RELs, or BASERELs in cases where the
result target
- * is an inheritance parent or a partitioned table.
+ * OTHER_MEMBER_RELs, or BASERELs in cases where the
relation is
+ * is an inherited target relation.
*/
if ((rel->reloptkind != RELOPT_OTHER_MEMBER_REL) &&
!(rel->reloptkind == RELOPT_BASEREL &&
@@ -1486,10 +1437,55 @@ relation_excluded_by_constraints(PlannerInfo *root,
return false;
/*
+ * We assume the relation has already been safely locked.
+ */
+ relation = table_open(rte->relid, NoLock);
+
+ /*
* OK to fetch the constraint expressions. Include "col IS NOT NULL"
* expressions for attnotnull columns, in case we can refute those.
*/
- constraint_pred = get_relation_constraints(root, rte->relid, rel, true);
+ constraint_pred = get_relation_constraints(root, relation, rel, true);
+
+ /*
+ * Append partition predicates, if any.
+ *
+ * If the partition is accessed indirectly via its parent table,
partition
+ * pruning is performed with the parent table's partition bound
descriptor,
+ * so there is no need to include the partition constraint in that case.
+ * We do need to include it if the partition is referenced directly in
the
+ * query, because no partition pruning would have occurred in that case,
+ * except in the case where the partition is a target relation. (See
+ * inheritance_planner().)
+ */
+ if (rel->reloptkind == RELOPT_BASEREL &&
+ !(root->inhTargetKind == INHKIND_PARTITIONED &&
+ rel->relid == root->parse->resultRelation))
+ {
+ List *pcqual = RelationGetPartitionQual(relation);
+
+ if (pcqual)
+ {
+ Index varno = rel->relid;
+
+ /*
+ * Run the partition quals through const-simplification
similar to
+ * check constraints. We skip canonicalize_qual,
though, because
+ * partition quals should be in canonical form already;
also,
+ * since the qual is in implicit-AND format, we'd have
to
+ * explicitly convert it to explicit-AND format and
back again.
+ */
+ pcqual = (List *) eval_const_expressions(root, (Node *)
pcqual);
+
+ /* Fix Vars to have the desired varno */
+ if (varno != 1)
+ ChangeVarNodes((Node *) pcqual, 1, varno, 0);
+
+ constraint_pred = list_concat(constraint_pred, pcqual);
+ }
+ }
+
+ table_close(relation, NoLock);
/*
* We do not currently enforce that CHECK constraints contain only
diff --git a/src/test/regress/expected/partition_prune.out
b/src/test/regress/expected/partition_prune.out
index 7806ba1d47..e957badf09 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -3643,4 +3643,52 @@ select * from listp where a = (select 2) and b <> 10;
-> Result (never executed)
(4 rows)
+--
+-- check that a partition directly accessed in a query is excluded with
+-- constraint_exclusion = on
+--
+-- turn off partition pruning, so that it doesn't interfere
+set enable_partition_pruning to off;
+-- constraint exclusion doesn't apply
+set constraint_exclusion to 'partition';
+explain (costs off) select * from listp1 where a = 2;
+ QUERY PLAN
+--------------------
+ Seq Scan on listp1
+ Filter: (a = 2)
+(2 rows)
+
+explain (costs off) select * from listp2 where a = 1;
+ QUERY PLAN
+-----------------------
+ Seq Scan on listp2_10
+ Filter: (a = 1)
+(2 rows)
+
+-- constraint exclusion applies
+set constraint_exclusion to 'on';
+explain (costs off) select * from listp1 where a = 2;
+ QUERY PLAN
+--------------------------
+ Result
+ One-Time Filter: false
+(2 rows)
+
+explain (costs off) select * from listp2 where a = 1;
+ QUERY PLAN
+--------------------------
+ Result
+ One-Time Filter: false
+(2 rows)
+
+explain (costs off) update listp set a = listp1.b from listp1 where listp.a =
listp1.a and listp1.a = 2;
+ QUERY PLAN
+--------------------------------
+ Update on listp
+ -> Result
+ One-Time Filter: false
+(3 rows)
+
+reset constraint_exclusion;
+reset enable_partition_pruning;
drop table listp;
diff --git a/src/test/regress/sql/partition_prune.sql
b/src/test/regress/sql/partition_prune.sql
index 2e4d2b483d..ff3312bb31 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -990,4 +990,23 @@ create table listp2_10 partition of listp2 for values in
(10);
explain (analyze, costs off, summary off, timing off)
select * from listp where a = (select 2) and b <> 10;
+--
+-- check that a partition directly accessed in a query is excluded with
+-- constraint_exclusion = on
+--
+
+-- turn off partition pruning, so that it doesn't interfere
+set enable_partition_pruning to off;
+
+-- constraint exclusion doesn't apply
+set constraint_exclusion to 'partition';
+explain (costs off) select * from listp1 where a = 2;
+explain (costs off) select * from listp2 where a = 1;
+-- constraint exclusion applies
+set constraint_exclusion to 'on';
+explain (costs off) select * from listp1 where a = 2;
+explain (costs off) select * from listp2 where a = 1;
+explain (costs off) update listp set a = listp1.b from listp1 where listp.a =
listp1.a and listp1.a = 2;
+reset constraint_exclusion;
+reset enable_partition_pruning;
drop table listp;
