Re: Adding support for Default partition in partitioning

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(dot)lathia(at)gmail(dot)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 <rahilasyed90(at)gmail(dot)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(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
>>>
>>
>>
>
>
> --
> Rushabh Lathia
>