Postgres Pro
Downloads
Home   >   mailing lists

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

From Jeevan Ladhe
Subject Re: [HACKERS] Adding support for Default partition in partitioning
Date
Msg-id CAOgcT0NUUQXWRXmeVKkYTDQvWoKLYRMiPbc89ua6i_gG8FPDmA@mail.gmail.com
Whole thread Raw
In response to Re: [HACKERS] Adding support for Default partition in partitioning  (Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>)
Responses Re: [HACKERS] Adding support for Default partition in partitioning  (Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>)
Re: [HACKERS] Adding support for Default partition in partitioning  (Rahila Syed <rahilasyed90@gmail.com>)
List pgsql-hackers
Tree view
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)

pgsql-hackers by date:

Previous
From: Robert Haas
Date:
Subject: [HACKERS] what's up with IDENTIFIER_LOOKUP_EXPR?
Next
From: Tom Lane
Date:
Subject: Re: [HACKERS] what's up with IDENTIFIER_LOOKUP_EXPR?