Hi Rahila, IIUC, your default_partition_v3.patch is trying to implement an error if new partition is added to a table already having a default partition.
I too tried to run the test and similar to Rushabh, I see the server is crashing with the given test. However, if I reverse the order of creating the partitions, i.e. if I create a partition with list first and later create the default partition. The reason is, while defining new relation DefineRelation() checks for overlapping partitions by calling check_new_partition_bound(). Where in case of list partitions it assumes that the ndatums should be > 0, but in case of default partition that is 0. The crash here seems to be coming because, following assertion getting failed in function check_new_partition_bound(): Assert(boundinfo && boundinfo->strategy == PARTITION_STRATEGY_LIST && (boundinfo->ndatums > 0 || boundinfo->has_null)); So, I think the error you have added needs to be moved before this assertion: @@ -690,6 +715,12 @@ check_new_partition_bound(char *relname, Relation parent, Node *bound) boundinfo->strategy == PARTITION_STRATEGY_LIST && (boundinfo->ndatums > 0 || boundinfo->has_null)); + if (boundinfo->has_def) + ereport(ERROR, + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), + errmsg("parent table \"%s\" has a default partition", + RelationGetRelationName(parent)))); If I do so, the server does not run into crash, and instead throws an error: postgres=# CREATE TABLE list_partitioned ( a int ) PARTITION BY LIST (a); CREATE TABLE 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); ERROR: parent table "list_partitioned" has a default partition Regards, Jeevan Ladhe On Mon, Mar 27, 2017 at 12:10 PM, Rushabh Lathia <rushabh.lat...@gmail.com> wrote: > 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 <rahilasye...@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.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 >>> >> >> > > > -- > Rushabh Lathia >