On 2017/09/16 1:57, Amit Langote wrote:
> On Sat, Sep 16, 2017 at 12:59 AM, Robert Haas <robertmh...@gmail.com> wrote:
>> I believe the intended advantage of the current system is that if you
>> specify multiple operations in a single ALTER TABLE command, you only
>> do one scan rather than having a second scan per operation. If that's
>> currently working, we probably don't want to make it stop working.
>
> OK.
>
> How about squash Jeevan's and my patch, so both
> check_default_allows_bound() and ValidatePartitionConstraints() know
> to scan default partition's children and there won't be any surprises
> in the regression test output as you found after applying just the
> Jeevan's patch. Unfortunately, I'm not able to post such a patch
> right now.
I guess we don't need to squash, as they could be seen as implementing
different features. Reordering the patches helps though. So, apply them
in this order:
1. My patch to teach ValidatePartitionConstraints() to skip scanning
a partition's own partitions, which optimizes ATTACH PARTITION
command's partition constraint validation scan (this also covers the
case of scanning the default partition to validate its updated
constraint when attaching a new partition)
2. Jeevan's patch to teach check_default_allows_bound() to skip scanning
the default partition's own partitions, which covers the case of
scanning the default partition to validate its updated constraint when
adding a new partition using CREATE TABLE
Attached 0001 and 0002 are ordered that way.
In addition to implementing the features mentioned in 1 and 2 above, the
patches also modify the INFO message to mention "updated partition
constraint for default partition \"%s\"", instead of "partition constraint
for table \"%s\"", when the default partition is involved. That's so that
it's consistent with the error message that would be emitted by either
check_default_allows_bound() or ATRewriteTable() when the scan finds a row
that violates updated default partition constraint, viz. the following:
"updated partition constraint for default partition \"%s\" would be
violated by some row"
Thoughts?
Thanks,
Amit
From 166cc082b9d652e186df4ef7b6c41976a22e55a8 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 7 Aug 2017 10:51:47 +0900
Subject: [PATCH 1/2] Teach ValidatePartitionConstraints 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.
Idea by Robert Haas.
---
src/backend/commands/tablecmds.c | 26 +++++++++++++++++++++++---
src/test/regress/expected/alter_table.out | 17 +++++++++++++++--
src/test/regress/sql/alter_table.sql | 10 ++++++++++
3 files changed, 48 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 563bcda30c..2d4dcd7556 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13635,9 +13635,14 @@ ValidatePartitionConstraints(List **wqueue, Relation
scanrel,
*/
if (PartConstraintImpliedByRelConstraint(scanrel, partConstraint))
{
- ereport(INFO,
- (errmsg("partition constraint for table \"%s\"
is implied by existing constraints",
-
RelationGetRelationName(scanrel))));
+ if (!validate_default)
+ ereport(INFO,
+ (errmsg("partition constraint for table
\"%s\" is implied by existing constraints",
+
RelationGetRelationName(scanrel))));
+ else
+ ereport(INFO,
+ (errmsg("updated partition constraint
for default partition \"%s\" is implied by existing constraints",
+
RelationGetRelationName(scanrel))));
return;
}
@@ -13678,6 +13683,21 @@ ValidatePartitionConstraints(List **wqueue, Relation
scanrel,
/* There can never be a whole-row reference here */
if (found_whole_row)
elog(ERROR, "unexpected whole-row reference
found in partition key");
+
+ /* Can we skip scanning this part_rel? */
+ if (PartConstraintImpliedByRelConstraint(part_rel,
my_partconstr))
+ {
+ if (!validate_default)
+ ereport(INFO,
+ (errmsg("partition
constraint for table \"%s\" is implied by existing constraints",
+
RelationGetRelationName(part_rel))));
+ else
+ ereport(INFO,
+ (errmsg("updated
partition constraint for default partition \"%s\" is implied by existing
constraints",
+
RelationGetRelationName(part_rel))));
+ heap_close(part_rel, NoLock);
+ continue;
+ }
}
/* Grab a work queue entry. */
diff --git a/src/test/regress/expected/alter_table.out
b/src/test/regress/expected/alter_table.out
index 0478a8ac60..fc644c12ae 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3436,7 +3436,7 @@ ALTER TABLE part_7 ATTACH PARTITION part_7_a_null FOR
VALUES IN ('a', null);
INFO: partition constraint for table "part_7_a_null" is implied by existing
constraints
ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7);
INFO: partition constraint for table "part_7" is implied by existing
constraints
-INFO: partition constraint for table "list_parted2_def" is implied by
existing constraints
+INFO: updated partition constraint for default partition "list_parted2_def"
is implied by existing constraints
-- Same example, but check this time that the constraint correctly detects
-- violating rows
ALTER TABLE list_parted2 DETACH PARTITION part_7;
@@ -3450,7 +3450,7 @@ SELECT tableoid::regclass, a, b FROM part_7 order by a;
(2 rows)
ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7);
-INFO: partition constraint for table "list_parted2_def" is implied by
existing constraints
+INFO: updated partition constraint for default partition "list_parted2_def"
is implied by existing constraints
ERROR: partition constraint is violated by some row
-- check that leaf partitions of default partition are scanned when
-- attaching a partitioned table.
@@ -3464,6 +3464,19 @@ ERROR: updated partition constraint for default
partition would be violated by
-- should be ok after deleting the bad row
DELETE FROM part5_def_p1 WHERE b = 'y';
ALTER TABLE part_5 ATTACH PARTITION part5_p1 FOR VALUES IN ('y');
+-- 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
+INFO: updated partition constraint for default partition "list_parted2_def"
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 37cca72620..d296314ca9 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2277,6 +2277,16 @@ ALTER TABLE part_5 ATTACH PARTITION part5_p1 FOR VALUES
IN ('y');
-- should be ok after deleting the bad row
DELETE FROM part5_def_p1 WHERE b = 'y';
ALTER TABLE part_5 ATTACH PARTITION part5_p1 FOR VALUES IN ('y');
+-- 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
From 619fa7dc53bea151201ef5d5893c4df039a073a1 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Fri, 15 Sep 2017 14:40:15 +0900
Subject: [PATCH 2/2] Skip scanning default partition's child tables if
possible
Optimize check_default_allows_bound() such that the default
partition children constraints are checked against new partition
constraints for implication and avoid scan of the child of which
existing constraints are implied by new default partition
constraints. Also, added testcase for the same.
Jeevan Ladhe.
---
src/backend/catalog/partition.c | 16 +++++++++++++++-
src/test/regress/expected/alter_table.out | 31 +++++++++++++++++++++++++++----
src/test/regress/sql/alter_table.sql | 9 +++++++++
3 files changed, 51 insertions(+), 5 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 1ab6dba7ae..25ef0f39d2 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -920,7 +920,7 @@ check_default_allows_bound(Relation parent, Relation
default_rel,
if (PartConstraintImpliedByRelConstraint(default_rel,
def_part_constraints))
{
ereport(INFO,
- (errmsg("partition constraint for table \"%s\"
is implied by existing constraints",
+ (errmsg("updated partition constraint for
default partition \"%s\" is implied by existing constraints",
RelationGetRelationName(default_rel))));
return;
}
@@ -953,7 +953,21 @@ check_default_allows_bound(Relation parent, Relation
default_rel,
/* Lock already taken above. */
if (part_relid != RelationGetRelid(default_rel))
+ {
part_rel = heap_open(part_relid, NoLock);
+
+ /* Can we skip scanning this part_rel? */
+ if (PartConstraintImpliedByRelConstraint(part_rel,
+
def_part_constraints))
+ {
+ ereport(INFO,
+ (errmsg("updated partition
constraint for default partition \"%s\" is implied by existing constraints",
+
RelationGetRelationName(part_rel))));
+
+ heap_close(part_rel, NoLock);
+ continue;
+ }
+ }
else
part_rel = default_rel;
diff --git a/src/test/regress/expected/alter_table.out
b/src/test/regress/expected/alter_table.out
index fc644c12ae..77b05ee5e5 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3345,7 +3345,19 @@ INFO: partition constraint for table "part_3_4" is
implied by existing constrai
-- check if default partition scan skipped
ALTER TABLE list_parted2_def ADD CONSTRAINT check_a CHECK (a IN (5, 6));
CREATE TABLE part_55_66 PARTITION OF list_parted2 FOR VALUES IN (55, 66);
-INFO: partition constraint for table "list_parted2_def" is implied by
existing constraints
+INFO: updated partition constraint for default partition "list_parted2_def"
is implied by existing constraints
+-- check if default partition child relation scan skipped
+DROP TABLE list_parted2_def;
+CREATE TABLE list_parted2_def PARTITION OF list_parted2 DEFAULT PARTITION BY
RANGE (a);
+CREATE TABLE list_parted2_def_p1 PARTITION OF list_parted2_def FOR VALUES FROM
(21) TO (30);
+CREATE TABLE list_parted2_def_p2 PARTITION OF list_parted2_def FOR VALUES FROM
(31) TO (40);
+ALTER TABLE list_parted2_def_p1 ADD CONSTRAINT check_a CHECK (a IN (21, 22));
+ALTER TABLE list_parted2_def_p2 ADD CONSTRAINT check_a CHECK (a IN (31, 32));
+CREATE TABLE part_9 PARTITION OF list_parted2 FOR VALUES IN (9);
+INFO: updated partition constraint for default partition
"list_parted2_def_p1" is implied by existing constraints
+INFO: updated partition constraint for default partition
"list_parted2_def_p2" is implied by existing constraints
+CREATE TABLE part_31 PARTITION OF list_parted2 FOR VALUES IN (31);
+INFO: updated partition constraint for default partition
"list_parted2_def_p1" is implied by existing constraints
-- check validation when attaching range partitions
CREATE TABLE range_parted (
a int,
@@ -3393,18 +3405,24 @@ CREATE TABLE part_5 (
CREATE TABLE part_5_a PARTITION OF part_5 FOR VALUES IN ('a');
INSERT INTO part_5_a (a, b) VALUES (6, 'a');
ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5);
+INFO: updated partition constraint for default partition
"list_parted2_def_p1" is implied by existing constraints
+INFO: updated partition constraint for default partition
"list_parted2_def_p2" is implied by existing constraints
ERROR: partition constraint is violated by some row
-- delete the faulting row and also add a constraint to skip the scan
DELETE FROM part_5_a WHERE a NOT IN (3);
ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 5);
ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5);
INFO: partition constraint for table "part_5" is implied by existing
constraints
+INFO: updated partition constraint for default partition
"list_parted2_def_p1" is implied by existing constraints
+INFO: updated partition constraint for default partition
"list_parted2_def_p2" is implied by existing constraints
ALTER TABLE list_parted2 DETACH PARTITION part_5;
ALTER TABLE part_5 DROP CONSTRAINT check_a;
-- scan should again be skipped, even though NOT NULL is now a column property
ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IN (5)), ALTER a SET NOT
NULL;
ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5);
INFO: partition constraint for table "part_5" is implied by existing
constraints
+INFO: updated partition constraint for default partition
"list_parted2_def_p1" is implied by existing constraints
+INFO: updated partition constraint for default partition
"list_parted2_def_p2" is implied by existing constraints
-- Check the case where attnos of the partitioning columns in the table being
-- attached differs from the parent. It should not affect the constraint-
-- checking logic that allows to skip the scan.
@@ -3416,6 +3434,8 @@ CREATE TABLE part_6 (
ALTER TABLE part_6 DROP c;
ALTER TABLE list_parted2 ATTACH PARTITION part_6 FOR VALUES IN (6);
INFO: partition constraint for table "part_6" is implied by existing
constraints
+INFO: updated partition constraint for default partition
"list_parted2_def_p1" is implied by existing constraints
+INFO: updated partition constraint for default partition
"list_parted2_def_p2" is implied by existing constraints
-- Similar to above, but the table being attached is a partitioned table
-- whose partition has still different attnos for the root partitioning
-- columns.
@@ -3436,7 +3456,8 @@ ALTER TABLE part_7 ATTACH PARTITION part_7_a_null FOR
VALUES IN ('a', null);
INFO: partition constraint for table "part_7_a_null" is implied by existing
constraints
ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7);
INFO: partition constraint for table "part_7" is implied by existing
constraints
-INFO: updated partition constraint for default partition "list_parted2_def"
is implied by existing constraints
+INFO: updated partition constraint for default partition
"list_parted2_def_p1" is implied by existing constraints
+INFO: updated partition constraint for default partition
"list_parted2_def_p2" is implied by existing constraints
-- Same example, but check this time that the constraint correctly detects
-- violating rows
ALTER TABLE list_parted2 DETACH PARTITION part_7;
@@ -3450,7 +3471,8 @@ SELECT tableoid::regclass, a, b FROM part_7 order by a;
(2 rows)
ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7);
-INFO: updated partition constraint for default partition "list_parted2_def"
is implied by existing constraints
+INFO: updated partition constraint for default partition
"list_parted2_def_p1" is implied by existing constraints
+INFO: updated partition constraint for default partition
"list_parted2_def_p2" is implied by existing constraints
ERROR: partition constraint is violated by some row
-- check that leaf partitions of default partition are scanned when
-- attaching a partitioned table.
@@ -3475,7 +3497,8 @@ CREATE TABLE part_7_b PARTITION OF part_7 (
-- 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
-INFO: updated partition constraint for default partition "list_parted2_def"
is implied by existing constraints
+INFO: updated partition constraint for default partition
"list_parted2_def_p1" is implied by existing constraints
+INFO: updated partition constraint for default partition
"list_parted2_def_p2" 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);
diff --git a/src/test/regress/sql/alter_table.sql
b/src/test/regress/sql/alter_table.sql
index d296314ca9..5f92486cb2 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2163,6 +2163,15 @@ ALTER TABLE list_parted2 ATTACH PARTITION part_3_4 FOR
VALUES IN (3, 4);
-- check if default partition scan skipped
ALTER TABLE list_parted2_def ADD CONSTRAINT check_a CHECK (a IN (5, 6));
CREATE TABLE part_55_66 PARTITION OF list_parted2 FOR VALUES IN (55, 66);
+-- check if default partition child relation scan skipped
+DROP TABLE list_parted2_def;
+CREATE TABLE list_parted2_def PARTITION OF list_parted2 DEFAULT PARTITION BY
RANGE (a);
+CREATE TABLE list_parted2_def_p1 PARTITION OF list_parted2_def FOR VALUES FROM
(21) TO (30);
+CREATE TABLE list_parted2_def_p2 PARTITION OF list_parted2_def FOR VALUES FROM
(31) TO (40);
+ALTER TABLE list_parted2_def_p1 ADD CONSTRAINT check_a CHECK (a IN (21, 22));
+ALTER TABLE list_parted2_def_p2 ADD CONSTRAINT check_a CHECK (a IN (31, 32));
+CREATE TABLE part_9 PARTITION OF list_parted2 FOR VALUES IN (9);
+CREATE TABLE part_31 PARTITION OF list_parted2 FOR VALUES IN (31);
-- check validation when attaching range partitions
CREATE TABLE range_parted (
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
