Re: Adding support for Default partition in partitioning

I applied the patch and was trying to perform some testing, but its
ending up with server crash with the test shared by you in your starting
mail:

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

postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
(4,5);
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.

Apart from this, few more explanation in the patch is needed to explain the
changes for the DEFAULT partition. Like I am not quite sure what exactly the
latest version of patch supports, like does that support the tuple row
movement,
or adding new partition will be allowed having partition table having
DEFAULT
partition, which is quite difficult to understand through the code changes.

Another part which is missing in the patch is the test coverage, adding
proper test coverage, which explain what is supported and what's not.

Thanks,

On Fri, Mar 24, 2017 at 3:25 PM, Rahila Syed <rahilasyed90(at)gmail(dot)com> wrote:

> Hello Rushabh,
>
> Thank you for reviewing.
> Have addressed all your comments in the attached patch. The attached patch
> currently throws an
> error if a new partition is added after default partition.
>
> >Rather then adding check for default here, I think this should be handle
> inside
> >get_qual_for_list().
> Have moved the check inside get_qual_for_partbound() as needed to do some
> operations
> before calling get_qual_for_list() for default partitions.
>
> Thank you,
> Rahila Syed
>
> On Tue, Mar 21, 2017 at 11:36 AM, Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com
> > wrote:
>
>> I picked this for review and noticed that patch is not getting
>> cleanly complied on my environment.
>>
>> partition.c: In function ‘RelationBuildPartitionDesc’:
>> partition.c:269:6: error: ISO C90 forbids mixed declarations and code
>> [-Werror=declaration-after-statement]
>> Const *val = lfirst(c);
>> ^
>> partition.c: In function ‘generate_partition_qual’:
>> partition.c:1590:2: error: ISO C90 forbids mixed declarations and code
>> [-Werror=declaration-after-statement]
>> PartitionBoundSpec *spec = (PartitionBoundSpec *)bound;
>> ^
>> partition.c: In function ‘get_partition_for_tuple’:
>> partition.c:1820:5: error: array subscript has type ‘char’
>> [-Werror=char-subscripts]
>> result = parent->indexes[partdesc->boundinfo->def_index];
>> ^
>> partition.c:1824:16: error: assignment makes pointer from integer without
>> a cast [-Werror]
>> *failed_at = RelationGetRelid(parent->reldesc);
>> ^
>> cc1: all warnings being treated as errors
>>
>> Apart from this, I was reading patch here are few more comments:
>>
>> 1) Variable initializing happening at two place.
>>
>> @@ -166,6 +170,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;
>>
>> /* Range partitioning specific */
>> @@ -239,6 +245,8 @@ RelationBuildPartitionDesc(Relation rel)
>> i = 0;
>> found_null = false;
>> null_index = -1;
>> + found_def = false;
>> + def_index = -1;
>> foreach(cell, boundspecs)
>> {
>> ListCell *c;
>> @@ -249,6 +257,15 @@ RelationBuildPartitionDesc(Relation rel)
>>
>>
>> 2)
>>
>> @@ -1558,6 +1586,19 @@ generate_partition_qual(Relation rel)
>> bound = stringToNode(TextDatumGetCString(boundDatum));
>> ReleaseSysCache(tuple);
>>
>> + /* Return if it is a default list partition */
>> + PartitionBoundSpec *spec = (PartitionBoundSpec *)bound;
>> + ListCell *cell;
>> + foreach(cell, spec->listdatums)
>>
>> More comment on above hunk is needed?
>>
>> Rather then adding check for default here, I think this should be handle
>> inside
>> get_qual_for_list().
>>
>> 3) Code is not aligned with existing
>>
>> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
>> index 6316688..ebb7db7 100644
>> --- a/src/backend/parser/gram.y
>> +++ b/src/backend/parser/gram.y
>> @@ -2594,6 +2594,7 @@ partbound_datum:
>> Sconst { $$ = makeStringConst($1, @1); }
>> | NumericOnly { $$ = makeAConst($1, @1); }
>> | NULL_P { $$ = makeNullAConst(@1); }
>> + | DEFAULT { $$ = (Node *)makeDefElem("DEFAULT", NULL, @1); }
>> ;
>>
>>
>> 4) Unnecessary hunk:
>>
>> @@ -2601,7 +2602,6 @@ partbound_datum_list:
>> | partbound_datum_list ',' partbound_datum
>> { $$ = lappend($1, $3); }
>> ;
>> -
>>
>> Note: this is just an initially review comments, I am yet to do the
>> detailed review
>> and the testing for the patch.
>>
>> Thanks.
>>
>> On Mon, Mar 20, 2017 at 9:27 AM, Rahila Syed <rahilasyed90(at)gmail(dot)com>
>> wrote:
>>
>>> Hello,
>>>
>>> Please find attached a rebased patch with support for pg_dump. I am
>>> working on the patch
>>> to handle adding a new partition after a default partition by throwing
>>> an error if
>>> conflicting rows exist in default partition and adding the partition
>>> successfully otherwise.
>>> Will post an updated patch by tomorrow.
>>>
>>> Thank you,
>>> Rahila Syed
>>>
>>> On Mon, Mar 13, 2017 at 5:42 AM, Robert Haas <robertmhaas(at)gmail(dot)com>
>>> wrote:
>>>
>>>> On Fri, Mar 10, 2017 at 2:17 PM, Peter Eisentraut
>>>> <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>>>> > On 3/2/17 21:40, Robert Haas wrote:
>>>> >> On the point mentioned above, I
>>>> >> don't think adding a partition should move tuples, necessarily; seems
>>>> >> like it would be good enough - maybe better - for it to fail if there
>>>> >> are any that would need to be moved.
>>>> >
>>>> > ISTM that the uses cases of various combinations of adding a default
>>>> > partition, adding another partition after it, removing the default
>>>> > partition, clearing out the default partition in order to add more
>>>> > nondefault partitions, and so on, need to be more clearly spelled out
>>>> > for each partitioning type. We also need to consider that pg_dump and
>>>> > pg_upgrade need to be able to reproduce all those states. Seems to
>>>> be a
>>>> > bit of work still ...
>>>>
>>>> This patch is only targeting list partitioning. It is not entirely
>>>> clear that the concept makes sense for range partitioning; you can
>>>> already define a partition from the end of the last partitioning up to
>>>> infinity, or from minus-infinity up to the starting point of the first
>>>> partition. The only thing a default range partition would do is let
>>>> you do is have a single partition cover all of the ranges that would
>>>> otherwise be unassigned, which might not even be something we want.
>>>>
>>>> I don't know how complete the patch is, but the specification seems
>>>> clear enough. If you have partitions for 1, 3, and 5, you get
>>>> partition constraints of (a = 1), (a = 3), and (a = 5). If you add a
>>>> default partition, you get a constraint of (a != 1 and a != 3 and a !=
>>>> 5). If you then add a partition for 7, the default partition's
>>>> constraint becomes (a != 1 and a != 3 and a != 5 and a != 7). The
>>>> partition must be revalidated at that point for conflicting rows,
>>>> which we can either try to move to the new partition, or just error
>>>> out if there are any, depending on what we decide we want to do. I
>>>> don't think any of that requires any special handling for either
>>>> pg_dump or pg_upgrade; it all just falls out of getting the
>>>> partitioning constraints correct and consistently enforcing them, just
>>>> as for any other partition.
>>>>
>>>> --
>>>> Robert Haas
>>>> EnterpriseDB: http://www.enterprisedb.com
>>>> The Enterprise PostgreSQL Company
>>>>
>>>
>>>
>>>
>>> --
>>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>>> To make changes to your subscription:
>>> http://www.postgresql.org/mailpref/pgsql-hackers
>>>
>>>
>>
>>
>> --
>> Rushabh Lathia
>>
>
>

--
Rushabh Lathia