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