Re: Declarative partitioning - another take

Thanks for the review!

On 2016/10/25 20:32, Amit Kapila wrote:
> 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?

Sure, CopyTo() can be be taught to scan leaf partitions when a partitioned
table is specified, but I thought this may be fine initially.

> 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

Actually "list partition" works here as a verb, as in "to list partition".

> 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?

Looks like a leftover hunk from previous revision.  Will fix.

> 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.

Oops, will fix.

> 5.
> +/*
> + * PartitionSpec - partition key definition including the strategy
> + *
> + * 'strategy' partition strategy name ('list', 'range', etc.)
> 
> etc. in above comment seems to be unnecessary.

Will fix, although  I thought that list is yet incomplete.

> 6.
> + {PartitionedRelationId, /* PARTEDRELID */
> 
> Here PARTEDRELID sounds inconvenient, how about PARTRELID?

Agreed.  There used to be another catalog which had used up PARTRELID, but
that's no longer an issue.

I will include these changes in the next version of patches I will post
soon in reply to [1].

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoYJcUTcN7vVgg54GHtffH11JJWYZnfF4KiRxjV-iaACQg%40mail.gmail.com