Re: Declarative partitioning - another take

Amit Kapila <amit.kapila16@gmail.com>

From: Amit Kapila <amit.kapila16@gmail.com>

To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>

Cc: Robert Haas <robertmhaas@gmail.com>, Amit Langote <amitlangote09@gmail.com>, Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>, Pg Hackers <pgsql-hackers@postgresql.org>

Date: 2016-10-25T11:32:54Z

Lists: pgsql-hackers

On Thu, Oct 6, 2016 at 12:44 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.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

Commits

Fix typo.
- 03bf59676ea0 10.0 landed
Document trigger-firing behavior for inheritance/partitioning.
- e17628145ac3 10.0 landed
Fire per-statement triggers on partitioned tables.
- e180c8aa8caf 10.0 landed
Set ecxt_scantuple correctly for tuple routing.
- 132488bfee68 10.0 landed
Fix interaction of partitioned tables with BulkInsertState.
- b1ecb9b3fcfb 10.0 landed
Avoid core dump for empty prepared statement in an aborted transaction.
- ba61a04bc7fe 10.0 cited
Fix some problems in check_new_partition_bound().
- 8a8afe2f54c2 10.0 landed
Remove unnecessary arguments from partitioning functions.
- 18fc5192a631 10.0 cited
Fix reporting of constraint violations for table partitioning.
- f1b4c771ea74 10.0 cited
Fix tuple routing in cases where tuple descriptors don't match.
- 2ac3ef7a01df 10.0 cited
Invalid parent's relcache after CREATE TABLE .. PARTITION OF.
- 7cd0fd655d68 10.0 landed
Doc: improve documentation about inheritance.
- 162477a63d3c 10.0 cited