Re: Adding support for Default partition in partitioning

Hello,

>(1) With the new patch, we allow new partitions when there is overlapping
data with default partition. The entries in default are ignored when
running queries satisfying the new partition.
This was introduced in latest version. We are not allowing adding a
partition when entries with same key value exist in default partition. So
this scenario should not
come in picture. Please find attached an updated patch which corrects this.

>(2) I get the following warning:

>partition.c: In function ‘check_new_partition_bound’:
>partition.c:882:15: warning: ‘boundinfo’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
> && boundinfo->has_default)
^
>preproc.y:3250.2-8: warning: type clash on default action: <str> != <>
I failed to notice this warning. I will look into it.

>This is incredibly ugly. I don't know exactly what should be done
>about it, but I think PARTITION_DEFAULT is a bad idea and has got to
>go. Maybe add a separate isDefault flag to PartitionBoundSpec
Will look at other ways to do it.

>Doesn't it strike you as a bit strange that get_qual_for_default()
>doesn't return a qual? Functions should generally have names that
>describe what they do.
Will fix this.

>There's an existing function that you can use to concatenate two lists
>instead of open-coding it.
Will check this.

>you should really add the documentation and
>regression tests which you mentioned as a TODO. And run the code
>through pgindent
I will also update the next version with documentation and regression tests
and run pgindent

Thank you,
Rahila Syed

On Fri, May 12, 2017 at 4:33 PM, Beena Emerson <memissemerson(at)gmail(dot)com>
wrote:

>
>
> On Thu, May 11, 2017 at 7:37 PM, Rahila Syed <rahilasyed90(at)gmail(dot)com>
> wrote:
>
>> Hello,
>>
>> Please find attached an updated patch with review comments and bugs
>> reported till date implemented.
>>
>
> Hello Rahila,
>
> Tested on "efa2c18 Doc fix: scale(numeric) returns integer, not numeric."
>
> (1) With the new patch, we allow new partitions when there is overlapping
> data with default partition. The entries in default are ignored when
> running queries satisfying the new partition.
>
> DROP TABLE list1;
> CREATE TABLE list1 (
> a int,
> b int
> ) PARTITION BY LIST (a);
> CREATE TABLE list1_1 (LIKE list1);
> ALTER TABLE list1 ATTACH PARTITION list1_1 FOR VALUES IN (1);
> CREATE TABLE list1_def PARTITION OF list1 DEFAULT;
> INSERT INTO list1 SELECT generate_series(1,2),1;
> -- Partition overlapping with DEF
> CREATE TABLE list1_2 PARTITION OF list1 FOR VALUES IN (2);
> INSERT INTO list1 SELECT generate_series(2,3),2;
>
> postgres=# SELECT * FROM list1 ORDER BY a,b;
> a | b
> ---+---
> 1 | 1
> 2 | 1
> 2 | 2
> 3 | 2
> (4 rows)
>
> postgres=# SELECT * FROM list1 WHERE a=2;
> a | b
> ---+---
> 2 | 2
> (1 row)
>
> This ignores the a=2 entries in the DEFAULT.
>
> postgres=# SELECT * FROM list1_def;
> a | b
> ---+---
> 2 | 1
> 3 | 2
> (2 rows)
>
>
> (2) I get the following warning:
>
> partition.c: In function ‘check_new_partition_bound’:
> partition.c:882:15: warning: ‘boundinfo’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]
> && boundinfo->has_default)
> ^
> preproc.y:3250.2-8: warning: type clash on default action: <str> != <>
>
>
>> >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.
>> found_def is used to set boundinfo->has_default which is used at couple
>> of other places to check if default partition exists. The implementation
>> is similar
>> to has_null.
>>
>> >3.
>> >In following function isDefaultPartitionBound, first statement "return
>> false"
>> >is not needed.
>> It is needed to return false if the node is not DefElem.
>>
>> Todo:
>> Add regression tests
>> Documentation
>>
>> Thank you,
>> Rahila Syed
>>
>>
>>