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.lat...@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 <rahilasye...@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 <robertmh...@gmail.com> >> wrote: >> >>> On Fri, Mar 10, 2017 at 2:17 PM, Peter Eisentraut >>> <peter.eisentr...@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 >
default_partition_v3.patch
Description: application/download
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers