On 2017/06/08 18:43, Amit Langote wrote:
> On 2017/06/08 1:44, Robert Haas wrote:
>> On Wed, Jun 7, 2017 at 7:47 AM, Ashutosh Bapat
>> <ashutosh.ba...@enterprisedb.com> wrote:
>>> In ATExecAttachPartition() there's following code
>>>
>>> 13715 partnatts = get_partition_natts(key);
>>> 13716 for (i = 0; i < partnatts; i++)
>>> 13717 {
>>> 13718 AttrNumber partattno;
>>> 13719
>>> 13720 partattno = get_partition_col_attnum(key, i);
>>> 13721
>>> 13722 /* If partition key is an expression, must not skip
>>> validation */
>>> 13723 if (!partition_accepts_null &&
>>> 13724 (partattno == 0 ||
>>> 13725 !bms_is_member(partattno, not_null_attrs)))
>>> 13726 skip_validate = false;
>>> 13727 }
>>>
>>> partattno obtained from the partition key is the attribute number of
>>> the partitioned table but not_null_attrs contains the attribute
>>> numbers of attributes of the table being attached which have NOT NULL
>>> constraint on them. But the attribute numbers of partitioned table and
>>> the table being attached may not agree i.e. partition key attribute in
>>> partitioned table may have a different position in the table being
>>> attached. So, this code looks buggy. Probably we don't have a test
>>> which tests this code with different attribute order between
>>> partitioned table and the table being attached. I didn't get time to
>>> actually construct a testcase and test it.
>
> There seem to be couple of bugs here:
>
> 1. When partition's key attributes differ in ordering from the parent,
> predicate_implied_by() will give up due to structural inequality of
> Vars in the expressions. By fixing this, we can get it to return
> 'true' when it's really so.
>
> 2. As you said, we store partition's attribute numbers in the
> not_null_attrs bitmap, but then check partattno (which is the parent's
> attribute number which might differ) against the bitmap, which seems
> like it might produce incorrect result. If, for example,
> predicate_implied_by() set skip_validate to true, and the above code
> failed to set skip_validate to false where it should have, then we
> would wrongly end up skipping the scan. That is, rows in the partition
> will contain null values whereas the partition constraint does not
> allow it. It's hard to reproduce this currently, because with
> different ordering of attributes, predicate_refute_by() never returns
> true (as mentioned in 1 above), so skip_validate does not need to be
> set to false again.
>
> Consider this example:
>
> create table p (a int, b char) partition by list (a);
>
> create table p1 (b char not null, a int check (a in (1)));
> insert into p1 values ('b', null);
>
> Note that not_null_attrs for p1 will contain 1 corresponding to column b,
> which matches key attribute of the parent, that is 1, corresponding to
> column a. Hence we end up wrongly concluding that p1's partition key
> column does not allow nulls.
[ ... ]
> I am working on a patch to fix the above mentioned issues and will post
> the same no later than Friday.
And here is the patch.
Thanks,
Amit
From fef05d64bfad370f79e9fc305eee3d430a8f5263 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 8 Jun 2017 14:01:34 +0900
Subject: [PATCH] Fixes around partition constraint handling when attaching
Failure to map attribute numbers in the partition key expressions to
to partition's would cause predicate_implied_by() to unnecessarily
return 'false', in turn causing the failure to skip the validation
scan.
Conversely, failure to map partition's NOT NULL column's attribute
numbers to parent's might cause incorrect conclusion about which
columns of the partition being attached must have NOT NULL constraint
defined on them.
Rearrange code and comments a little around this area to make things
clearer.
---
src/backend/commands/tablecmds.c | 109 ++++++++++++++----------------
src/test/regress/expected/alter_table.out | 32 +++++++++
src/test/regress/sql/alter_table.sql | 31 +++++++++
3 files changed, 113 insertions(+), 59 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b61fda9909..481bc97155 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13409,6 +13409,7 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
List *childrels;
TupleConstr *attachRel_constr;
List *partConstraint,
+ *partConstraintOrig,
*existConstraint;
SysScanDesc scan;
ScanKeyData skey;
@@ -13574,14 +13575,24 @@ 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
- * 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.
+ * only the table's check constraints in the list of "restrictions"
passed
+ * to predicate_implied_by(). If the partition constraint requires that
+ * the partition key not be NULL, it is checked by looking for explicit
+ * NOT NULL constraints on the partition key columns. If any partition
+ * key is an expression for which there cannot be a NOT NULL constraint,
+ * we simply give up on skipping the scan.
*/
attachRel_constr = tupleDesc->constr;
existConstraint = NIL;
@@ -13589,42 +13600,36 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
{
int num_check = attachRel_constr->num_check;
int i;
+ AttrNumber *parent_attnos;
Bitmapset *not_null_attrs = NULL;
List *part_constr;
ListCell *lc;
- bool partition_accepts_null = true;
+ bool partition_accepts_null;
int partnatts;
if (attachRel_constr->has_not_null)
{
int natts =
attachRel->rd_att->natts;
+ parent_attnos =
+
convert_tuples_by_name_map(RelationGetDescr(rel),
+
RelationGetDescr(attachRel),
+
gettext_noop("could not convert row type"));
for (i = 1; i <= natts; i++)
{
Form_pg_attribute att =
attachRel->rd_att->attrs[i - 1];
+ /*
+ * We check below if this NOT NULL attribute in
the partition
+ * is a key column. Instead of storing the
partition's own
+ * attribute number however, we need to store
the parent's
+ * corresponding attribute number, because we
will be comparing
+ * against the partition key which contains
parent's attribute
+ * numbers.
+ */
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);
- not_null_attrs =
bms_add_member(not_null_attrs, i);
- }
+ not_null_attrs =
bms_add_member(not_null_attrs,
+
parent_attnos[i - 1]);
}
}
@@ -13661,30 +13666,12 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
skip_validate = true;
/*
- * We choose to err on the safer side, i.e., give up on
skipping the
- * validation scan, if the partition key column doesn't have
the NOT
- * NULL constraint and the table is to become a list partition
that
- * does not accept nulls. In this case, the partition predicate
- * (partConstraint) does include an 'key IS NOT NULL'
expression,
- * however, because of the way
predicate_implied_by_simple_clause() is
- * designed to handle IS NOT NULL predicates in the absence of
a IS
- * NOT NULL clause, we cannot rely on just the above proof.
- *
- * That is not an issue in case of a range partition, because
if there
- * were no NOT NULL constraint defined on the key columns, an
error
- * would be thrown before we get here anyway. That is not true,
- * however, if any of the partition keys is an expression,
which is
- * handled below.
+ * First determine if the partition accepts NULL; it doesn't if
there
+ * is an IS NOT NULL expression in the partition constraint.
*/
part_constr = linitial(partConstraint);
part_constr = make_ands_implicit((Expr *) part_constr);
-
- /*
- * part_constr contains an IS NOT NULL expression, if this is a
list
- * partition that does not accept nulls (in fact, also if this
is a
- * range partition and some partition key is an expression, but
we
- * never skip validation in that case anyway; see below)
- */
+ partition_accepts_null = true;
foreach(lc, part_constr)
{
Node *expr = lfirst(lc);
@@ -13697,18 +13684,22 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
}
}
- partnatts = get_partition_natts(key);
- for (i = 0; i < partnatts; i++)
+ if (!partition_accepts_null)
{
- AttrNumber partattno;
-
- partattno = get_partition_col_attnum(key, i);
+ /* Check that none of the partition keys allows null
values. */
+ partnatts = get_partition_natts(key);
+ for (i = 0; i < partnatts; i++)
+ {
+ AttrNumber partattno =
get_partition_col_attnum(key, i);
- /* If partition key is an expression, must not skip
validation */
- if (!partition_accepts_null &&
- (partattno == 0 ||
- !bms_is_member(partattno, not_null_attrs)))
- skip_validate = false;
+ /* We don't know much about the expression
keys. */
+ if (partattno == 0 ||
+ !bms_is_member(partattno,
not_null_attrs))
+ {
+ skip_validate = false;
+ break;
+ }
+ }
}
}
@@ -13763,7 +13754,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 8aadbb88a3..222d7e13d3 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3301,6 +3301,17 @@ ALTER TABLE list_parted2 DETACH PARTITION part_3_4;
ALTER TABLE part_3_4 ALTER a SET NOT NULL;
ALTER TABLE list_parted2 ATTACH PARTITION part_3_4 FOR VALUES IN (3, 4);
INFO: partition constraint for table "part_3_4" is implied by existing
constraints
+-- check that able to handle different ordering of attributes when analyzing
+-- table's constraint to skip the validation scan
+CREATE TABLE part_6 (
+ c int,
+ b char,
+ a int NOT NULL,
+ CONSTRAINT check_a CHECK (a IN (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 validation when attaching range partitions
CREATE TABLE range_parted (
a int,
@@ -3326,6 +3337,27 @@ CREATE TABLE part2 (
);
ALTER TABLE range_parted ATTACH PARTITION part2 FOR VALUES FROM (1, 10) TO (1,
20);
INFO: partition constraint for table "part2" is implied by existing
constraints
+-- check that able to handle different ordering of attributes when analyzing
+-- table's constraint to skip the validation scan
+CREATE TABLE part3 (
+ c int,
+ a int NOT NULL CHECK (a = 1),
+ b int NOT NULL CHECK (b >= 21 AND b < 25)
+);
+ALTER TABLE part3 DROP c;
+ALTER TABLE range_parted ATTACH PARTITION part3 FOR VALUES FROM (1, 20) TO (1,
30);
+INFO: partition constraint for table "part3" is implied by existing
constraints
+-- check that null values in the range partition key are correctly
+-- reported
+CREATE TABLE part4 (
+ c int,
+ a int CHECK (a = 1),
+ b int CHECK (b >= 21 AND b < 25)
+);
+ALTER TABLE part4 DROP c;
+INSERT INTO part4 VALUES (null, null);
+ALTER TABLE range_parted ATTACH PARTITION part4 FOR VALUES FROM (1, 30) TO (1,
40);
+ERROR: partition constraint is violated by some row
-- check that leaf partitions are scanned when attaching a partitioned
-- table
CREATE TABLE part_5 (
diff --git a/src/test/regress/sql/alter_table.sql
b/src/test/regress/sql/alter_table.sql
index c41b48785b..16edce89f8 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2128,6 +2128,16 @@ ALTER TABLE list_parted2 DETACH PARTITION part_3_4;
ALTER TABLE part_3_4 ALTER a SET NOT NULL;
ALTER TABLE list_parted2 ATTACH PARTITION part_3_4 FOR VALUES IN (3, 4);
+-- check that able to handle different ordering of attributes when analyzing
+-- table's constraint to skip the validation scan
+CREATE TABLE part_6 (
+ c int,
+ b char,
+ a int NOT NULL,
+ CONSTRAINT check_a CHECK (a IN (6))
+);
+ALTER TABLE part_6 DROP c;
+ALTER TABLE list_parted2 ATTACH PARTITION part_6 FOR VALUES IN (6);
-- check validation when attaching range partitions
CREATE TABLE range_parted (
@@ -2156,6 +2166,27 @@ CREATE TABLE part2 (
);
ALTER TABLE range_parted ATTACH PARTITION part2 FOR VALUES FROM (1, 10) TO (1,
20);
+-- check that able to handle different ordering of attributes when analyzing
+-- table's constraint to skip the validation scan
+CREATE TABLE part3 (
+ c int,
+ a int NOT NULL CHECK (a = 1),
+ b int NOT NULL CHECK (b >= 21 AND b < 25)
+);
+ALTER TABLE part3 DROP c;
+ALTER TABLE range_parted ATTACH PARTITION part3 FOR VALUES FROM (1, 20) TO (1,
30);
+
+-- check that null values in the range partition key are correctly
+-- reported
+CREATE TABLE part4 (
+ c int,
+ a int CHECK (a = 1),
+ b int CHECK (b >= 21 AND b < 25)
+);
+ALTER TABLE part4 DROP c;
+INSERT INTO part4 VALUES (null, null);
+ALTER TABLE range_parted ATTACH PARTITION part4 FOR VALUES FROM (1, 30) TO (1,
40);
+
-- check that leaf partitions are scanned when attaching a partitioned
-- table
CREATE TABLE part_5 (
--
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
