From 3ec76ff1f2cf52e9b900349957b42d28128b7bc7 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Thu, 18 May 2017 13:48:10 -0400 Subject: [PATCH] Don't explicitly mark range partitioning columns NOT NULL. This seemed like a good idea originally because there's no way to mark a range partition as accepting NULL, but that now seems more like a current limitation than something we want to lock down for all time. For example, there's a proposal to add the notion of a default partition which accepts all rows not otherwise routed, which directly conflicts with the idea that a range-partitioned table should never allow nulls anywhere. So let's change this while we still can, by putting the NOT NULL test into the partition constraint instead of changing the column properties. Amit Langote and Robert Haas, reviewed by Amit Kapila Discussion: http://postgr.es/m/8e2dd63d-c6fb-bb74-3c2b-ed6d63629c9d@lab.ntt.co.jp --- .../postgres_fdw/expected/postgres_fdw.out | 4 +- doc/src/sgml/ref/create_table.sgml | 5 -- src/backend/catalog/partition.c | 71 ++++++++++++------- src/backend/commands/tablecmds.c | 57 +-------------- src/test/regress/expected/alter_table.out | 7 -- src/test/regress/expected/create_table.out | 40 ++++------- src/test/regress/expected/insert.out | 5 +- src/test/regress/sql/alter_table.sql | 8 --- src/test/regress/sql/create_table.sql | 5 -- 9 files changed, 64 insertions(+), 138 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index d1bc5b0660..4d86ab54dd 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -6987,7 +6987,7 @@ FDW Options: (schema_name 'import_source', table_name 't3') Foreign table "import_dest1.t4" Column | Type | Collation | Nullable | Default | FDW Options --------+---------+-----------+----------+---------+-------------------- - c1 | integer | | not null | | (column_name 'c1') + c1 | integer | | | | (column_name 'c1') Server: loopback FDW Options: (schema_name 'import_source', table_name 't4') @@ -7051,7 +7051,7 @@ FDW Options: (schema_name 'import_source', table_name 't3') Foreign table "import_dest2.t4" Column | Type | Collation | Nullable | Default | FDW Options --------+---------+-----------+----------+---------+-------------------- - c1 | integer | | not null | | (column_name 'c1') + c1 | integer | | | | (column_name 'c1') Server: loopback FDW Options: (schema_name 'import_source', table_name 't4') 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 acd9f2d273..7304f6c29a 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -1449,17 +1449,18 @@ get_range_key_properties(PartitionKey key, int keynum, * as the lower bound tuple and (au, bu, cu) as the upper bound tuple, we * generate an expression tree of the following form: * + * (a IS NOT NULL) and (b IS NOT NULL) and (c IS NOT NULL) + * AND * (a > al OR (a = al AND b > bl) OR (a = al AND b = bl AND c >= cl)) * AND * (a < au OR (a = au AND b < bu) OR (a = au AND b = bu AND c < cu)) * - * If, say, b were an expression key instead of a simple column, we also - * append (b IS NOT NULL) to the AND's argument list. - * * It is often the case that a prefix of lower and upper bound tuples contains * the same values, for example, (al = au), in which case, we will emit an * expression tree of the following form: * + * (a IS NOT NULL) and (b IS NOT NULL) and (c IS NOT NULL) + * AND * (a = al) * AND * (b > bl OR (b = bl AND c >= cl)) @@ -1472,11 +1473,11 @@ get_range_key_properties(PartitionKey key, int keynum, * (b < bu) OR (b = bu), which is simplified to (b <= bu) * * In most common cases with only one partition column, say a, the following - * expression tree will be generated: a >= al AND a < au + * expression tree will be generated: a IS NOT NULL AND a >= al AND a < au * * 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 @@ -1512,32 +1513,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); } /* @@ -1948,7 +1954,8 @@ get_partition_for_tuple(PartitionDispatch *pd, { *failed_at = parent; *failed_slot = slot; - return -1; + result = -1; + goto error_exit; } /* @@ -1964,12 +1971,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; + } + } } /* @@ -2032,6 +2048,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/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 7319aa597e..99c51b812d 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); - } } /* @@ -5702,32 +5673,6 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode) heap_close(parent, AccessShareLock); } - /* - * If the table is a range partitioned table, check that the column is not - * in the partition key. - */ - if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - { - PartitionKey key = RelationGetPartitionKey(rel); - - if (get_partition_strategy(key) == PARTITION_STRATEGY_RANGE) - { - int partnatts = get_partition_natts(key), - i; - - for (i = 0; i < partnatts; i++) - { - AttrNumber partattnum = get_partition_col_attnum(key, i); - - if (partattnum == attnum) - ereport(ERROR, - (errcode(ERRCODE_INVALID_TABLE_DEFINITION), - errmsg("column \"%s\" is in range partition key", - colName))); - } - } - } - /* * Okay, actually perform the catalog change ... if needed */ diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 6ebebb4a03..c88fd76848 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -3079,13 +3079,6 @@ ALTER TABLE partitioned DROP COLUMN b; ERROR: cannot drop column referenced in partition key expression ALTER TABLE partitioned ALTER COLUMN b TYPE char(5); ERROR: cannot alter type of column referenced in partition key expression --- cannot drop NOT NULL on columns in the range partition key -ALTER TABLE partitioned ALTER COLUMN a DROP NOT NULL; -ERROR: column "a" is in range partition key --- it's fine however to drop one on the list partition key column -CREATE TABLE list_partitioned (a int not null) partition by list (a); -ALTER TABLE list_partitioned ALTER a DROP NOT NULL; -DROP TABLE list_partitioned; -- partitioned table cannot participate in regular inheritance CREATE TABLE nonpartitioned ( a int, 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..8b0752a0d2 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); @@ -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 ----------+---+---- diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index f014675628..c0e29720dc 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1947,14 +1947,6 @@ ALTER TABLE partitioned ALTER COLUMN a TYPE char(5); ALTER TABLE partitioned DROP COLUMN b; ALTER TABLE partitioned ALTER COLUMN b TYPE char(5); --- cannot drop NOT NULL on columns in the range partition key -ALTER TABLE partitioned ALTER COLUMN a DROP NOT NULL; - --- it's fine however to drop one on the list partition key column -CREATE TABLE list_partitioned (a int not null) partition by list (a); -ALTER TABLE list_partitioned ALTER a DROP NOT NULL; -DROP TABLE list_partitioned; - -- partitioned table cannot participate in regular inheritance CREATE TABLE nonpartitioned ( a int, 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.40.0