Re: Adding support for Default partition in partitioning

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
>