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...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
