Re: Declarative partitioning - another take

On Thu, Oct 6, 2016 at 12:44 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2016/10/05 2:12, Robert Haas wrote:

> Attached revised patches.

Few assorted review comments for 0001-Catalog*:

1.
@@ -1775,6 +1775,12 @@ BeginCopyTo(ParseState *pstate,
{
..
+ else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot copy from partitioned table \"%s\"",
+ RelationGetRelationName(rel)),
+ errhint("Try the COPY (SELECT ...) TO variant.")));
..
}

Why is this restriction? Won't it be useful to allow it for the cases
when user wants to copy the data of all the partitions?

2.
+ if (!pg_strcasecmp(stmt->partspec->strategy, "list") &&
+ partnatts > 1)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("cannot list partition using more than one column")));

/cannot list/cannot use list

3.
@@ -77,7 +77,7 @@ typedef enum DependencyType
DEPENDENCY_INTERNAL = 'i',
DEPENDENCY_EXTENSION = 'e',
DEPENDENCY_AUTO_EXTENSION = 'x',
- DEPENDENCY_PIN = 'p'
+ DEPENDENCY_PIN = 'p',
} DependencyType;

Why is this change required?

4.
@@ -0,0 +1,69 @@
+/*-------------------------------------------------------------------------
+ *
+ * pg_partitioned_table.h
+ * definition of the system "partitioned table" relation
+ * along with the relation's initial contents.
+ *
+ *
+ * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group

Copyright year should be 2016.

5.
+/*
+ * PartitionSpec - partition key definition including the strategy
+ *
+ * 'strategy' partition strategy name ('list', 'range', etc.)

etc. in above comment seems to be unnecessary.

6.
+ {PartitionedRelationId, /* PARTEDRELID */

Here PARTEDRELID sounds inconvenient, how about PARTRELID?

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