Re: Adding support for Default partition in partitioning - Mailing list pgsql-hackers

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@gmail.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.lathia@gmail.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@gmail.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@gmail.com> wrote:

On Fri, Mar 10, 2017 at 2:17 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--

Rushabh Lathia

--

Rushabh Lathia