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
Re: [HACKERS] Adding support for Default partition in partitioning |
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)
pgsql-hackers by date: