NOT NULL constraints on range partition key columns

Started by Amit Langoteover 8 years ago8 messages
#1Amit Langote
Langote_Amit_f8@lab.ntt.co.jp

Starting a new thread to discuss the proposal I put forward in [1]/messages/by-id/f52fb5c3-b6ad-b6ff-4bb6-145fdb805fa6@lab.ntt.co.jp to stop
creating explicit NOT NULL constraint on range partition keys that are
simple columns. I said the following:

On 2017/05/12 11:20, Robert Haas wrote:

On Thu, May 11, 2017 at 10:15 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:

On 2017/05/12 10:42, Robert Haas wrote:

Actually, I think that not supporting nulls for range partitioning may
have been a fairly bad decision.

I think the relevant discussion concluded [1] that way, because we
couldn't decide which interface to provide for specifying where NULLs are
placed or because we decided to think about it later.

Yeah, but I have a feeling that marking the columns NOT NULL is going
to make it really hard to support that in the future when we get the
syntax hammered out. If it had only affected the partition
constraints that'd be different.

So, adding keycol IS NOT NULL (like we currently do for expressions) in
the implicit partition constraint would be more future-proof than
generating an actual catalogued NOT NULL constraint on the keycol? I now
tend to think it would be better. Directly inserting into a range
partition with a NULL value for a column currently generates a "null value
in column \"%s\" violates not-null constraint" instead of perhaps more
relevant "new row for relation \"%s\" violates partition constraint".
That said, we *do* document the fact that a NOT NULL constraint is added
on range key columns, but we might as well document instead that we don't
currently support routing tuples with NULL values in the partition key
through a range-partitioned table and so NULL values cause error.

Can we still decide to do that instead?

Thanks,
Amit

[1]: /messages/by-id/f52fb5c3-b6ad-b6ff-4bb6-145fdb805fa6@lab.ntt.co.jp
/messages/by-id/f52fb5c3-b6ad-b6ff-4bb6-145fdb805fa6@lab.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

#2Amit Kapila
amit.kapila16@gmail.com
In reply to: Amit Langote (#1)
Re: NOT NULL constraints on range partition key columns

On Mon, May 15, 2017 at 11:46 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Starting a new thread to discuss the proposal I put forward in [1] to stop
creating explicit NOT NULL constraint on range partition keys that are
simple columns. I said the following:

On 2017/05/12 11:20, Robert Haas wrote:

On Thu, May 11, 2017 at 10:15 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:

On 2017/05/12 10:42, Robert Haas wrote:

Actually, I think that not supporting nulls for range partitioning may
have been a fairly bad decision.

I think the relevant discussion concluded [1] that way, because we
couldn't decide which interface to provide for specifying where NULLs are
placed or because we decided to think about it later.

Yeah, but I have a feeling that marking the columns NOT NULL is going
to make it really hard to support that in the future when we get the
syntax hammered out. If it had only affected the partition
constraints that'd be different.

So, adding keycol IS NOT NULL (like we currently do for expressions) in
the implicit partition constraint would be more future-proof than
generating an actual catalogued NOT NULL constraint on the keycol? I now
tend to think it would be better. Directly inserting into a range
partition with a NULL value for a column currently generates a "null value
in column \"%s\" violates not-null constraint" instead of perhaps more
relevant "new row for relation \"%s\" violates partition constraint".
That said, we *do* document the fact that a NOT NULL constraint is added
on range key columns, but we might as well document instead that we don't
currently support routing tuples with NULL values in the partition key
through a range-partitioned table and so NULL values cause error.

Can't we allow NULL to get inserted into the partition (leaf
partition) if the user uses the partition name in Insert statement?
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.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Robert Haas
robertmhaas@gmail.com
In reply to: Amit Kapila (#2)
Re: NOT NULL constraints on range partition key columns

On Mon, May 15, 2017 at 9:12 AM, Amit Kapila <amit.kapila16@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.

--
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

#4Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Robert Haas (#3)
2 attachment(s)
Re: NOT NULL constraints on range partition key columns

On 2017/05/16 4:29, Robert Haas wrote:

On Mon, May 15, 2017 at 9:12 AM, Amit Kapila <amit.kapila16@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

Attachments:

0001-No-explicit-NOT-NULL-constraints-on-range-partition-.patchtext/x-diff; name=0001-No-explicit-NOT-NULL-constraints-on-range-partition-.patchDownload
From 001db3ae9258f90964b73d2ef06af435be0a680d Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@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

0002-Change-error-when-tuple-routing-sees-NULL-in-range-k.patchtext/x-diff; name=0002-Change-error-when-tuple-routing-sees-NULL-in-range-k.patchDownload
From e8082713c21cb23913a59739a13c309bd59b3a55 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@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

#5Amit Kapila
amit.kapila16@gmail.com
In reply to: Amit Langote (#4)
Re: NOT NULL constraints on range partition key columns

On Tue, May 16, 2017 at 3:26 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2017/05/16 4:29, Robert Haas wrote:

On Mon, May 15, 2017 at 9:12 AM, Amit Kapila <amit.kapila16@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.

Right and that makes sense.

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.

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.
*/

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Amit Kapila (#5)
2 attachment(s)
Re: NOT NULL constraints on range partition key columns

On 2017/05/16 21:16, Amit Kapila wrote:

On Tue, May 16, 2017 at 3:26 PM, Amit Langote
<Langote_Amit_f8@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

Attachments:

0001-No-explicit-NOT-NULL-constraints-on-range-partition-.patchtext/x-diff; name=0001-No-explicit-NOT-NULL-constraints-on-range-partition-.patchDownload
From 789d35765a1c00bd5ae3ab2956c4b0292f36c9b1 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@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

0002-Change-error-when-tuple-routing-sees-NULL-in-range-k.patchtext/x-diff; name=0002-Change-error-when-tuple-routing-sees-NULL-in-range-k.patchDownload
From 7cc3668d6c3f520377d4cc0e2c08e55903c44cc6 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@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

#7Robert Haas
robertmhaas@gmail.com
In reply to: Amit Langote (#6)
Re: NOT NULL constraints on range partition key columns

On Tue, May 16, 2017 at 8:19 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Thanks for the review. I updated the comments.

I found several more places that also needed to be updated using 'git
grep'. Committed with various additions.

--
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

#8Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Robert Haas (#7)
Re: NOT NULL constraints on range partition key columns

On 2017/05/19 3:06, Robert Haas wrote:

On Tue, May 16, 2017 at 8:19 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Thanks for the review. I updated the comments.

I found several more places that also needed to be updated using 'git
grep'. Committed with various additions.

Thanks a lot.

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