Re: Adding support for Default partition in partitioning

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(dot)ladhe(at)enterprisedb(dot)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(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)
>>
>
>