Re: unsupportable composite type partition keys - Mailing list pgsql-hackers

I wrote:
> Now as far as point 1 goes, I think it's not really that awful to use
> CheckAttributeType() with a dummy attribute name.  The attached
> incomplete patch uses "partition key" which causes it to emit errors
> like
> regression=# create table fool (a int, b int) partition by list ((row(a, b)));
> ERROR:  column "partition key" has pseudo-type record
> I don't think that that's unacceptable.  But if we wanted to improve it,
> we could imagine adding another flag, say CHKATYPE_IS_PARTITION_KEY,
> that doesn't affect CheckAttributeType's semantics, just the wording of
> the error messages it throws.

Here's a fleshed-out patch that does it like that.

While poking at this, I also started to wonder why CheckAttributeType
wasn't recursing into ranges, since those are our other kind of
container type.  And the answer is that it must, because we allow
creation of ranges over composite types:

regression=# create table foo (f1 int, f2 int);
CREATE TABLE
regression=# create type foorange as range (subtype = foo);
CREATE TYPE
regression=# alter table foo add column r foorange;
ALTER TABLE

Simple things still work on table foo, but surely this is exactly
what CheckAttributeType is supposed to be preventing.  With the
second attached patch you get

regression=# alter table foo add column r foorange;
ERROR:  composite type foo cannot be made a member of itself

The second patch needs to go back all the way, the first one
only as far as we have partitions.

            regards, tom lane

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 8404904..82398da 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -572,6 +572,10 @@ CheckAttributeNamesTypes(TupleDesc tupdesc, char relkind,
  * are reliably identifiable only within a session, since the identity info
  * may use a typmod that is only locally assigned.  The caller is expected
  * to know whether these cases are safe.)
+ *
+ * flags can also control the phrasing of the error messages.  If
+ * CHKATYPE_IS_PARTKEY is specified, "attname" should be a partition key
+ * column number as text, not a real column name.
  * --------------------------------
  */
 void
@@ -598,10 +602,19 @@ CheckAttributeType(const char *attname,
         if (!((atttypid == ANYARRAYOID && (flags & CHKATYPE_ANYARRAY)) ||
               (atttypid == RECORDOID && (flags & CHKATYPE_ANYRECORD)) ||
               (atttypid == RECORDARRAYOID && (flags & CHKATYPE_ANYRECORD))))
-            ereport(ERROR,
-                    (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
-                     errmsg("column \"%s\" has pseudo-type %s",
-                            attname, format_type_be(atttypid))));
+        {
+            if (flags & CHKATYPE_IS_PARTKEY)
+                ereport(ERROR,
+                        (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+                /* translator: first %s is an integer not a name */
+                         errmsg("partition key column %s has pseudo-type %s",
+                                attname, format_type_be(atttypid))));
+            else
+                ereport(ERROR,
+                        (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+                         errmsg("column \"%s\" has pseudo-type %s",
+                                attname, format_type_be(atttypid))));
+        }
     }
     else if (att_typtype == TYPTYPE_DOMAIN)
     {
@@ -648,7 +661,7 @@ CheckAttributeType(const char *attname,
             CheckAttributeType(NameStr(attr->attname),
                                attr->atttypid, attr->attcollation,
                                containing_rowtypes,
-                               flags);
+                               flags & ~CHKATYPE_IS_PARTKEY);
         }

         relation_close(relation, AccessShareLock);
@@ -670,11 +683,21 @@ CheckAttributeType(const char *attname,
      * useless, and it cannot be dumped, so we must disallow it.
      */
     if (!OidIsValid(attcollation) && type_is_collatable(atttypid))
-        ereport(ERROR,
-                (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
-                 errmsg("no collation was derived for column \"%s\" with collatable type %s",
-                        attname, format_type_be(atttypid)),
-                 errhint("Use the COLLATE clause to set the collation explicitly.")));
+    {
+        if (flags & CHKATYPE_IS_PARTKEY)
+            ereport(ERROR,
+                    (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+            /* translator: first %s is an integer not a name */
+                     errmsg("no collation was derived for partition key column %s with collatable type %s",
+                            attname, format_type_be(atttypid)),
+                     errhint("Use the COLLATE clause to set the collation explicitly.")));
+        else
+            ereport(ERROR,
+                    (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+                     errmsg("no collation was derived for column \"%s\" with collatable type %s",
+                            attname, format_type_be(atttypid)),
+                     errhint("Use the COLLATE clause to set the collation explicitly.")));
+    }
 }

 /*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e8e004e..845f010 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15096,12 +15096,24 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
         {
             /* Expression */
             Node       *expr = pelem->expr;
