Re: Adding support for Default partition in partitioning

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