Re: Adding support for Default partition in partitioning

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(dot)raghuwanshi(at)enterprisedb(dot)com> wrote:

> On Thu, May 4, 2017 at 5:14 PM, Rahila Syed <rahilasyed90(at)gmail(dot)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)
>