Re: [HACKERS] Adding support for Default partition in partitioning - Mailing list pgsql-hackers

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.raghuwanshi@enterprisedb.com> wrote:

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