Re: Adding support for Default partition in partitioning

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(dot)ladhe(at)enterprisedb(dot)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(at)omniti(dot)com> wrote:
>
>>
>> On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed <rahilasyed90(at)gmail(dot)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
>>
>>
>