+            char        partattname[16];

             Assert(expr != NULL);
             atttype = exprType(expr);
             attcollation = exprCollation(expr);

             /*
+             * The expression must be of a storable type (e.g., not RECORD).
+             * The test is the same as for whether a table column is of a safe
+             * type (which is why we needn't check for the non-expression
+             * case).
+             */
+            snprintf(partattname, sizeof(partattname), "%d", attn + 1);
+            CheckAttributeType(partattname,
+                               atttype, attcollation,
+                               NIL, CHKATYPE_IS_PARTKEY);
+
+            /*
              * Strip any top-level COLLATE clause.  This ensures that we treat
              * "x COLLATE y" and "(x COLLATE y)" alike.
              */
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index eec71c2..2e63137 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -22,6 +22,7 @@
 /* flag bits for CheckAttributeType/CheckAttributeNamesTypes */
 #define CHKATYPE_ANYARRAY        0x01    /* allow ANYARRAY */
 #define CHKATYPE_ANYRECORD        0x02    /* allow RECORD and RECORD[] */
+#define CHKATYPE_IS_PARTKEY        0x04    /* attname is part key # not column */

 typedef struct RawColumnDefault
 {
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index f630168..50de4b3 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -375,7 +375,7 @@ CREATE TABLE partitioned (
 ERROR:  cannot use subquery in partition key expression
 CREATE TABLE partitioned (
     a int
-) PARTITION BY RANGE (('a'));
+) PARTITION BY RANGE ((42));
 ERROR:  cannot use constant expression as partition key
 CREATE FUNCTION const_func () RETURNS int AS $$ SELECT 1; $$ LANGUAGE SQL IMMUTABLE;
 CREATE TABLE partitioned (
@@ -402,6 +402,17 @@ CREATE TABLE partitioned (
 ERROR:  cannot use system column "xmin" in partition key
 LINE 3: ) PARTITION BY RANGE (xmin);
                               ^
+-- cannot use pseudotypes
+CREATE TABLE partitioned (
+    a int,
+    b int
+) PARTITION BY RANGE (((a, b)));
+ERROR:  partition key column 1 has pseudo-type record
+CREATE TABLE partitioned (
+    a int,
+    b int
+) PARTITION BY RANGE (a, ('unknown'));
+ERROR:  partition key column 2 has pseudo-type unknown
 -- functions in key must be immutable
 CREATE FUNCTION immut_func (a int) RETURNS int AS $$ SELECT a + random()::int; $$ LANGUAGE SQL;
 CREATE TABLE partitioned (
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index e835b65..9a40d7b 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -361,7 +361,7 @@ CREATE TABLE partitioned (

 CREATE TABLE partitioned (
     a int
-) PARTITION BY RANGE (('a'));
+) PARTITION BY RANGE ((42));

 CREATE FUNCTION const_func () RETURNS int AS $$ SELECT 1; $$ LANGUAGE SQL IMMUTABLE;
 CREATE TABLE partitioned (
@@ -384,6 +384,16 @@ CREATE TABLE partitioned (
     a int
 ) PARTITION BY RANGE (xmin);

+-- cannot use pseudotypes
+CREATE TABLE partitioned (
+    a int,
+    b int
+) PARTITION BY RANGE (((a, b)));
+CREATE TABLE partitioned (
+    a int,
+    b int
+) PARTITION BY RANGE (a, ('unknown'));
+
 -- functions in key must be immutable
 CREATE FUNCTION immut_func (a int) RETURNS int AS $$ SELECT a + random()::int; $$ LANGUAGE SQL;
 CREATE TABLE partitioned (
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 82398da..452a7f3 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -668,6 +668,15 @@ CheckAttributeType(const char *attname,
 
         containing_rowtypes = list_delete_last(containing_rowtypes);
     }
+    else if (att_typtype == TYPTYPE_RANGE)
+    {
+        /*
+         * If it's a range, recurse to check its subtype.
+         */
+        CheckAttributeType(attname, get_range_subtype(atttypid), attcollation,
+                           containing_rowtypes,
+                           flags);
+    }
     else if (OidIsValid((att_typelem = get_element_type(atttypid))))
     {
         /*