From: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, Amit Langote <amitlangote09(at)gmail(dot)com>, Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Declarative partitioning - another take |
Date: | 2016-10-26 01:06:14 |
Message-ID: | e4b2a493-9f40-e8ee-03e2-0437ae682714@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(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?
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
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Langote | 2016-10-26 02:00:15 | Re: Declarative partitioning - another take |
Previous Message | Kyotaro HORIGUCHI | 2016-10-26 00:49:51 | Unused variable in postgres_fdw/deparse.c |