Re: [HACKERS] Optimise default partition scanning while adding new partition
On 2017/10/13 4:18, Robert Haas wrote: > On Thu, Oct 5, 2017 at 9:29 PM, Amit Langote >wrote: >> Attached a patch to modify the INFO messages in check_default_allows_bound. > > Committed. However, I didn't see a reason to adopt the comment change > you proposed, so I left that part out. Okay, thanks. Regards, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimise default partition scanning while adding new partition
On Thu, Oct 5, 2017 at 9:29 PM, Amit Langotewrote: > Attached a patch to modify the INFO messages in check_default_allows_bound. Committed. However, I didn't see a reason to adopt the comment change you proposed, so I left that part out. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimise default partition scanning while adding new partition
On 2017/10/06 2:25, Robert Haas wrote: > On Tue, Sep 26, 2017 at 4:27 AM, Amit Langote wrote: >> 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. > > OK, I pushed all of this, spread out over 3 commits. I reworked the > test cases not to be entangled with the existing test cases, and also > to test both of these highly-related features together. Hopefully you > like the result. Thanks for committing. I noticed that 6476b26115f3 doesn't use the same INFO message as 14f67a8ee28. You can notice in the following example that the message emitted (that the default partition scan is skipped) is different when a new partition is added with CREATE TABLE and with ATTACH PARTITION, respectively. create table foo (a int, b char) partition by list (a); -- default partition create table food partition of foo default partition by list (b); -- default partition's partition with the check constraint create table fooda partition of food (check (not (a is not null and a = 1 and a = 2))) for values in ('a'); create table foo1 partition of foo for values in (1); INFO: partition constraint for table "fooda" is implied by existing constraints CREATE TABLE create table foo2 (like foo); alter table foo attach partition foo2 for values in (2); INFO: updated partition constraint for default partition "fooda" is implied by existing constraints ALTER TABLE That's because it appears that you applied Jeevan's original patch. Revised version of his patch that I had last posted contained the new message. Actually the revised patch had not only addressed the case where the default partition's child's scan is skipped, but also the case where the scan of default partition itself is skipped. As things stand now: alter table food add constraint skip_check check (not (a is not null and (a = any (array[1, 2]; create table foo1 partition of foo for values in (1); INFO: partition constraint for table "food" is implied by existing constraints CREATE TABLE create table foo2 (like foo); CREATE TABLE alter table foo attach partition foo2 for values in (2); INFO: updated partition constraint for default partition "food" is implied by existing constraints ALTER TABLE Attached a patch to modify the INFO messages in check_default_allows_bound. Thanks, Amit From 1bd3b03bcd1f8af6cec3a22c40e96c9cde46e705 Mon Sep 17 00:00:00 2001 From: amitDate: Fri, 6 Oct 2017 10:23:24 +0900 Subject: [PATCH] Like c31e9d4bafd, but for check_default_allows_bound --- src/backend/catalog/partition.c | 10 +++--- src/test/regress/expected/alter_table.out | 4 ++-- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 3a8306a055..1459fba778 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; } @@ -956,16 +956,12 @@ check_default_allows_bound(Relation parent, Relation default_rel, { part_rel = heap_open(part_relid, NoLock); - /* -* If the partition constraints on default partition child imply -* that it will not contain any row that would belong to the new -* partition, we can avoid scanning the child table. -*/ + /* Can we skip scanning this part_rel? */ if (PartConstraintImpliedByRelConstraint(part_rel, def_part_constraints))
Re: [HACKERS] Optimise default partition scanning while adding new partition
On Tue, Sep 26, 2017 at 4:27 AM, Amit Langotewrote: > On 2017/09/16 1:57, Amit Langote wrote: >> On Sat, Sep 16, 2017 at 12:59 AM, Robert Haas 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. OK, I pushed all of this, spread out over 3 commits. I reworked the test cases not to be entangled with the existing test cases, and also to test both of these highly-related features together. Hopefully you like the result. Thanks for the patches and the analysis. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimise default partition scanning while adding new partition
On 2017/09/16 1:57, Amit Langote wrote: > On Sat, Sep 16, 2017 at 12:59 AM, Robert Haaswrote: >> 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 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", +
Re: [HACKERS] Optimise default partition scanning while adding new partition
On Sat, Sep 16, 2017 at 12:59 AM, Robert Haaswrote: > On Fri, Sep 15, 2017 at 2:00 AM, Amit Langote > wrote: >> I wonder if we should call check_default_allows_bound() from >> ATExecAttachPartition(), too, instead of validating updated default >> partition constraint using ValidatePartitionConstraints()? That is, call >> the latter only to validate the partition constraint of the table being >> attached and call check_default_allows_bound() to validate the updated >> default partition constraint. That way, INFO/ERROR messages related to >> default partition constraint are consistent across the board. > > 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. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimise default partition scanning while adding new partition
On Fri, Sep 15, 2017 at 2:00 AM, Amit Langotewrote: > I wonder if we should call check_default_allows_bound() from > ATExecAttachPartition(), too, instead of validating updated default > partition constraint using ValidatePartitionConstraints()? That is, call > the latter only to validate the partition constraint of the table being > attached and call check_default_allows_bound() to validate the updated > default partition constraint. That way, INFO/ERROR messages related to > default partition constraint are consistent across the board. 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimise default partition scanning while adding new partition
On 2017/09/15 0:59, Robert Haas wrote: > On Thu, Sep 14, 2017 at 4:03 AM, Jeevan Ladhe >wrote: >> Thanks Amit for reviewing. >>> Patch looks fine to me. By the way, why don't we just say "Can we skip >>> scanning part_rel?" in the comment before the newly added call to >>> PartConstraintImpliedByRelConstraint()? We don't need to repeat the >>> explanation of what it does at the every place we call it. >> >> I have changed the comment. >> PFA. > > I'm probably missing something stupid, but why does this happen? > > 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 > ERROR: partition constraint is violated by some row > > Based on the regression test changes made up higher in the file, I'd > expect that line to be replaced by two lines, one for > list_parted2_def_p1 and another for list_parted2_def_p2, because at > this point, list_parted2_def is a partitioned table with those two > children, and they seem to have appropriate constraints. > > list_parted2_def_p1 has > Check constraints: > "check_a" CHECK (a = ANY (ARRAY[21, 22])) > > list_parted2_def_p2 has > Check constraints: > "check_a" CHECK (a = ANY (ARRAY[31, 32])) > > Well, if a is 21 or 22 for the first, or 31 or 32 for the second, then > it's not 7. Or so I would think. ISTM, Jeevan's patch modifies check_default_allows_bound(), which only DefineRelation() calls as things stand today. IOW, it doesn't handle the ATTACH PARTITION case, so you're not seeing the lines for list_parted2_def_p1 and list_parted2_def_p2, because ATExecAttachPartition doesn't yet know how to skip the validation scan for default partition's children. I wonder if we should call check_default_allows_bound() from ATExecAttachPartition(), too, instead of validating updated default partition constraint using ValidatePartitionConstraints()? That is, call the latter only to validate the partition constraint of the table being attached and call check_default_allows_bound() to validate the updated default partition constraint. That way, INFO/ERROR messages related to default partition constraint are consistent across the board. I hacked that up in the attached 0001. The patch also modifies the INFO message that check_default_allows_bound() emits when the scan is skipped to make it apparent that a default partition is involved. (Sorry, this should've been a review comment I should've posted before the default partition patch was committed.) Then apply 0002, which is Jeevan's patch. With 0001 in place, Robert's complaint is taken care of. Then comes 0003, which is my patch to teach ValidatePartitionConstraints() to skip child table scans using their existing constraint. Thanks, Amit From 00765f2166c78902d0c92ed9d1a4dea033a50a12 Mon Sep 17 00:00:00 2001 From: amit Date: Fri, 15 Sep 2017 14:30:36 +0900 Subject: [PATCH 1/3] Teach ATExecAttachPartition to use check_default_allows_bound Currently it schedules validation scan of the default partition in the same way as it schedules the scan of the partition being attached. Instead, call check_default_allows_bound() to do the scanning, so the handling is symmetric with DefineRelation(). Also, update the INFO message in check_default_allows_bound(), so that it's clear from the message that the default partition is involved. --- src/backend/catalog/partition.c | 8 ++- src/backend/commands/tablecmds.c | 40 ++- src/test/regress/expected/alter_table.out | 12 +- 3 files changed, 20 insertions(+), 40 deletions(-) diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 1ab6dba7ae..027b98d850 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -912,15 +912,11 @@ check_default_allows_bound(Relation parent, Relation default_rel, def_part_constraints = get_proposed_default_constraint(new_part_constraints); - /* -* If the existing constraints on the default partition imply that it will -* not contain any row that would belong to the new partition, we can -* avoid scanning the default partition. -*/ + /* Can we skip scanning 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; } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 563bcda30c..ed4a2092b3 100644
Re: [HACKERS] Optimise default partition scanning while adding new partition
On Thu, Sep 14, 2017 at 4:03 AM, Jeevan Ladhewrote: > Thanks Amit for reviewing. >> Patch looks fine to me. By the way, why don't we just say "Can we skip >> scanning part_rel?" in the comment before the newly added call to >> PartConstraintImpliedByRelConstraint()? We don't need to repeat the >> explanation of what it does at the every place we call it. > > I have changed the comment. > PFA. I'm probably missing something stupid, but why does this happen? 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 ERROR: partition constraint is violated by some row Based on the regression test changes made up higher in the file, I'd expect that line to be replaced by two lines, one for list_parted2_def_p1 and another for list_parted2_def_p2, because at this point, list_parted2_def is a partitioned table with those two children, and they seem to have appropriate constraints. list_parted2_def_p1 has Check constraints: "check_a" CHECK (a = ANY (ARRAY[21, 22])) list_parted2_def_p2 has Check constraints: "check_a" CHECK (a = ANY (ARRAY[31, 32])) Well, if a is 21 or 22 for the first, or 31 or 32 for the second, then it's not 7. Or so I would think. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimise default partition scanning while adding new partition
Thanks Amit for reviewing. Patch looks fine to me. By the way, why don't we just say "Can we skip > scanning part_rel?" in the comment before the newly added call to > PartConstraintImpliedByRelConstraint()? We don't need to repeat the > explanation of what it does at the every place we call it. > I have changed the comment. PFA. Regards, Jeevan Ladhe 0001-Check-default-partitition-child-validation-scan-is.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimise default partition scanning while adding new partition
Hi Jeevan, On 2017/09/12 18:22, Jeevan Ladhe wrote: > Commit 6f6b99d1335be8ea1b74581fc489a97b109dd08a introduced default > partitioning support. This commit added a new function > check_default_allows_bound(), > which checks if there exists a row in the default partition that would > belong to > the new partition being added. If it finds one, it throws an error. Before > taking > the decision to scan the default partition, this function checks if there > are > existing constraints on default partition that would imply the new partition > constraints, if yes it skips scanning the default partition, otherwise it > scans the > default partition and its children(if any). But, while doing so the current > code > misses the fact that there can be constraints on the child of default > partition > such that they would imply the constraints of the new partition being added, > and hence individual child scan can also be skipped. > Attached is the patch which does this. > > This is previously discussed in default partitioning thread[1], and decision > was made that we can take this a separate patch rather than as a part of the > default partitioning support. Patch looks fine to me. By the way, why don't we just say "Can we skip scanning part_rel?" in the comment before the newly added call to PartConstraintImpliedByRelConstraint()? We don't need to repeat the explanation of what it does at the every place we call it. > Amit Langote has a similar patch[2] for scanning the children of a > partitioned > table which is being attached as partition of another partitioned table. I just posted my rebased patch there [1]. Thanks, Amit [1] https://www.postgresql.org/message-id/a83a0899-19f5-594c-9aac-3ba0f16989a1%40lab.ntt.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers