From 16036f23e166bf0d5c3c03a70e92cf0bf13f63ef Mon Sep 17 00:00:00 2001 From: amit Date: Thu, 15 Jun 2017 19:22:31 +0900 Subject: [PATCH 2/2] Teach ATExecAttachPartition to skip validation in more cases In cases where the table being attached is a partitioned table and the table itself does not have constraints that would allow validation on the whole table to be skipped, we can still skip the validations of individual partitions if they each happen to have the requisite constraints. Per an idea of Robert Haas', with code refactoring suggestions from Ashutosh Bapat. --- src/backend/commands/tablecmds.c | 223 ++++++++++++++++-------------- src/test/regress/expected/alter_table.out | 12 ++ src/test/regress/sql/alter_table.sql | 11 ++ 3 files changed, 145 insertions(+), 101 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 683bbbc08f..0d954e7e2d 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -473,6 +473,8 @@ static void CreateInheritance(Relation child_rel, Relation parent_rel); static void RemoveInheritance(Relation child_rel, Relation parent_rel); static ObjectAddress ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd); +static bool PartConstraintImpliedByRelConstraint(Relation partrel, + List *partConstraint); static ObjectAddress ATExecDetachPartition(Relation rel, RangeVar *name); @@ -13408,15 +13410,12 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) Relation attachRel, catalog; List *attachRel_children; - TupleConstr *attachRel_constr; - List *partConstraint, - *existConstraint; + List *partConstraint; SysScanDesc scan; ScanKeyData skey; AttrNumber attno; int natts; TupleDesc tupleDesc; - bool skip_validate = false; ObjectAddress address; attachRel = heap_openrv(cmd->name, AccessExclusiveLock); @@ -13596,89 +13595,10 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) rel); /* - * Check if we can do away with having to scan the table being attached to - * validate the partition constraint, by *proving* that the existing - * constraints of the table *imply* the partition predicate. We include - * the table's check constraints and NOT NULL constraints in the list of - * clauses passed to predicate_implied_by(). - * - * There is a case in which we cannot rely on just the result of the - * proof. + * Based on the table's existing constraints, determine if we can skip the + * partition constraint validation scan. */ - attachRel_constr = tupleDesc->constr; - existConstraint = NIL; - if (attachRel_constr != NULL) - { - int num_check = attachRel_constr->num_check; - int i; - - if (attachRel_constr->has_not_null) - { - int natts = attachRel->rd_att->natts; - - for (i = 1; i <= natts; i++) - { - Form_pg_attribute att = attachRel->rd_att->attrs[i - 1]; - - if (att->attnotnull && !att->attisdropped) - { - NullTest *ntest = makeNode(NullTest); - - ntest->arg = (Expr *) makeVar(1, - i, - att->atttypid, - att->atttypmod, - att->attcollation, - 0); - ntest->nulltesttype = IS_NOT_NULL; - - /* - * argisrow=false is correct even for a composite column, - * because attnotnull does not represent a SQL-spec IS NOT - * NULL test in such a case, just IS DISTINCT FROM NULL. - */ - ntest->argisrow = false; - ntest->location = -1; - existConstraint = lappend(existConstraint, ntest); - } - } - } - - for (i = 0; i < num_check; i++) - { - Node *cexpr; - - /* - * If this constraint hasn't been fully validated yet, we must - * ignore it here. - */ - if (!attachRel_constr->check[i].ccvalid) - continue; - - cexpr = stringToNode(attachRel_constr->check[i].ccbin); - - /* - * Run each expression through const-simplification and - * canonicalization. It is necessary, because we will be - * comparing it to similarly-processed qual clauses, and may fail - * to detect valid matches without this. - */ - cexpr = eval_const_expressions(NULL, cexpr); - cexpr = (Node *) canonicalize_qual((Expr *) cexpr); - - existConstraint = list_concat(existConstraint, - make_ands_implicit((Expr *) cexpr)); - } - - existConstraint = list_make1(make_ands_explicit(existConstraint)); - - /* And away we go ... */ - if (predicate_implied_by(partConstraint, existConstraint, true)) - skip_validate = true; - } - - /* It's safe to skip the validation scan after all */ - if (skip_validate) + if (PartConstraintImpliedByRelConstraint(attachRel, partConstraint)) ereport(INFO, (errmsg("partition constraint for table \"%s\" is implied by existing constraints", RelationGetRelationName(attachRel)))); @@ -13687,12 +13607,15 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) ListCell *lc; /* - * Schedule the table (or leaf partitions if partitioned) to be scanned - * later. + * For each leaf partition, check if it we can skip the validation + * scan because it has constraints that allows to do so. If not, + * schedule it to be scanned later. * - * Note that attachRel's OID is in this list. If it's partitioned, we + * Note that attachRel's OID is in this list. Since we already + * determined above that its validation scan cannot be skipped, we + * need not check that again in the loop below. If it's partitioned, * we don't need to schedule it to be scanned (would be a noop anyway - * even if we did), so just remove it from the list. + * even if we did) either, so just remove it from the list. */ Assert(linitial_oid(attachRel_children) == RelationGetRelid(attachRel)); @@ -13704,7 +13627,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) AlteredTableInfo *tab; Oid part_relid = lfirst_oid(lc); Relation part_rel; - Expr *constr; + List *my_partconstr; /* Lock already taken */ if (part_relid != RelationGetRelid(attachRel)) @@ -13713,6 +13636,23 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) part_rel = attachRel; /* + * Adjust the constraint that we constructed above for attachRel + * so that it matches this partition's attribute numbers. + */ + my_partconstr = map_partition_varattnos(partConstraint, 1, + part_rel, + attachRel); + if (PartConstraintImpliedByRelConstraint(part_rel, my_partconstr)) + { + ereport(INFO, + (errmsg("partition constraint for table \"%s\" is implied by existing constraints", + RelationGetRelationName(part_rel)))); + if (part_rel != attachRel) + heap_close(part_rel, NoLock); + continue; + } + + /* * Skip if it's a partitioned table. Only RELKIND_RELATION * relations (ie, leaf partitions) need to be scanned. */ @@ -13723,18 +13663,10 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) continue; } - /* Grab a work queue entry */ + /* Nope. So grab a work queue entry. */ tab = ATGetQueueEntry(wqueue, part_rel); + tab->partition_constraint = (Expr *) linitial(my_partconstr); - /* - * Adjust the constraint that we constructed above for the table - * being attached so that it matches this partition's attribute - * numbers. - */ - constr = linitial(partConstraint); - tab->partition_constraint = (Expr *) - map_partition_varattnos((List *) constr, 1, - part_rel, attachRel); /* keep our lock until commit */ if (part_rel != attachRel) heap_close(part_rel, NoLock); @@ -13750,6 +13682,95 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) } /* + * skipPartConstraintValidation + * Can we skip partition constraint validation? + * + * This basically returns if the partrel's existing constraints, which + * includes its check constraints and column-level NOT NULL constraints, + * imply the partition constraint as described in partConstraint. + */ +static bool +PartConstraintImpliedByRelConstraint(Relation partrel, List *partConstraint) +{ + List *existConstraint = NIL; + TupleConstr *constr = RelationGetDescr(partrel)->constr; + int num_check, + i; + + if (constr == NULL) + return false; + + num_check = constr->num_check; + + if (constr->has_not_null) + { + int natts = partrel->rd_att->natts; + + for (i = 1; i <= natts; i++) + { + Form_pg_attribute att = partrel->rd_att->attrs[i - 1]; + + if (att->attnotnull && !att->attisdropped) + { + NullTest *ntest = makeNode(NullTest); + + ntest->arg = (Expr *) makeVar(1, + i, + att->atttypid, + att->atttypmod, + att->attcollation, + 0); + ntest->nulltesttype = IS_NOT_NULL; + + /* + * argisrow=false is correct even for a composite column, + * because attnotnull does not represent a SQL-spec IS NOT + * NULL test in such a case, just IS DISTINCT FROM NULL. + */ + ntest->argisrow = false; + ntest->location = -1; + existConstraint = lappend(existConstraint, ntest); + } + } + } + + for (i = 0; i < num_check; i++) + { + Node *cexpr; + + /* + * If this constraint hasn't been fully validated yet, we must + * ignore it here. + */ + if (!constr->check[i].ccvalid) + continue; + + cexpr = stringToNode(constr->check[i].ccbin); + + /* + * Run each expression through const-simplification and + * canonicalization. It is necessary, because we will be + * comparing it to similarly-processed qual clauses, and may fail + * to detect valid matches without this. + */ + cexpr = eval_const_expressions(NULL, cexpr); + cexpr = (Node *) canonicalize_qual((Expr *) cexpr); + + existConstraint = list_concat(existConstraint, + make_ands_implicit((Expr *) cexpr)); + } + + existConstraint = list_make1(make_ands_explicit(existConstraint)); + + /* And away we go ... */ + if (predicate_implied_by(partConstraint, existConstraint, true)) + return true; + + /* Tough luck. */ + return false; +} + +/* * ALTER TABLE DETACH PARTITION * * Return the address of the relation that is no longer a partition of rel. diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 3ec5080fd6..03571f0e7c 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -3392,6 +3392,18 @@ SELECT tableoid::regclass, a, b FROM part_7 order by a; ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7); ERROR: partition constraint is violated by some row +-- If the partitioned table being attached does not have a constraint that +-- would allow validation scan to be skipped, but an individual partition +-- does, then the partition's validation scan is skipped. Note that the +-- following leaf partition only allows rows that have a = 7 (and b = 'b' but +-- that's irrelevant). +CREATE TABLE part_7_b PARTITION OF part_7 ( + CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 7) +) FOR VALUES IN ('b'); +-- The faulting row in part_7_a_null will still cause the command to fail +ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7); +INFO: partition constraint for table "part_7_b" is implied by existing constraints +ERROR: partition constraint is violated by some row -- check that the table being attached is not already a partition ALTER TABLE list_parted2 ATTACH PARTITION part_2 FOR VALUES IN (2); ERROR: "part_2" is already a partition diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index e0b7b37278..7e270b77ca 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -2216,6 +2216,17 @@ INSERT INTO part_7 (a, b) VALUES (8, null), (9, 'a'); SELECT tableoid::regclass, a, b FROM part_7 order by a; ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7); +-- If the partitioned table being attached does not have a constraint that +-- would allow validation scan to be skipped, but an individual partition +-- does, then the partition's validation scan is skipped. Note that the +-- following leaf partition only allows rows that have a = 7 (and b = 'b' but +-- that's irrelevant). +CREATE TABLE part_7_b PARTITION OF part_7 ( + CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 7) +) FOR VALUES IN ('b'); +-- The faulting row in part_7_a_null will still cause the command to fail +ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7); + -- check that the table being attached is not already a partition ALTER TABLE list_parted2 ATTACH PARTITION part_2 FOR VALUES IN (2); -- 2.11.0