On 2017/06/14 5:36, Robert Haas wrote:
> On Tue, Jun 13, 2017 at 10:24 AM, Robert Haas <[email protected]> wrote:
>> I think that's going to come as an unpleasant surprise to more than
>> one user. I'm not sure exactly how we need to restructure things here
>> so that this works properly, and maybe modifying
>> predicate_implied_by() isn't the right thing at all; for instance,
>> there's also predicate_refuted_by(), which maybe could be used in some
>> way (inject NOT?). But I don't much like the idea that you copy and
>> paste the partitioning constraint into a CHECK constraint and that
>> doesn't work. That's not cool.
>
> OK, I think I see the problem here. predicate_implied_by() and
> predicate_refuted_by() differ in what they assume about the predicate
> evaluating to NULL, but both of them assume that if the clause
> evaluates to NULL, that's equivalent to false. So there's actually no
> option to get the behavior we want here, which is to treat both
> operands using CHECK-semantics (null is true) rather than
> WHERE-semantics (null is false).
>
> Given that, Ashutosh's proposal of passing an additional flag to
> predicate_implied_by() seems like the best option. Here's a patch
> implementing that.
I tried this patch and it seems to work correctly.
Some comments on the patch itself:
The following was perhaps included for debugging?
+#include "nodes/print.h"
I think the following sentence in a comment near the affected code in
ATExecAttachPartition() needs to be removed.
*
* There is a case in which we cannot rely on just the result of the
* proof.
We no longer need the Bitmapset not_null_attrs. So, the line declaring it
and the following line can be removed:
not_null_attrs = bms_add_member(not_null_attrs, i);
I thought I would make these changes myself and send the v2, but realized
that you might be updating it yourself based on Tom's comments, so didn't.
By the way, I mentioned an existing problem in one of the earlier emails
on this thread about differing attribute numbers in the table being
attached causing predicate_implied_by() to give up due to structural
inequality of Vars. To clarify: table's check constraints will bear the
table's attribute numbers whereas the partition constraint generated using
get_qual_for_partbound() (the predicate) bears the parent's attribute
numbers. That results in Var arguments of the expressions passed to
predicate_implied_by() not matching and causing the latter to return
failure prematurely. Attached find a patch to fix that that applies on
top of your patch (added a test too).
Thanks,
Amit
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3ca23c8ef5..7fa054f56a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13416,6 +13416,7 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
List *childrels;
TupleConstr *attachRel_constr;
List *partConstraint,
+ *partConstraintOrig,
*existConstraint;
SysScanDesc scan;
ScanKeyData skey;
@@ -13581,6 +13582,15 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
partConstraint = list_make1(make_ands_explicit(partConstraint));
/*
+ * Adjust the generated constraint to match this partition's attribute
+ * numbers. Save the original to be used later if we decide to proceed
+ * with the validation scan after all.
+ */
+ partConstraintOrig = copyObject(partConstraint);
+ partConstraint = map_partition_varattnos(partConstraint, 1, attachRel,
+
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
@@ -13715,7 +13725,7 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
tab = ATGetQueueEntry(wqueue, part_rel);
/* Adjust constraint to match this partition */
- constr = linitial(partConstraint);
+ constr = linitial(partConstraintOrig);
tab->partition_constraint = (Expr *)
map_partition_varattnos((List *) constr, 1,
part_rel, rel);
diff --git a/src/test/regress/expected/alter_table.out
b/src/test/regress/expected/alter_table.out
index 13d6a4b747..ec67b4cc73 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3347,6 +3347,16 @@ ALTER TABLE part_5 DROP CONSTRAINT check_a;
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
+-- scan will be skipped, even though partition column attribute numbers differ
+-- from the parent (provided the appropriate check constraint is present)
+CREATE TABLE part_6 (
+ c int,
+ LIKE list_parted2,
+ CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 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
-- 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 5dd1402ea6..5779ad1161 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2178,6 +2178,16 @@ ALTER TABLE part_5 DROP CONSTRAINT check_a;
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);
+-- scan will be skipped, even though partition column attribute numbers differ
+-- from the parent (provided the appropriate check constraint is present)
+CREATE TABLE part_6 (
+ c int,
+ LIKE list_parted2,
+ CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 6)
+);
+ALTER TABLE part_6 DROP c;
+ALTER TABLE list_parted2 ATTACH PARTITION part_6 FOR VALUES IN (6);
+
-- check that the table being attached is not already a partition
ALTER TABLE list_parted2 ATTACH PARTITION part_2 FOR VALUES IN (2);
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
