On 2017/05/16 21:16, Amit Kapila wrote:
> On Tue, May 16, 2017 at 3:26 PM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>> On 2017/05/16 4:29, Robert Haas wrote:
>>> 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.
>>
> 
> After your changes in get_qual_for_range(), below comment in that
> function needs an update and I feel it is better to update the
> function header as well.
> 
> /*
> * A range-partitioned table does not allow partition keys to be null. For
> * simple columns, their NOT NULL constraint suffices for the enforcement
> * of non-nullability.  But for the expression keys, which are still
> * nullable, we must emit a IS NOT NULL expression.  Collect them in
> * result first.
> */

Thanks for the review.  I updated the comments.

Thanks,
Amit
>From 789d35765a1c00bd5ae3ab2956c4b0292f36c9b1 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            | 41 +++++++++++++++++-------------
 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, 39 insertions(+), 85 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..9de6511e59 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1484,7 +1484,7 @@ get_range_key_properties(PartitionKey key, int keynum,
  *
  * If all values of both lower and upper bounds are UNBOUNDED, the partition
  * does not really have a constraint, except the IS NOT NULL constraint for
- * any expression keys.
+ * partition keys.
  *
  * If we end up with an empty result list, we append return a single-member
  * list containing a constant-true expression in that case, because callers
@@ -1520,32 +1520,37 @@ get_qual_for_range(PartitionKey key, PartitionBoundSpec *spec)
 	num_or_arms = key->partnatts;
 
 	/*
-	 * A range-partitioned table does not allow partition keys to be null. For
-	 * simple columns, their NOT NULL constraint suffices for the enforcement
-	 * of non-nullability.  But for the expression keys, which are still
-	 * nullable, we must emit a IS NOT NULL expression.  Collect them in
-	 * result first.
+	 * A range-partitioned table does not currently allow partition keys to
+	 * be null, so emit an IS NOT NULL expression for each key column.
 	 */
 	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 7cc3668d6c3f520377d4cc0e2c08e55903c44cc6 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 9de6511e59..eaa18254bd 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1961,7 +1961,8 @@ get_partition_for_tuple(PartitionDispatch *pd,
 		{
 			*failed_at = parent;
 			*failed_slot = slot;
-			return -1;
+			result = -1;
+			goto error_exit;
 		}
 
 		/*
@@ -1977,12 +1978,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;
+				}
+			}
 		}
 
 		/*
@@ -2045,6 +2055,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

Reply via email to