Hello, Please find attached an updated patch with review comments and bugs reported till date implemented.
>1. >In following block, we can just do with def_index, and we do not need found_def >flag. We can check if def_index is -1 or not to decide if default partition is >present. found_def is used to set boundinfo->has_default which is used at couple of other places to check if default partition exists. The implementation is similar to has_null. >3. >In following function isDefaultPartitionBound, first statement "return false" >is not needed. It is needed to return false if the node is not DefElem. Todo: Add regression tests Documentation Thank you, Rahila Syed On Fri, May 5, 2017 at 1:30 AM, Jeevan Ladhe <jeevan.la...@enterprisedb.com> wrote: > Hi Rahila, > > I have started reviewing your latest patch, and here are my initial > comments: > > 1. > In following block, we can just do with def_index, and we do not need > found_def > flag. We can check if def_index is -1 or not to decide if default > partition is > present. > > @@ -166,6 +172,8 @@ RelationBuildPartitionDesc(Relation rel) > /* List partitioning specific */ > PartitionListValue **all_values = NULL; > bool found_null = false; > + bool found_def = false; > + int def_index = -1; > int null_index = -1; > > 2. > In check_new_partition_bound, in case of PARTITION_STRATEGY_LIST, remove > following duplicate declaration of boundinfo, because it is confusing and > after > your changes it is not needed as its not getting overridden in the if block > locally. > if (partdesc->nparts > 0) > { > PartitionBoundInfo boundinfo = partdesc->boundinfo; > ListCell *cell; > > > 3. > In following function isDefaultPartitionBound, first statement "return > false" > is not needed. > > + * Returns true if the partition bound is default > + */ > +bool > +isDefaultPartitionBound(Node *value) > +{ > + if (IsA(value, DefElem)) > + { > + DefElem *defvalue = (DefElem *) value; > + if(!strcmp(defvalue->defname, "DEFAULT")) > + return true; > + return false; > + } > + return false; > +} > > 4. > As mentioned in my previous set of comments, following if block inside a > loop > in get_qual_for_default needs a break: > > + foreach(cell1, bspec->listdatums) > + { > + Node *value = lfirst(cell1); > + if (isDefaultPartitionBound(value)) > + { > + def_elem = true; > + *defid = inhrelid; > + } > + } > > 5. > In the grammar the rule default_part_list is not needed: > > +default_partition: > + DEFAULT { $$ = (Node *)makeDefElem("DEFAULT", NULL, @1); } > + > +default_part_list: > + default_partition { $$ = list_make1($1); } > + ; > + > > Instead you can simply declare default_partition as a list and write it as: > > default_partition: > DEFAULT > { > Node *def = (Node *)makeDefElem("DEFAULT", NULL, @1); > $$ = list_make1(def); > } > > 6. > You need to change the output of the describe command, which is currently > as below: postgres=# \d+ test; Table "public.test" Column | Type | > Collation | Nullable | Default | Storage | Stats target | Description > --------+---------+-----------+----------+---------+---------+--------------+------------- > a | integer | | | | plain | | b | date | | | | plain | | Partition key: > LIST (a) Partitions: pd FOR VALUES IN (DEFAULT), test_p1 FOR VALUES IN (4, > 5) What about changing the Paritions output as below: Partitions: *pd > DEFAULT,* test_p1 FOR VALUES IN (4, 5) > > 7. > You need to handle tab completion for DEFAULT. > e.g. > If I partially type following command: > CREATE TABLE pd PARTITION OF test DEFA > and then press tab, I get following completion: > CREATE TABLE pd PARTITION OF test FOR VALUES > > I did some primary testing and did not find any problem so far. > > I will review and test further and let you know my comments. > > Regards, > Jeevan Ladhe > > On Thu, May 4, 2017 at 6:09 PM, Rajkumar Raghuwanshi < > rajkumar.raghuwan...@enterprisedb.com> wrote: > >> On Thu, May 4, 2017 at 5:14 PM, Rahila Syed <rahilasye...@gmail.com> >> wrote: >> >>> The syntax implemented in this patch is as follows, >>> >>> CREATE TABLE p11 PARTITION OF p1 DEFAULT; >>> >>> Applied v9 patches, table description still showing old pattern of >> default partition. Is it expected? >> >> create table lpd (a int, b int, c varchar) partition by list(a); >> create table lpd_d partition of lpd DEFAULT; >> >> \d+ lpd >> Table "public.lpd" >> Column | Type | Collation | Nullable | Default | Storage | >> Stats target | Description >> --------+-------------------+-----------+----------+-------- >> -+----------+--------------+------------- >> a | integer | | | | plain >> | | >> b | integer | | | | plain >> | | >> c | character varying | | | | extended >> | | >> Partition key: LIST (a) >> Partitions: lpd_d FOR VALUES IN (DEFAULT) >> > >
default_partition_v10.patch
Description: application/download
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers