Re: [HACKERS] 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 > 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
Re: [HACKERS] NOT NULL constraints on range partition key columns
On Tue, May 16, 2017 at 8:19 PM, Amit Langote 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
Re: [HACKERS] 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
> 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
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.
-
- When using range partitioning, a NOT NULL constraint
- is added to each non-expression column in the partition key.
-
-
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 partop
Re: [HACKERS] NOT NULL constraints on range partition key columns
On Tue, May 16, 2017 at 3:26 PM, Amit Langote wrote: > On 2017/05/16 4:29, Robert Haas wrote: >> On Mon, May 15, 2017 at 9:12 AM, Amit Kapila 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
Re: [HACKERS] 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 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
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.
-
- When using range partitioning, a NOT NULL constraint
- is added to each non-expression column in the partition key.
-
-
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
Re: [HACKERS] NOT NULL constraints on range partition key columns
On Mon, May 15, 2017 at 9:12 AM, Amit Kapila 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
Re: [HACKERS] NOT NULL constraints on range partition key columns
On Mon, May 15, 2017 at 11:46 AM, Amit Langote 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 >> 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
[HACKERS] NOT NULL constraints on range partition key columns
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 > 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] https://www.postgresql.org/message-id/f52fb5c3-b6ad-b6ff-4bb6-145fdb805fa6%40lab.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
