Re: Adding support for Default partition in partitioning

Hello,

On Fri, May 12, 2017 at 5:30 PM, Rahila Syed <rahilasyed90(at)gmail(dot)com> wrote:

> 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.
>

Thank you for the updated patch. However, now I cannot create a partition
after default.

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;
CREATE TABLE list1_5 PARTITION OF list1 FOR VALUES IN (3);

server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

>
>
> >(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
>>>
>>>
>>>
>

--

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company