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 CAOgcT0PH=Wo-TeqD_xJ7f=neBbST4RMeUWO3LyjbhC-CBiugLQ@mail.gmail.com
Whole thread Raw
In response to Re: [HACKERS] Adding support for Default partition in partitioning  (Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>)
List pgsql-hackers
Tree view
Hi Rahila,

I tried to review the code, and here are some of my early comments:

1.
When I configure using "-Werror", I see unused variable in function DefineRelation:

tablecmds.c: In function ‘DefineRelation’:
tablecmds.c:761:17: error: unused variable ‘partdesc’ [-Werror=unused-variable]
   PartitionDesc partdesc;
                 ^

2.
Typo in comment:
+ /*
+ * When adding a list partition after default partition, scan the
+ * default partiton for rows satisfying the new partition
+ * constraint. If found dont allow addition of a new partition.
+ * Otherwise continue with the creation of new  partition.
+ */

partition
don't

3.
I think instead of a period '.', it will be good if you can use semicolon ';'
in following declaration similar to the comment for 'null_index'.

+ int def_index; /* Index of the default list partition. -1 for
+ * range partitioned tables */

4.
You may want to consider 80 column alignment for changes done in function
get_qual_from_partbound, and other places as applicable.

5.
It would be good if the patch has some test coverage that explains what is
being achieved, what kind of error handling is done etc.

6.
There are some places having code like following:

+ Node *value = lfirst(c);
  Const   *val = lfirst(c);
  PartitionListValue *list_value = NULL;
 
+ if (IsA(value, DefElem))

The additional variable is not needed and you can call IsA on val itself.

7.
Also, in places like below where you are just trying to check for node is a
DefaultElem, you can avoid an extra variable:

+ foreach(cell1, bspec->listdatums)
+ {
+ Node *value = lfirst(cell1);
+ if (IsA(value, DefElem))
+ {
+ def_elem = true;
+ *defid = inhrelid;
+ }

Can be written as:
+ foreach(cell1, bspec->listdatums)
+ {
+ if (IsA(lfirst(cell1), DefElem))
+ {
+ def_elem = true;
+ *defid = inhrelid;
+ }
+ }


Regards,
Jeevan Ladhe



On Mon, Apr 10, 2017 at 8:12 PM, Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> wrote:

Hi Rahila,


With your latest patch:

Consider a case when a table is partitioned on a boolean key.

Even when there are existing separate partitions for 'true' and

'false', still default partition can be created.


I think this should not be allowed.


Consider following case:


postgres=# CREATE TABLE list_partitioned (               

    a bool

) PARTITION BY LIST (a);

CREATE TABLE

postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN ('false');

CREATE TABLE

postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN ('true');

CREATE TABLE

postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR VALUES IN (DEFAULT);

CREATE TABLE


The creation of table part_default should have failed instead.


Thanks,

Jeevan Ladhe



On Thu, Apr 6, 2017 at 9:37 PM, Keith Fiske <keith@omniti.com> wrote:

On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
Hello,

Thanks a lot for testing and reporting this. Please find attached an updated patch with the fix. The patch also contains a fix
regarding operator used at the time of creating expression as default partition constraint. This was notified offlist by Amit Langote.

Thank you,
Rahila Syed


Could probably use some more extensive testing, but all examples I had on my previously mentioned blog post are now working.

Keith



pgsql-hackers by date:

Previous
From: Jeevan Ladhe
Date:
Subject: Re: [HACKERS] Adding support for Default partition in partitioning
Next
From: Peter Eisentraut
Date:
Subject: [HACKERS] GCC 7 warnings