On 2017/05/16 4:29, Robert Haas wrote:
> On Mon, May 15, 2017 at 9:12 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
>> Can't we allow NULL to get inserted into the partition (leaf
>> partition) if the user uses the partition name in Insert statement?
>
> That would be terrible behavior - the behavior of tuple routing should
> match the enforced constraints.
>
>> For root partitions, I think for now giving an error is okay, but once
>> we have default partitions (Rahila's patch), we can route NULLS to
>> default partition.
>
> Yeah, that's exactly why I think we should make the change Amit is
> proposing here. If we don't, then we won't be able to accept NULL
> values even after we have the default partitioning stuff.
Attached is a patch for consideration. There are 2 actually, but maybe
they should be committed together if we decide do go with this.
Thanks,
Amit
>From 001db3ae9258f90964b73d2ef06af435be0a680d Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 16 May 2017 18:32:31 +0900
Subject: [PATCH 1/2] No explicit NOT NULL constraints on range partition keys
Instead generate them implicitly in get_qual_for_range().
---
doc/src/sgml/ref/create_table.sgml | 5 ----
src/backend/catalog/partition.c | 32 +++++++++++++++---------
src/backend/commands/tablecmds.c | 31 +----------------------
src/test/regress/expected/create_table.out | 40 +++++++++++-------------------
src/test/regress/expected/insert.out | 2 +-
src/test/regress/sql/create_table.sql | 5 ----
6 files changed, 36 insertions(+), 79 deletions(-)
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 484f81898b..0478e40447 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -454,11 +454,6 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
these constraints on individual partitions.
</para>
- <para>
- When using range partitioning, a <literal>NOT NULL</literal> constraint
- is added to each non-expression column in the partition key.
- </para>
-
</listitem>
</varlistentry>
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 832049c1ee..dee904d99d 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1529,23 +1529,31 @@ get_qual_for_range(PartitionKey key, PartitionBoundSpec *spec)
partexprs_item = list_head(key->partexprs);
for (i = 0; i < key->partnatts; i++)
{
- if (key->partattrs[i] == 0)
- {
- Expr *keyCol;
+ Expr *keyCol;
+ if (key->partattrs[i] != 0)
+ {
+ keyCol = (Expr *) makeVar(1,
+ key->partattrs[i],
+ key->parttypid[i],
+ key->parttypmod[i],
+ key->parttypcoll[i],
+ 0);
+ }
+ else
+ {
if (partexprs_item == NULL)
elog(ERROR, "wrong number of partition key expressions");
- keyCol = lfirst(partexprs_item);
+ keyCol = copyObject(lfirst(partexprs_item));
partexprs_item = lnext(partexprs_item);
- Assert(!IsA(keyCol, Var));
-
- nulltest = makeNode(NullTest);
- nulltest->arg = keyCol;
- nulltest->nulltesttype = IS_NOT_NULL;
- nulltest->argisrow = false;
- nulltest->location = -1;
- result = lappend(result, nulltest);
}
+
+ nulltest = makeNode(NullTest);
+ nulltest->arg = keyCol;
+ nulltest->nulltesttype = IS_NOT_NULL;
+ nulltest->argisrow = false;
+ nulltest->location = -1;
+ result = lappend(result, nulltest);
}
/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e259378051..e4d2bfb6b0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -805,13 +805,11 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
if (stmt->partspec)
{
char strategy;
- int partnatts,
- i;
+ int partnatts;
AttrNumber partattrs[PARTITION_MAX_KEYS];
Oid partopclass[PARTITION_MAX_KEYS];
Oid partcollation[PARTITION_MAX_KEYS];
List *partexprs = NIL;
- List *cmds = NIL;
/*
* We need to transform the raw parsetrees corresponding to partition
@@ -828,33 +826,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
partnatts = list_length(stmt->partspec->partParams);
StorePartitionKey(rel, strategy, partnatts, partattrs, partexprs,
partopclass, partcollation);
-
- /* Force key columns to be NOT NULL when using range partitioning */
- if (strategy == PARTITION_STRATEGY_RANGE)
- {
- for (i = 0; i < partnatts; i++)
- {
- AttrNumber partattno = partattrs[i];
- Form_pg_attribute attform = descriptor->attrs[partattno - 1];
-
- if (partattno != 0 && !attform->attnotnull)
- {
- /* Add a subcommand to make this one NOT NULL */
- AlterTableCmd *cmd = makeNode(AlterTableCmd);
-
- cmd->subtype = AT_SetNotNull;
- cmd->name = pstrdup(NameStr(attform->attname));
- cmds = lappend(cmds, cmd);
- }
- }
-
- /*
- * Although, there cannot be any partitions yet, we still need to
- * pass true for recurse; ATPrepSetNotNull() complains if we don't
- */
- if (cmds != NIL)
- AlterTableInternal(RelationGetRelid(rel), cmds, true);
- }
}
/*
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index bbf039ccad..39edf04cb4 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -407,18 +407,6 @@ SELECT relkind FROM pg_class WHERE relname = 'partitioned';
p
(1 row)
--- check that range partition key columns are marked NOT NULL
-SELECT attname, attnotnull FROM pg_attribute
- WHERE attrelid = 'partitioned'::regclass AND attnum > 0
- ORDER BY attnum;
- attname | attnotnull
----------+------------
- a | t
- b | f
- c | t
- d | t
-(4 rows)
-
-- prevent a function referenced in partition key from being dropped
DROP FUNCTION plusone(int);
ERROR: cannot drop function plusone(integer) because other objects depend on it
@@ -435,10 +423,10 @@ ERROR: cannot inherit from partitioned table "partitioned2"
Table "public.partitioned"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
- a | integer | | not null |
+ a | integer | | |
b | integer | | |
- c | text | | not null |
- d | text | | not null |
+ c | text | | |
+ d | text | | |
Partition key: RANGE (a oid_ops, plusone(b), c, d COLLATE "C")
\d partitioned2
@@ -544,9 +532,9 @@ CREATE TABLE part_forced_oids PARTITION OF oids_parted FOR VALUES FROM (1) TO (1
Table "public.part_forced_oids"
Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
- a | integer | | not null | | plain | |
+ a | integer | | | | plain | |
Partition of: oids_parted FOR VALUES FROM (1) TO (10)
-Partition constraint: ((a >= 1) AND (a < 10))
+Partition constraint: ((a IS NOT NULL) AND (a >= 1) AND (a < 10))
Has OIDs: yes
DROP TABLE oids_parted, part_forced_oids;
@@ -678,7 +666,7 @@ Partitions: part_c_1_10 FOR VALUES FROM (1) TO (10)
a | text | | | | extended | |
b | integer | | not null | 0 | plain | |
Partition of: part_c FOR VALUES FROM (1) TO (10)
-Partition constraint: ((a IS NOT NULL) AND (a = ANY (ARRAY['c'::text])) AND (b >= 1) AND (b < 10))
+Partition constraint: ((a IS NOT NULL) AND (a = ANY (ARRAY['c'::text])) AND (b IS NOT NULL) AND (b >= 1) AND (b < 10))
Check constraints:
"check_a" CHECK (length(a) > 0)
@@ -706,9 +694,9 @@ CREATE TABLE unbounded_range_part PARTITION OF range_parted4 FOR VALUES FROM (UN
--------+---------+-----------+----------+---------+---------+--------------+-------------
a | integer | | | | plain | |
b | integer | | | | plain | |
- c | integer | | not null | | plain | |
+ c | integer | | | | plain | |
Partition of: range_parted4 FOR VALUES FROM (UNBOUNDED, UNBOUNDED, UNBOUNDED) TO (UNBOUNDED, UNBOUNDED, UNBOUNDED)
-Partition constraint: ((abs(a) IS NOT NULL) AND (abs(b) IS NOT NULL))
+Partition constraint: ((abs(a) IS NOT NULL) AND (abs(b) IS NOT NULL) AND (c IS NOT NULL))
DROP TABLE unbounded_range_part;
CREATE TABLE range_parted4_1 PARTITION OF range_parted4 FOR VALUES FROM (UNBOUNDED, UNBOUNDED, UNBOUNDED) TO (1, UNBOUNDED, UNBOUNDED);
@@ -718,9 +706,9 @@ CREATE TABLE range_parted4_1 PARTITION OF range_parted4 FOR VALUES FROM (UNBOUND
--------+---------+-----------+----------+---------+---------+--------------+-------------
a | integer | | | | plain | |
b | integer | | | | plain | |
- c | integer | | not null | | plain | |
+ c | integer | | | | plain | |
Partition of: range_parted4 FOR VALUES FROM (UNBOUNDED, UNBOUNDED, UNBOUNDED) TO (1, UNBOUNDED, UNBOUNDED)
-Partition constraint: ((abs(a) IS NOT NULL) AND (abs(b) IS NOT NULL) AND (abs(a) <= 1))
+Partition constraint: ((abs(a) IS NOT NULL) AND (abs(b) IS NOT NULL) AND (c IS NOT NULL) AND (abs(a) <= 1))
CREATE TABLE range_parted4_2 PARTITION OF range_parted4 FOR VALUES FROM (3, 4, 5) TO (6, 7, UNBOUNDED);
\d+ range_parted4_2
@@ -729,9 +717,9 @@ CREATE TABLE range_parted4_2 PARTITION OF range_parted4 FOR VALUES FROM (3, 4, 5
--------+---------+-----------+----------+---------+---------+--------------+-------------
a | integer | | | | plain | |
b | integer | | | | plain | |
- c | integer | | not null | | plain | |
+ c | integer | | | | plain | |
Partition of: range_parted4 FOR VALUES FROM (3, 4, 5) TO (6, 7, UNBOUNDED)
-Partition constraint: ((abs(a) IS NOT NULL) AND (abs(b) IS NOT NULL) AND ((abs(a) > 3) OR ((abs(a) = 3) AND (abs(b) > 4)) OR ((abs(a) = 3) AND (abs(b) = 4) AND (c >= 5))) AND ((abs(a) < 6) OR ((abs(a) = 6) AND (abs(b) <= 7))))
+Partition constraint: ((abs(a) IS NOT NULL) AND (abs(b) IS NOT NULL) AND (c IS NOT NULL) AND ((abs(a) > 3) OR ((abs(a) = 3) AND (abs(b) > 4)) OR ((abs(a) = 3) AND (abs(b) = 4) AND (c >= 5))) AND ((abs(a) < 6) OR ((abs(a) = 6) AND (abs(b) <= 7))))
CREATE TABLE range_parted4_3 PARTITION OF range_parted4 FOR VALUES FROM (6, 8, UNBOUNDED) TO (9, UNBOUNDED, UNBOUNDED);
\d+ range_parted4_3
@@ -740,9 +728,9 @@ CREATE TABLE range_parted4_3 PARTITION OF range_parted4 FOR VALUES FROM (6, 8, U
--------+---------+-----------+----------+---------+---------+--------------+-------------
a | integer | | | | plain | |
b | integer | | | | plain | |
- c | integer | | not null | | plain | |
+ c | integer | | | | plain | |
Partition of: range_parted4 FOR VALUES FROM (6, 8, UNBOUNDED) TO (9, UNBOUNDED, UNBOUNDED)
-Partition constraint: ((abs(a) IS NOT NULL) AND (abs(b) IS NOT NULL) AND ((abs(a) > 6) OR ((abs(a) = 6) AND (abs(b) >= 8))) AND (abs(a) <= 9))
+Partition constraint: ((abs(a) IS NOT NULL) AND (abs(b) IS NOT NULL) AND (c IS NOT NULL) AND ((abs(a) > 6) OR ((abs(a) = 6) AND (abs(b) >= 8))) AND (abs(a) <= 9))
DROP TABLE range_parted4;
-- cleanup
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index 02429a37e3..232777bc3d 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -189,7 +189,7 @@ DETAIL: Failing row contains (a, 10).
insert into part4 values ('b', 10);
-- fail (partition key a has a NOT NULL constraint)
insert into part1 values (null);
-ERROR: null value in column "a" violates not-null constraint
+ERROR: new row for relation "part1" violates partition constraint
DETAIL: Failing row contains (null, null).
-- fail (expression key (b+0) cannot be null either)
insert into part1 values (1);
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 766f35a3ed..5a2774395e 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -410,11 +410,6 @@ CREATE TABLE partitioned (
-- check relkind
SELECT relkind FROM pg_class WHERE relname = 'partitioned';
--- check that range partition key columns are marked NOT NULL
-SELECT attname, attnotnull FROM pg_attribute
- WHERE attrelid = 'partitioned'::regclass AND attnum > 0
- ORDER BY attnum;
-
-- prevent a function referenced in partition key from being dropped
DROP FUNCTION plusone(int);
--
2.11.0
>From e8082713c21cb23913a59739a13c309bd59b3a55 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 16 May 2017 18:44:03 +0900
Subject: [PATCH 2/2] Change error when tuple-routing sees NULL in range keys
---
src/backend/catalog/partition.c | 21 ++++++++++++++++-----
src/test/regress/expected/insert.out | 3 ++-
2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index dee904d99d..336305a3bb 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1964,7 +1964,8 @@ get_partition_for_tuple(PartitionDispatch *pd,
{
*failed_at = parent;
*failed_slot = slot;
- return -1;
+ result = -1;
+ goto error_exit;
}
/*
@@ -1980,12 +1981,21 @@ get_partition_for_tuple(PartitionDispatch *pd,
if (key->strategy == PARTITION_STRATEGY_RANGE)
{
- /* Disallow nulls in the range partition key of the tuple */
+ /*
+ * Since we cannot route tuples with NULL partition keys through
+ * a range-partitioned table, simply return that no partition
+ * exists
+ */
for (i = 0; i < key->partnatts; i++)
+ {
if (isnull[i])
- ereport(ERROR,
- (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
- errmsg("range partition key of row contains null")));
+ {
+ *failed_at = parent;
+ *failed_slot = slot;
+ result = -1;
+ goto error_exit;
+ }
+ }
}
/*
@@ -2048,6 +2058,7 @@ get_partition_for_tuple(PartitionDispatch *pd,
parent = pd[-parent->indexes[cur_index]];
}
+error_exit:
ecxt->ecxt_scantuple = ecxt_scantuple_old;
return result;
}
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index 232777bc3d..8b0752a0d2 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -247,7 +247,8 @@ insert into range_parted values ('b', 1);
insert into range_parted values ('b', 10);
-- fail (partition key (b+0) is null)
insert into range_parted values ('a');
-ERROR: range partition key of row contains null
+ERROR: no partition of relation "range_parted" found for row
+DETAIL: Partition key of the failing row contains (a, (b + 0)) = (a, null).
select tableoid::regclass, * from range_parted;
tableoid | a | b
----------+---+----
--
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
