Thread

Commits

Allow a partitioned table to have a default partition.
- 6f6b99d1335b 11.0 landed
Adjust min/max values when changing sequence type
- 60a0b2ec8943 10.0 cited
BRIN auto-summarization
- 7526e10224f0 10.0 cited

Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-03-01T00:59:28Z

Hello,

Currently inserting the data into a partitioned table that does not fit into
any of its partitions is not allowed.

The attached patch provides a capability to add a default partition to a
list
partitioned table as follows.

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);
CREATE TABLE

postgres=# insert into list_partitioned values (9);
INSERT 0 1

postgres=# select * from part_default;
 a
---
 9
(1 row)

The attached patch is in a  preliminary stage and has following ToDos:
1. Adding pg_dump support.
2. Documentation
3. Handling adding a new partition to a partitioned table
   with default partition.
   This will require moving tuples from existing default partition to
  newly created partition if they satisfy its partition bound.
4. Handling of update of partition key in a default partition. As per
current design it should throw an error if the update requires the tuple to
be moved to any other partition. But this can changed by the following
proposal.

https://www.postgresql.org/message-id/CAJ3gD9do9o2ccQ7j7+tSgiE1REY65XRiMb=
yJO3u3QhyP8EEPQ@mail.gmail.com

I am adding it to the current commitfest with the status Waiting on Author
as I will submit an updated patch with above ToDos.
Kindly give your suggestions.

Thank you,
Rahila Syed

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-03-03T02:40:52Z

On Wed, Mar 1, 2017 at 6:29 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
> 3. Handling adding a new partition to a partitioned table
>    with default partition.
>    This will require moving tuples from existing default partition to
>   newly created partition if they satisfy its partition bound.

Considering that this patch was submitted at the last minute and isn't
even complete, I can't see this getting into v10.  But that doesn't
mean we can't talk about it.  I'm curious to hear other opinions on
whether we should have this feature.  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.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

David Fetter <david@fetter.org> — 2017-03-07T16:21:30Z

On Fri, Mar 03, 2017 at 08:10:52AM +0530, Robert Haas wrote:
> On Wed, Mar 1, 2017 at 6:29 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
> > 3. Handling adding a new partition to a partitioned table
> >    with default partition.
> >    This will require moving tuples from existing default partition to
> >   newly created partition if they satisfy its partition bound.
> 
> Considering that this patch was submitted at the last minute and isn't
> even complete, I can't see this getting into v10.  But that doesn't
> mean we can't talk about it.  I'm curious to hear other opinions on
> whether we should have this feature.  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.

I see this as a bug fix.

The current state of declarative partitions is such that you need way
too much foresight in order to use them.  Missed adding a partition?
Writes fail and can't be made to succeed.  This is not a failure mode
we should be forcing on people, especially as it's a massive
regression from the extant inheritance-based partitioning.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: Adding support for Default partition in partitioning

Keith Fiske <keith@omniti.com> — 2017-03-07T16:30:59Z

On Thu, Mar 2, 2017 at 9:40 PM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Wed, Mar 1, 2017 at 6:29 AM, Rahila Syed <rahilasyed90@gmail.com>
> wrote:
> > 3. Handling adding a new partition to a partitioned table
> >    with default partition.
> >    This will require moving tuples from existing default partition to
> >   newly created partition if they satisfy its partition bound.
>
> Considering that this patch was submitted at the last minute and isn't
> even complete, I can't see this getting into v10.  But that doesn't
> mean we can't talk about it.  I'm curious to hear other opinions on
> whether we should have this feature.  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.
>
> --
> 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
>

I'm all for this feature and had suggested it back in the original thread
to add partitioning to 10. I agree that adding a new partition should not
move any data out of the default. It's easy enough to set up a monitor to
watch for data existing in the default. Perhaps also adding a column to
pg_partitioned_table that contains the oid of the default partition so it's
easier to identify from a system catalog perspective and make that
monitoring easier. I don't even see a need for it to fail either and not
quite sure how that would even work? If they can't add a necessary child
due to data being in the default, how can they ever get it out? Just leave
it to the user to keep an eye on the default and fix it as necessary. This
is what I do in pg_partman.

--
Keith Fiske
Database Administrator
OmniTI Computer Consulting, Inc.
http://www.keithf4.com

Re: Adding support for Default partition in partitioning

Jim Nasby <jim.nasby@openscg.com> — 2017-03-10T07:12:06Z

On 3/7/17 10:30 AM, Keith Fiske wrote:
> I'm all for this feature and had suggested it back in the original

FWIW, I was working with a system just today that has an overflow partition.

> thread to add partitioning to 10. I agree that adding a new partition
> should not move any data out of the default. It's easy enough to set up

+1

> a monitor to watch for data existing in the default. Perhaps also adding
> a column to pg_partitioned_table that contains the oid of the default
> partition so it's easier to identify from a system catalog perspective
> and make that monitoring easier. I don't even see a need for it to fail

I agree that there should be a way to identify the default partition.

> either and not quite sure how that would even work? If they can't add a
> necessary child due to data being in the default, how can they ever get
> it out?

Yeah, was wondering that as well...
-- 
Jim Nasby, Chief Data Architect, OpenSCG
http://OpenSCG.com

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-03-10T13:04:44Z

 >I agree that adding a new partition should not move any data out of the
default. It's easy enough to set up a monitor to watch for data existing in
the >default. Perhaps also adding a column to pg_partitioned_table that
contains the oid of the default partition so it's easier to identify from a
system >catalog perspective and make that monitoring easier.

Wont it incur overhead of scanning the default partition for matching rows
each time a select happens on any matching partition?
This extra scan would be required until rows satisfying the newly added
partition are left around in default partition.

>I don't even see a need for it to fail either and not quite sure how that
would even work? If they can't add a necessary child due to data being in
the >default, how can they ever get it out? Just leave it to the user to
keep an eye on the default and fix it as necessary.
This patch intends to provide a way to insert data that does not satisfy
any of the existing partitions. For this patch, we can disallow adding a
new partition when a default partition with conflicting rows exist. There
can be many solutions to the problem of adding a new partition. One is to
move the relevant tuples from default to the new partition or like you
suggest keep monitoring the default partition until user moves the rows out
of the default.

Thank you,
Rahila Syed

On Tue, Mar 7, 2017 at 10:00 PM, Keith Fiske <keith@omniti.com> wrote:

> On Thu, Mar 2, 2017 at 9:40 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
>> On Wed, Mar 1, 2017 at 6:29 AM, Rahila Syed <rahilasyed90@gmail.com>
>> wrote:
>> > 3. Handling adding a new partition to a partitioned table
>> >    with default partition.
>> >    This will require moving tuples from existing default partition to
>> >   newly created partition if they satisfy its partition bound.
>>
>> Considering that this patch was submitted at the last minute and isn't
>> even complete, I can't see this getting into v10.  But that doesn't
>> mean we can't talk about it.  I'm curious to hear other opinions on
>> whether we should have this feature.  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.
>>
>> --
>> 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
>>
>
> I'm all for this feature and had suggested it back in the original thread
> to add partitioning to 10. I agree that adding a new partition should not
> move any data out of the default. It's easy enough to set up a monitor to
> watch for data existing in the default. Perhaps also adding a column to
> pg_partitioned_table that contains the oid of the default partition so it's
> easier to identify from a system catalog perspective and make that
> monitoring easier. I don't even see a need for it to fail either and not
> quite sure how that would even work? If they can't add a necessary child
> due to data being in the default, how can they ever get it out? Just leave
> it to the user to keep an eye on the default and fix it as necessary. This
> is what I do in pg_partman.
>
> --
> Keith Fiske
> Database Administrator
> OmniTI Computer Consulting, Inc.
> http://www.keithf4.com
>

Re: Adding support for Default partition in partitioning

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-03-10T19:17:43Z

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 ...

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-03-13T00:12:05Z

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

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-03-20T03:57:05Z

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
>

Re: Adding support for Default partition in partitioning

Rushabh Lathia <rushabh.lathia@gmail.com> — 2017-03-21T06:06:00Z

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

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-03-24T09:55:02Z

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
>

Re: Adding support for Default partition in partitioning

Rushabh Lathia <rushabh.lathia@gmail.com> — 2017-03-27T06:40:23Z

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

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-03-27T11:06:01Z

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.lathia@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 <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
>

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-03-29T12:13:59Z

Thanks for reporting. I have identified the problem and have a fix.
Currently working on allowing
adding a partition after default partition if the default partition does
not have any conflicting rows.
Will update the patch with both of these.

Thank you,
Rahila Syed

On Mon, Mar 27, 2017 at 12:10 PM, Rushabh Lathia <rushabh.lathia@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 <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
>

Re: Adding support for Default partition in partitioning

David Steele <david@pgmasters.net> — 2017-03-31T14:45:36Z

On 3/29/17 8:13 AM, Rahila Syed wrote:

> Thanks for reporting. I have identified the problem and have a fix.
> Currently working on allowing
> adding a partition after default partition if the default partition does
> not have any conflicting rows.
> Will update the patch with both of these.

The CF has been extended but until April 7 but time is still growing 
short.  Please respond with a new patch by 2017-04-04 00:00 AoE (UTC-12) 
or this submission will be marked "Returned with Feedback".

Thanks,
-- 
-David
david@pgmasters.net

Re: Adding support for Default partition in partitioning

David Steele <david@pgmasters.net> — 2017-04-04T12:52:00Z

On 3/31/17 10:45 AM, David Steele wrote:
> On 3/29/17 8:13 AM, Rahila Syed wrote:
> 
>> Thanks for reporting. I have identified the problem and have a fix.
>> Currently working on allowing
>> adding a partition after default partition if the default partition does
>> not have any conflicting rows.
>> Will update the patch with both of these.
> 
> The CF has been extended but until April 7 but time is still growing
> short.  Please respond with a new patch by 2017-04-04 00:00 AoE (UTC-12)
> or this submission will be marked "Returned with Feedback".

This submission has been marked "Returned with Feedback".  Please feel
free to resubmit to a future commitfest.

Regards,
-- 
-David
david@pgmasters.net

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-04-04T13:30:21Z

Hello,

Please find attached an updated patch.
Following has been accomplished in this update:

1. A new partition can be added after default partition if there are no
conflicting rows in default partition.
2. Solved the crash reported earlier.

Thank you,
Rahila Syed



On Tue, Apr 4, 2017 at 6:22 PM, David Steele <david@pgmasters.net> wrote:

> On 3/31/17 10:45 AM, David Steele wrote:
> > On 3/29/17 8:13 AM, Rahila Syed wrote:
> >
> >> Thanks for reporting. I have identified the problem and have a fix.
> >> Currently working on allowing
> >> adding a partition after default partition if the default partition does
> >> not have any conflicting rows.
> >> Will update the patch with both of these.
> >
> > The CF has been extended but until April 7 but time is still growing
> > short.  Please respond with a new patch by 2017-04-04 00:00 AoE (UTC-12)
> > or this submission will be marked "Returned with Feedback".
>
> This submission has been marked "Returned with Feedback".  Please feel
> free to resubmit to a future commitfest.
>
> Regards,
> --
> -David
> david@pgmasters.net
>

Re: Adding support for Default partition in partitioning

Keith Fiske <keith@omniti.com> — 2017-04-04T21:22:14Z

On Tue, Apr 4, 2017 at 9:30 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:

> Hello,
>
> Please find attached an updated patch.
> Following has been accomplished in this update:
>
> 1. A new partition can be added after default partition if there are no
> conflicting rows in default partition.
> 2. Solved the crash reported earlier.
>
> Thank you,
> Rahila Syed
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>
Installed and compiled against commit
60a0b2ec8943451186dfa22907f88334d97cb2e0 (Date: Tue Apr 4 12:36:15 2017
-0400) without any issue

However, running your original example, I'm getting this error

keith@keith=# CREATE TABLE list_partitioned (
keith(#     a int
keith(# ) PARTITION BY LIST (a);
CREATE TABLE
Time: 4.933 ms
keith@keith=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
VALUES IN (DEFAULT);
CREATE TABLE
Time: 3.492 ms
keith@keith=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES
IN (4,5);
ERROR:  unrecognized node type: 216
Time: 0.979 ms

Also, I'm still of the opinion that denying future partitions of values in
the default would be a cause of confusion. In order to move the data out of
the default and into a proper child it would require first removing that
data from the default, storing it elsewhere, creating the child, then
moving it back. If it's only a small amount of data it may not be a big
deal, but if it's a large amount, that could cause quite a lot of
contention if done in a single transaction. Either that or the user would
have to deal with data existing in the table, disappearing, then
reappearing again.

This also makes it harder to migrate an existing table easily. Essentially
no child tables for a large, existing data set could ever be created
without going through one of the two situations above.

However, thinking through this, I'm not sure I can see a solution without
the global index support. If this restriction is removed, there's still an
issue of data duplication after the necessary child table is added. So
guess it's a matter of deciding which user experience is better for the
moment?

--
Keith Fiske
Database Administrator
OmniTI Computer Consulting, Inc.
http://www.keithf4.com

Re: Adding support for Default partition in partitioning

Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-04-05T05:29:08Z

On 2017/04/05 6:22, Keith Fiske wrote:
> On Tue, Apr 4, 2017 at 9:30 AM, Rahila Syed wrote:
>> Please find attached an updated patch.
>> Following has been accomplished in this update:
>>
>> 1. A new partition can be added after default partition if there are no
>> conflicting rows in default partition.
>> 2. Solved the crash reported earlier.
>
> Installed and compiled against commit
> 60a0b2ec8943451186dfa22907f88334d97cb2e0 (Date: Tue Apr 4 12:36:15 2017
> -0400) without any issue
> 
> However, running your original example, I'm getting this error
> 
> keith@keith=# CREATE TABLE list_partitioned (
> keith(#     a int
> keith(# ) PARTITION BY LIST (a);
> CREATE TABLE
> Time: 4.933 ms
> keith@keith=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
> VALUES IN (DEFAULT);
> CREATE TABLE
> Time: 3.492 ms
> keith@keith=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES
> IN (4,5);
> ERROR:  unrecognized node type: 216

It seems like the new ExecPrepareCheck should be used in the place of
ExecPrepareExpr in the code added in check_new_partition_bound().

> Also, I'm still of the opinion that denying future partitions of values in
> the default would be a cause of confusion. In order to move the data out of
> the default and into a proper child it would require first removing that
> data from the default, storing it elsewhere, creating the child, then
> moving it back. If it's only a small amount of data it may not be a big
> deal, but if it's a large amount, that could cause quite a lot of
> contention if done in a single transaction. Either that or the user would
> have to deal with data existing in the table, disappearing, then
> reappearing again.
>
> This also makes it harder to migrate an existing table easily. Essentially
> no child tables for a large, existing data set could ever be created
> without going through one of the two situations above.

I thought of the following possible way to allow future partitions when
the default partition exists which might contain rows that belong to the
newly created partition (unfortunately, nothing that we could implement at
this point for v10):

Suppose you want to add a new partition which will accept key=3 rows.

1. If no default partition exists, we're done; no key=3 rows would have
   been accepted by any of the table's existing partitions, so no need to
   move any rows

2. Default partition exists which might contain key=3 rows, which we'll
   need to move.  If we do this in the same transaction, as you say, it
   might result in unnecessary unavailability of table's data.  So, it's
   better to delegate that responsibility to a background process.  The
   current transaction will only add the new key=3 partition, so any key=3
   rows will be routed to the new partition from this point on.  But we
   haven't updated the default partition's constraint yet to say that it
   no longer contains key=3 rows (constraint that the planner consumes),
   so it will continue to be scanned for queries that request key=3 rows
   (there should be some metadata to indicate that the default partition's
   constraint is invalid), along with the new partition.

3. A background process receives a "work item" requesting it to move all
   key=3 rows from the default partition heap to the new partition's heap.
   The movement occurs without causing the table to become unavailable;
   once all rows have been moved, we momentarily lock the table to update
   the default partition's constraint to mark it valid, so that it will
   no longer be accessed by queries that want to see key=3 rows.

Regarding 2, there is a question of whether it should not be possible for
the row movement to occur in the same transaction.  Somebody may want that
to happen because they chose to run the command during a maintenance
window, when the table's becoming unavailable is not an issue.  In that
case, we need to think of the interface more carefully.

Regarding 3, I think the new autovacuum work items infrastructure added by
the following commit looks very promising:

* BRIN auto-summarization *
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=7526e10224f0792201e99631567bbe44492bbde4

> However, thinking through this, I'm not sure I can see a solution without
> the global index support. If this restriction is removed, there's still an
> issue of data duplication after the necessary child table is added. So
> guess it's a matter of deciding which user experience is better for the
> moment?

I'm not sure about the fate of this particular patch for v10, but until we
implement a solution to move rows and design appropriate interface for the
same, we could error out if moving rows is required at all, like the patch
does.

Could you briefly elaborate why you think the lack global index support
would be a problem in this regard?

I agree that some design is required here to implement a solution
redistribution of rows; not only in the context of supporting the notion
of default partitions, but also to allow the feature to split/merge range
(only?) partitions.  I'd like to work on the latter for v11 for which I
would like to post a proposal soon; if anyone would like to collaborate
(ideas, code, review), I look forward to.  (sorry for hijacking this thread.)

Thanks,
Amit

Re: Adding support for Default partition in partitioning

Rushabh Lathia <rushabh.lathia@gmail.com> — 2017-04-05T05:41:50Z

On Wed, Apr 5, 2017 at 10:59 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp
> wrote:

> On 2017/04/05 6:22, Keith Fiske wrote:
> > On Tue, Apr 4, 2017 at 9:30 AM, Rahila Syed wrote:
> >> Please find attached an updated patch.
> >> Following has been accomplished in this update:
> >>
> >> 1. A new partition can be added after default partition if there are no
> >> conflicting rows in default partition.
> >> 2. Solved the crash reported earlier.
> >
> > Installed and compiled against commit
> > 60a0b2ec8943451186dfa22907f88334d97cb2e0 (Date: Tue Apr 4 12:36:15 2017
> > -0400) without any issue
> >
> > However, running your original example, I'm getting this error
> >
> > keith@keith=# CREATE TABLE list_partitioned (
> > keith(#     a int
> > keith(# ) PARTITION BY LIST (a);
> > CREATE TABLE
> > Time: 4.933 ms
> > keith@keith=# CREATE TABLE part_default PARTITION OF list_partitioned
> FOR
> > VALUES IN (DEFAULT);
> > CREATE TABLE
> > Time: 3.492 ms
> > keith@keith=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR
> VALUES
> > IN (4,5);
> > ERROR:  unrecognized node type: 216
>
> It seems like the new ExecPrepareCheck should be used in the place of
> ExecPrepareExpr in the code added in check_new_partition_bound().
>
> > Also, I'm still of the opinion that denying future partitions of values
> in
> > the default would be a cause of confusion. In order to move the data out
> of
> > the default and into a proper child it would require first removing that
> > data from the default, storing it elsewhere, creating the child, then
> > moving it back. If it's only a small amount of data it may not be a big
> > deal, but if it's a large amount, that could cause quite a lot of
> > contention if done in a single transaction. Either that or the user would
> > have to deal with data existing in the table, disappearing, then
> > reappearing again.
> >
> > This also makes it harder to migrate an existing table easily.
> Essentially
> > no child tables for a large, existing data set could ever be created
> > without going through one of the two situations above.
>
> I thought of the following possible way to allow future partitions when
> the default partition exists which might contain rows that belong to the
> newly created partition (unfortunately, nothing that we could implement at
> this point for v10):
>
> Suppose you want to add a new partition which will accept key=3 rows.
>
> 1. If no default partition exists, we're done; no key=3 rows would have
>    been accepted by any of the table's existing partitions, so no need to
>    move any rows
>
> 2. Default partition exists which might contain key=3 rows, which we'll
>    need to move.  If we do this in the same transaction, as you say, it
>    might result in unnecessary unavailability of table's data.  So, it's
>    better to delegate that responsibility to a background process.  The
>    current transaction will only add the new key=3 partition, so any key=3
>    rows will be routed to the new partition from this point on.  But we
>    haven't updated the default partition's constraint yet to say that it
>    no longer contains key=3 rows (constraint that the planner consumes),
>    so it will continue to be scanned for queries that request key=3 rows
>    (there should be some metadata to indicate that the default partition's
>    constraint is invalid), along with the new partition.
>
> 3. A background process receives a "work item" requesting it to move all
>    key=3 rows from the default partition heap to the new partition's heap.
>    The movement occurs without causing the table to become unavailable;
>    once all rows have been moved, we momentarily lock the table to update
>    the default partition's constraint to mark it valid, so that it will
>    no longer be accessed by queries that want to see key=3 rows.
>
> Regarding 2, there is a question of whether it should not be possible for
> the row movement to occur in the same transaction.  Somebody may want that
> to happen because they chose to run the command during a maintenance
> window, when the table's becoming unavailable is not an issue.  In that
> case, we need to think of the interface more carefully.
>
> Regarding 3, I think the new autovacuum work items infrastructure added by
> the following commit looks very promising:
>
> * BRIN auto-summarization *
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=
> 7526e10224f0792201e99631567bbe44492bbde4
>
> > However, thinking through this, I'm not sure I can see a solution without
> > the global index support. If this restriction is removed, there's still
> an
> > issue of data duplication after the necessary child table is added. So
> > guess it's a matter of deciding which user experience is better for the
> > moment?
>
> I'm not sure about the fate of this particular patch for v10, but until we
> implement a solution to move rows and design appropriate interface for the
> same, we could error out if moving rows is required at all, like the patch
> does.
>
>
+1

I agree about the future plan about the row movement, how that is I am
not quite sure at this stage.

I was thinking that CREATE new partition is the DDL command, so even
if row-movement works with holding the lock on the new partition table,
that should be fine. I am not quire sure, why row movement should be
happen in the back-ground process.

Of-course, one idea is that if someone don't want feature of row-movement,
then we might add that under some GUC or may be as another option into
the CREATE partition table.

Could you briefly elaborate why you think the lack global index support
> would be a problem in this regard?
>
> I agree that some design is required here to implement a solution
> redistribution of rows; not only in the context of supporting the notion
> of default partitions, but also to allow the feature to split/merge range
> (only?) partitions.  I'd like to work on the latter for v11 for which I
> would like to post a proposal soon; if anyone would like to collaborate
> (ideas, code, review), I look forward to.  (sorry for hijacking this
> thread.)
>
> Thanks,
> Amit
>
>
>


-- 
Rushabh Lathia

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-04-05T07:28:50Z

Hi,

>However, running your original example, I'm getting this error
Thank you for testing. Please find attached an updated patch which fixes
the above.


Thank you,
Rahila Syed

Re: Adding support for Default partition in partitioning

Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-04-05T07:52:30Z

On 2017/04/05 14:41, Rushabh Lathia wrote:
> I agree about the future plan about the row movement, how that is I am
> not quite sure at this stage.
> 
> I was thinking that CREATE new partition is the DDL command, so even
> if row-movement works with holding the lock on the new partition table,
> that should be fine. I am not quire sure, why row movement should be
> happen in the back-ground process.

I think to improve the availability of access to the partitioned table.

Consider that the default partition may have gotten pretty large.
Scanning it and moving rows to the newly added partition while holding an
AccessExclusiveLock on the parent will block any and all of the concurrent
activity on it until the row-movement is finished.  One may be prepared to
pay this cost, for which there should definitely be an option to perform
the row-movement in the same transaction (also possibly the default behavior).

Thanks,
Amit

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-04-05T09:57:11Z

Hello Amit,

>Could you briefly elaborate why you think the lack global index support
>would be a problem in this regard?
I think following can happen if we allow rows satisfying the new partition
to lie around in the
default partition until background process moves it.
Consider a scenario where partition key is a primary key and the data in
the default partition is
not yet moved into the newly added partition. If now, new data is added
into the new partition
which also exists(same key) in default partition there will be data
duplication. If now
we scan the partitioned table for that key(from both the default and new
partition as we
have not moved the rows) it will fetch the both rows.
Unless we have global indexes for partitioned tables, there is chance of
data duplication between
child table added after default partition and the default partition.

>Scanning it and moving rows to the newly added partition while holding an
>AccessExclusiveLock on the parent will block any and all of the concurrent
>activity on it until the row-movement is finished.
Can you explain why this will require AccessExclusiveLock on parent and
not just the default partition and newly added partition?

Thank you,
Rahila Syed


On Wed, Apr 5, 2017 at 1:22 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
wrote:

> On 2017/04/05 14:41, Rushabh Lathia wrote:
> > I agree about the future plan about the row movement, how that is I am
> > not quite sure at this stage.
> >
> > I was thinking that CREATE new partition is the DDL command, so even
> > if row-movement works with holding the lock on the new partition table,
> > that should be fine. I am not quire sure, why row movement should be
> > happen in the back-ground process.
>
> I think to improve the availability of access to the partitioned table.
>
> Consider that the default partition may have gotten pretty large.
> Scanning it and moving rows to the newly added partition while holding an
> AccessExclusiveLock on the parent will block any and all of the concurrent
> activity on it until the row-movement is finished.  One may be prepared to
> pay this cost, for which there should definitely be an option to perform
> the row-movement in the same transaction (also possibly the default
> behavior).
>
> Thanks,
> Amit
>
>
>

Re: Adding support for Default partition in partitioning

Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-04-05T10:23:35Z

Hi Rahila,

On 2017/04/05 18:57, Rahila Syed wrote:
> Hello Amit,
> 
>> Could you briefly elaborate why you think the lack global index support
>> would be a problem in this regard?
> I think following can happen if we allow rows satisfying the new partition
> to lie around in the
> default partition until background process moves it.
> Consider a scenario where partition key is a primary key and the data in
> the default partition is
> not yet moved into the newly added partition. If now, new data is added
> into the new partition
> which also exists(same key) in default partition there will be data
> duplication. If now
> we scan the partitioned table for that key(from both the default and new
> partition as we
> have not moved the rows) it will fetch the both rows.
> Unless we have global indexes for partitioned tables, there is chance of
> data duplication between
> child table added after default partition and the default partition.

Ah, okay.  I think I wrote that question before even reading the next
sentence in Keith's message ("there's still an issue of data duplication
after the necessary child table is added.")

Maybe we can disallow background row movement if such global constraint
exists.

>> Scanning it and moving rows to the newly added partition while holding an
>> AccessExclusiveLock on the parent will block any and all of the concurrent
>> activity on it until the row-movement is finished.
> Can you explain why this will require AccessExclusiveLock on parent and
> not just the default partition and newly added partition?

Because we take an AccessExclusiveLock on the parent table when
adding/removing a partition in general.  We do that because concurrent
accessors of the parent table rely on its partition descriptor from not
changing under them.

Thanks,
Amit

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-04-05T15:19:31Z

On Wed, Apr 5, 2017 at 5:57 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>>Could you briefly elaborate why you think the lack global index support
>>would be a problem in this regard?
> I think following can happen if we allow rows satisfying the new partition
> to lie around in the
> default partition until background process moves it.
> Consider a scenario where partition key is a primary key and the data in the
> default partition is
> not yet moved into the newly added partition. If now, new data is added into
> the new partition
> which also exists(same key) in default partition there will be data
> duplication. If now
> we scan the partitioned table for that key(from both the default and new
> partition as we
> have not moved the rows) it will fetch the both rows.
> Unless we have global indexes for partitioned tables, there is chance of
> data duplication between
> child table added after default partition and the default partition.

Yes, I think it would be completely crazy to try to migrate the data
in the background:

- The migration might never complete because of a UNIQUE or CHECK
constraint on the partition to which rows are being migrated.

- Even if the migration eventually succeeded, such a design abandons
all hope of making INSERT .. ON CONFLICT DO NOTHING work sensibly
while the migration is in progress, unless the new partition has no
UNIQUE constraints.

- Partition-wise join and partition-wise aggregate would need to have
special case handling for the case of an unfinished migration, as
would any user code that accesses partitions directly.

- More generally, I think users expect that when a DDL command
finishes execution, it's done all of the work that there is to do (or
at the very least, that any remaining work has no user-visible
consequences, which would not be the case here).

IMV, the question of whether we have efficient ways to move data
around between partitions is somewhat separate from the question of
whether partitions can be defined in a certain way in the first place.
The problems that Keith refers to upthread already exist for
subpartitioning; you've got to detach the old partition, create a new
one, and then reinsert the data.  And for partitioning an
unpartitioned table: create a replacement table, insert all the data,
substitute it for the original table.  The fact that we have these
limitation is not good, but we're not going to rip out partitioning
entirely because we don't have clever ways of migrating the data in
those cases, and the proposed behavior here is not any worse.

Also, waiting for those problems to get fixed might be waiting for
Godot.  I'm not really all that sanguine about our chances of coming
up with a really nice way of handling these cases.  In a designed
based on table inheritance, you can leave it murky where certain data
is supposed to end up and migrate it on-line and you might get away
with that, but a major point of having declarative partitioning at all
is to remove that sort of murkiness.  It's probably not that hard to
come up with a command that locks the parent and moves data around via
full table scans, but I'm not sure how far that really gets us; you
could do the same thing easily enough with a sequence of commands
generated via a script.  And being able to do this in a general way
without a full table lock looks pretty hard - it doesn't seem
fundamentally different from trying to perform a table-rewriting
operation like CLUSTER without a full table lock, which we also don't
support.  The executor is not built to cope with any aspect of the
table definition shifting under it, and that includes the set of child
tables with are partitions of the table mentioned in the query.  Maybe
the executor can be taught to survive such definitional changes at
least in limited cases, but that's a much different project than
allowing default partitions.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Keith Fiske <keith@omniti.com> — 2017-04-05T18:51:32Z

On Wed, Apr 5, 2017 at 11:19 AM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Wed, Apr 5, 2017 at 5:57 AM, Rahila Syed <rahilasyed90@gmail.com>
> wrote:
> >>Could you briefly elaborate why you think the lack global index support
> >>would be a problem in this regard?
> > I think following can happen if we allow rows satisfying the new
> partition
> > to lie around in the
> > default partition until background process moves it.
> > Consider a scenario where partition key is a primary key and the data in
> the
> > default partition is
> > not yet moved into the newly added partition. If now, new data is added
> into
> > the new partition
> > which also exists(same key) in default partition there will be data
> > duplication. If now
> > we scan the partitioned table for that key(from both the default and new
> > partition as we
> > have not moved the rows) it will fetch the both rows.
> > Unless we have global indexes for partitioned tables, there is chance of
> > data duplication between
> > child table added after default partition and the default partition.
>
> Yes, I think it would be completely crazy to try to migrate the data
> in the background:
>
> - The migration might never complete because of a UNIQUE or CHECK
> constraint on the partition to which rows are being migrated.
>
> - Even if the migration eventually succeeded, such a design abandons
> all hope of making INSERT .. ON CONFLICT DO NOTHING work sensibly
> while the migration is in progress, unless the new partition has no
> UNIQUE constraints.
>
> - Partition-wise join and partition-wise aggregate would need to have
> special case handling for the case of an unfinished migration, as
> would any user code that accesses partitions directly.
>
> - More generally, I think users expect that when a DDL command
> finishes execution, it's done all of the work that there is to do (or
> at the very least, that any remaining work has no user-visible
> consequences, which would not be the case here).
>
> IMV, the question of whether we have efficient ways to move data
> around between partitions is somewhat separate from the question of
> whether partitions can be defined in a certain way in the first place.
> The problems that Keith refers to upthread already exist for
> subpartitioning; you've got to detach the old partition, create a new
> one, and then reinsert the data.  And for partitioning an
> unpartitioned table: create a replacement table, insert all the data,
> substitute it for the original table.  The fact that we have these
> limitation is not good, but we're not going to rip out partitioning
> entirely because we don't have clever ways of migrating the data in
> those cases, and the proposed behavior here is not any worse.
>
> Also, waiting for those problems to get fixed might be waiting for
> Godot.  I'm not really all that sanguine about our chances of coming
> up with a really nice way of handling these cases.  In a designed
> based on table inheritance, you can leave it murky where certain data
> is supposed to end up and migrate it on-line and you might get away
> with that, but a major point of having declarative partitioning at all
> is to remove that sort of murkiness.  It's probably not that hard to
> come up with a command that locks the parent and moves data around via
> full table scans, but I'm not sure how far that really gets us; you
> could do the same thing easily enough with a sequence of commands
> generated via a script.  And being able to do this in a general way
> without a full table lock looks pretty hard - it doesn't seem
> fundamentally different from trying to perform a table-rewriting
> operation like CLUSTER without a full table lock, which we also don't
> support.  The executor is not built to cope with any aspect of the
> table definition shifting under it, and that includes the set of child
> tables with are partitions of the table mentioned in the query.  Maybe
> the executor can be taught to survive such definitional changes at
> least in limited cases, but that's a much different project than
> allowing default partitions.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Confirmed that v5 patch works with examples given in the original post but
segfaulted when I tried the examples I used in my blog post (taken from the
documentation at the time I wrote it).
https://www.keithf4.com/postgresql-10-built-in-partitioning/

keith@keith=# drop table cities;
DROP TABLE
Time: 6.055 ms
keith@keith=# CREATE TABLE cities (
    city_id         bigserial not null,
    name         text not null,
    population   int
) PARTITION BY LIST (initcap(name));
CREATE TABLE
Time: 7.130 ms
keith@keith=# CREATE TABLE cities_west
    PARTITION OF cities (
    CONSTRAINT city_id_nonzero CHECK (city_id != 0)
) FOR VALUES IN ('Los Angeles', 'San Francisco');
CREATE TABLE
Time: 6.690 ms
keith@keith=# CREATE TABLE cities_default
keith-#     PARTITION OF cities FOR VALUES IN (DEFAULT);
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: WARNING:
terminating connection because of crash of another server process
DETAIL:  The postmaster has commanded this server process to roll back the
current transaction and exit, because another server process exited
abnormally and possibly corrupted shared memory.
HINT:  In a moment you should be able to reconnect to the database and
repeat your command.
Failed.
Time: 387.887 ms

After reading responses, I think I'd be fine with how Rahila implemented
this with disallowing the child until the data is removed from the default
if this would allow it to be included in v10. As was mentioned, there just
doesn't seem to be a way to easily handle the data conflicts cleanly at
this time, but I think the value of the default to be able to catch
accidental data vs returning an error is worth it. It also at least gives a
slightly easier migration path vs having to migrate to a completely new
table. Any chance this could be adapted for range partitioning as well? I'd
be happy to create some pgtap tests with pg_partman for this then to make
sure it works.

Only issue I see with this, and I'm not sure if it is an issue, is what
happens to that default constraint clause when 1000s of partitions start
getting added? From what I gather the default's constraint is built based
off the cumulative opposite of all other child constraints. I don't
understand the code well enough to see what it's actually doing, but if
there are no gaps, is the method used smart enough to aggregate all the
child constraints to make a simpler constraint that is simply outside the
current min/max boundaries? If so, for serial/time range partitioning this
should typically work out fine since there are rarely gaps. This actually
seems more of an issue for list partitioning where each child is a distinct
value or range of values that are completely arbitrary. Won't that check
and re-evaluation of the default's constraint just get worse and worse as
more children are added? Is there really even a need for the default to
have an opposite constraint like this? Not sure on how the planner works
with partitioning now, but wouldn't it be better to first check all
non-default children for a match the same as it does now without a default
and, failing that, then route to the default if one is declared? The
default should accept any data then so I don't see the need for the
constraint unless it's required for the current implementation. If that's
the case, could that be changed?

Keith

Re: Adding support for Default partition in partitioning

Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-04-06T01:10:19Z

On 2017/04/06 0:19, Robert Haas wrote:
> On Wed, Apr 5, 2017 at 5:57 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>>> Could you briefly elaborate why you think the lack global index support
>>> would be a problem in this regard?
>> I think following can happen if we allow rows satisfying the new partition
>> to lie around in the
>> default partition until background process moves it.
>> Consider a scenario where partition key is a primary key and the data in the
>> default partition is
>> not yet moved into the newly added partition. If now, new data is added into
>> the new partition
>> which also exists(same key) in default partition there will be data
>> duplication. If now
>> we scan the partitioned table for that key(from both the default and new
>> partition as we
>> have not moved the rows) it will fetch the both rows.
>> Unless we have global indexes for partitioned tables, there is chance of
>> data duplication between
>> child table added after default partition and the default partition.
> 
> Yes, I think it would be completely crazy to try to migrate the data
> in the background:
> 
> - The migration might never complete because of a UNIQUE or CHECK
> constraint on the partition to which rows are being migrated.
> 
> - Even if the migration eventually succeeded, such a design abandons
> all hope of making INSERT .. ON CONFLICT DO NOTHING work sensibly
> while the migration is in progress, unless the new partition has no
> UNIQUE constraints.
> 
> - Partition-wise join and partition-wise aggregate would need to have
> special case handling for the case of an unfinished migration, as
> would any user code that accesses partitions directly.
> 
> - More generally, I think users expect that when a DDL command
> finishes execution, it's done all of the work that there is to do (or
> at the very least, that any remaining work has no user-visible
> consequences, which would not be the case here).

OK, I realize the background migration was a poorly thought out idea.  And
a *first* version that will handle the row-movement should be doing that
as part of the same command anyway.

> IMV, the question of whether we have efficient ways to move data
> around between partitions is somewhat separate from the question of
> whether partitions can be defined in a certain way in the first place.
> The problems that Keith refers to upthread already exist for
> subpartitioning; you've got to detach the old partition, create a new
> one, and then reinsert the data.  And for partitioning an
> unpartitioned table: create a replacement table, insert all the data,
> substitute it for the original table.  The fact that we have these
> limitation is not good, but we're not going to rip out partitioning
> entirely because we don't have clever ways of migrating the data in
> those cases, and the proposed behavior here is not any worse.
>
> Also, waiting for those problems to get fixed might be waiting for
> Godot.  I'm not really all that sanguine about our chances of coming
> up with a really nice way of handling these cases.  In a designed
> based on table inheritance, you can leave it murky where certain data
> is supposed to end up and migrate it on-line and you might get away
> with that, but a major point of having declarative partitioning at all
> is to remove that sort of murkiness.  It's probably not that hard to
> come up with a command that locks the parent and moves data around via
> full table scans, but I'm not sure how far that really gets us; you
> could do the same thing easily enough with a sequence of commands
> generated via a script.  And being able to do this in a general way
> without a full table lock looks pretty hard - it doesn't seem
> fundamentally different from trying to perform a table-rewriting
> operation like CLUSTER without a full table lock, which we also don't
> support.  The executor is not built to cope with any aspect of the
> table definition shifting under it, and that includes the set of child
> tables with are partitions of the table mentioned in the query.  Maybe
> the executor can be taught to survive such definitional changes at
> least in limited cases, but that's a much different project than
> allowing default partitions.

Agreed.

Thanks,
Amit

Re: Adding support for Default partition in partitioning

Keith Fiske <keith@omniti.com> — 2017-04-06T04:08:36Z

On Wed, Apr 5, 2017 at 2:51 PM, Keith Fiske <keith@omniti.com> wrote:

>
>
> Only issue I see with this, and I'm not sure if it is an issue, is what
> happens to that default constraint clause when 1000s of partitions start
> getting added? From what I gather the default's constraint is built based
> off the cumulative opposite of all other child constraints. I don't
> understand the code well enough to see what it's actually doing, but if
> there are no gaps, is the method used smart enough to aggregate all the
> child constraints to make a simpler constraint that is simply outside the
> current min/max boundaries? If so, for serial/time range partitioning this
> should typically work out fine since there are rarely gaps. This actually
> seems more of an issue for list partitioning where each child is a distinct
> value or range of values that are completely arbitrary. Won't that check
> and re-evaluation of the default's constraint just get worse and worse as
> more children are added? Is there really even a need for the default to
> have an opposite constraint like this? Not sure on how the planner works
> with partitioning now, but wouldn't it be better to first check all
> non-default children for a match the same as it does now without a default
> and, failing that, then route to the default if one is declared? The
> default should accept any data then so I don't see the need for the
> constraint unless it's required for the current implementation. If that's
> the case, could that be changed?
>
> Keith
>

Actually, thinking on this more, I realized this does again come back to
the lack of a global index. Without the constraint, data could be put
directly into the default that could technically conflict with the
partition scheme elsewhere. Perhaps, instead of the constraint, inserts
directly to the default could be prevented on the user level. Writing to
valid children directly certainly has its place, but been thinking about
it, and I can't see any reason why one would ever want to write directly to
the default. It's use case seems to be around being a sort of temporary
storage until that data can be moved to a valid location. Would still need
to allow removal of data, though.

Not sure if that's even a workable solution. Just trying to think of ways
around the current limitations and still allow this feature.

Re: Adding support for Default partition in partitioning

Rushabh Lathia <rushabh.lathia@gmail.com> — 2017-04-06T05:17:13Z

On 2017/04/06 0:19, Robert Haas wrote:
> On Wed, Apr 5, 2017 at 5:57 AM, Rahila Syed <rahilasyed90@gmail.com>
wrote:
>>> Could you briefly elaborate why you think the lack global index support
>>> would be a problem in this regard?
>> I think following can happen if we allow rows satisfying the new
partition
>> to lie around in the
>> default partition until background process moves it.
>> Consider a scenario where partition key is a primary key and the data in
the
>> default partition is
>> not yet moved into the newly added partition. If now, new data is added
into
>> the new partition
>> which also exists(same key) in default partition there will be data
>> duplication. If now
>> we scan the partitioned table for that key(from both the default and new
>> partition as we
>> have not moved the rows) it will fetch the both rows.
>> Unless we have global indexes for partitioned tables, there is chance of
>> data duplication between
>> child table added after default partition and the default partition.
>
> Yes, I think it would be completely crazy to try to migrate the data
> in the background:
>
> - The migration might never complete because of a UNIQUE or CHECK
> constraint on the partition to which rows are being migrated.
>
> - Even if the migration eventually succeeded, such a design abandons
> all hope of making INSERT .. ON CONFLICT DO NOTHING work sensibly
> while the migration is in progress, unless the new partition has no
> UNIQUE constraints.
>
> - Partition-wise join and partition-wise aggregate would need to have
> special case handling for the case of an unfinished migration, as
> would any user code that accesses partitions directly.
>
> - More generally, I think users expect that when a DDL command
> finishes execution, it's done all of the work that there is to do (or
> at the very least, that any remaining work has no user-visible
> consequences, which would not be the case here).

Thanks Robert for this explanation. This makes it more clear, why row
movement by background is not sensible idea.

On Thu, Apr 6, 2017 at 9:38 AM, Keith Fiske <keith@omniti.com> wrote:

>
> On Wed, Apr 5, 2017 at 2:51 PM, Keith Fiske <keith@omniti.com> wrote:
>
>>
>>
>> Only issue I see with this, and I'm not sure if it is an issue, is what
>> happens to that default constraint clause when 1000s of partitions start
>> getting added? From what I gather the default's constraint is built based
>> off the cumulative opposite of all other child constraints. I don't
>> understand the code well enough to see what it's actually doing, but if
>> there are no gaps, is the method used smart enough to aggregate all the
>> child constraints to make a simpler constraint that is simply outside the
>> current min/max boundaries? If so, for serial/time range partitioning this
>> should typically work out fine since there are rarely gaps. This actually
>> seems more of an issue for list partitioning where each child is a distinct
>> value or range of values that are completely arbitrary. Won't that check
>> and re-evaluation of the default's constraint just get worse and worse as
>> more children are added? Is there really even a need for the default to
>> have an opposite constraint like this? Not sure on how the planner works
>> with partitioning now, but wouldn't it be better to first check all
>> non-default children for a match the same as it does now without a default
>> and, failing that, then route to the default if one is declared? The
>> default should accept any data then so I don't see the need for the
>> constraint unless it's required for the current implementation. If that's
>> the case, could that be changed?
>>
>> Keith
>>
>
> Actually, thinking on this more, I realized this does again come back to
> the lack of a global index. Without the constraint, data could be put
> directly into the default that could technically conflict with the
> partition scheme elsewhere. Perhaps, instead of the constraint, inserts
> directly to the default could be prevented on the user level. Writing to
> valid children directly certainly has its place, but been thinking about
> it, and I can't see any reason why one would ever want to write directly to
> the default. It's use case seems to be around being a sort of temporary
> storage until that data can be moved to a valid location. Would still need
> to allow removal of data, though.
>
> Not sure if that's even a workable solution. Just trying to think of ways
> around the current limitations and still allow this feature.
>

I like the idea about having DEFAULT partition for the range partition.
With the
way partition is designed it can have holes into range partition. I think
DEFAULT
for the range partition is a good idea, generally when the range having
holes. When
range is serial then of course DEFAULT partition doen't much sense.

Regarda,

Rushabh Lathia
www.EnterpriseDB.com

Re: Adding support for Default partition in partitioning

Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-04-06T05:18:51Z

On 2017/04/06 13:08, Keith Fiske wrote:
> On Wed, Apr 5, 2017 at 2:51 PM, Keith Fiske wrote:
>> Only issue I see with this, and I'm not sure if it is an issue, is what
>> happens to that default constraint clause when 1000s of partitions start
>> getting added? From what I gather the default's constraint is built based
>> off the cumulative opposite of all other child constraints. I don't
>> understand the code well enough to see what it's actually doing, but if
>> there are no gaps, is the method used smart enough to aggregate all the
>> child constraints to make a simpler constraint that is simply outside the
>> current min/max boundaries? If so, for serial/time range partitioning this
>> should typically work out fine since there are rarely gaps. This actually
>> seems more of an issue for list partitioning where each child is a distinct
>> value or range of values that are completely arbitrary. Won't that check
>> and re-evaluation of the default's constraint just get worse and worse as
>> more children are added? Is there really even a need for the default to
>> have an opposite constraint like this? Not sure on how the planner works
>> with partitioning now, but wouldn't it be better to first check all
>> non-default children for a match the same as it does now without a default
>> and, failing that, then route to the default if one is declared? The
>> default should accept any data then so I don't see the need for the
>> constraint unless it's required for the current implementation. If that's
>> the case, could that be changed?

Unless I misread your last sentence, I think there might be some
confusion.  Currently, the partition constraint (think of these as you
would of user-defined check constraints) is needed for two reasons: 1. to
prevent direct insertion of rows into the default partition for which a
non-default partition exists; no two partitions should ever have duplicate
rows.  2. so that planner can use the constraint to determine if the
default partition needs to be scanned for a query using constraint
exclusion; no need, for example, to scan the default partition if the
query requests only key=3 rows and a partition for the same exists (no
other partition should have key=3 rows by definition, not even the
default).  As things stand today, planner needs to look at every partition
individually for using constraint exclusion to possibly exclude it, *even*
with declarative partitioning and that would include the default partition.

> Actually, thinking on this more, I realized this does again come back to
> the lack of a global index. Without the constraint, data could be put
> directly into the default that could technically conflict with the
> partition scheme elsewhere. Perhaps, instead of the constraint, inserts
> directly to the default could be prevented on the user level. Writing to
> valid children directly certainly has its place, but been thinking about
> it, and I can't see any reason why one would ever want to write directly to
> the default. It's use case seems to be around being a sort of temporary
> storage until that data can be moved to a valid location. Would still need
> to allow removal of data, though.

As mentioned above, the default partition will not allow directly
inserting a row whose key maps to some existing (non-default) partition.

As far as tuple-routing is concerned, it will choose the default partition
only if no other partition is found for the key.  Tuple-routing doesn't
use the partition constraints directly per se, like one of the two things
mentioned above do.  One could say that tuple-routing assigns the incoming
rows to partitions such that their individual partition constraints are
not violated.

Finally, we don't yet offer global guarantees for constraints like unique.
 The only guarantee that's in place is that no two partitions can contain
the same partition key.

Thanks,
Amit

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-04-06T11:30:24Z

Hello,

Thanks a lot for testing and reporting this. Please find attached an
updated patch with the fix. The patch also contains a fix
regarding operator used at the time of creating expression as default
partition constraint. This was notified offlist by Amit Langote.

Thank you,
Rahila Syed


On Thu, Apr 6, 2017 at 12:21 AM, Keith Fiske <keith@omniti.com> wrote:

>
> On Wed, Apr 5, 2017 at 11:19 AM, Robert Haas <robertmhaas@gmail.com>
> wrote:
>
>> On Wed, Apr 5, 2017 at 5:57 AM, Rahila Syed <rahilasyed90@gmail.com>
>> wrote:
>> >>Could you briefly elaborate why you think the lack global index support
>> >>would be a problem in this regard?
>> > I think following can happen if we allow rows satisfying the new
>> partition
>> > to lie around in the
>> > default partition until background process moves it.
>> > Consider a scenario where partition key is a primary key and the data
>> in the
>> > default partition is
>> > not yet moved into the newly added partition. If now, new data is added
>> into
>> > the new partition
>> > which also exists(same key) in default partition there will be data
>> > duplication. If now
>> > we scan the partitioned table for that key(from both the default and new
>> > partition as we
>> > have not moved the rows) it will fetch the both rows.
>> > Unless we have global indexes for partitioned tables, there is chance of
>> > data duplication between
>> > child table added after default partition and the default partition.
>>
>> Yes, I think it would be completely crazy to try to migrate the data
>> in the background:
>>
>> - The migration might never complete because of a UNIQUE or CHECK
>> constraint on the partition to which rows are being migrated.
>>
>> - Even if the migration eventually succeeded, such a design abandons
>> all hope of making INSERT .. ON CONFLICT DO NOTHING work sensibly
>> while the migration is in progress, unless the new partition has no
>> UNIQUE constraints.
>>
>> - Partition-wise join and partition-wise aggregate would need to have
>> special case handling for the case of an unfinished migration, as
>> would any user code that accesses partitions directly.
>>
>> - More generally, I think users expect that when a DDL command
>> finishes execution, it's done all of the work that there is to do (or
>> at the very least, that any remaining work has no user-visible
>> consequences, which would not be the case here).
>>
>> IMV, the question of whether we have efficient ways to move data
>> around between partitions is somewhat separate from the question of
>> whether partitions can be defined in a certain way in the first place.
>> The problems that Keith refers to upthread already exist for
>> subpartitioning; you've got to detach the old partition, create a new
>> one, and then reinsert the data.  And for partitioning an
>> unpartitioned table: create a replacement table, insert all the data,
>> substitute it for the original table.  The fact that we have these
>> limitation is not good, but we're not going to rip out partitioning
>> entirely because we don't have clever ways of migrating the data in
>> those cases, and the proposed behavior here is not any worse.
>>
>> Also, waiting for those problems to get fixed might be waiting for
>> Godot.  I'm not really all that sanguine about our chances of coming
>> up with a really nice way of handling these cases.  In a designed
>> based on table inheritance, you can leave it murky where certain data
>> is supposed to end up and migrate it on-line and you might get away
>> with that, but a major point of having declarative partitioning at all
>> is to remove that sort of murkiness.  It's probably not that hard to
>> come up with a command that locks the parent and moves data around via
>> full table scans, but I'm not sure how far that really gets us; you
>> could do the same thing easily enough with a sequence of commands
>> generated via a script.  And being able to do this in a general way
>> without a full table lock looks pretty hard - it doesn't seem
>> fundamentally different from trying to perform a table-rewriting
>> operation like CLUSTER without a full table lock, which we also don't
>> support.  The executor is not built to cope with any aspect of the
>> table definition shifting under it, and that includes the set of child
>> tables with are partitions of the table mentioned in the query.  Maybe
>> the executor can be taught to survive such definitional changes at
>> least in limited cases, but that's a much different project than
>> allowing default partitions.
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
> Confirmed that v5 patch works with examples given in the original post but
> segfaulted when I tried the examples I used in my blog post (taken from the
> documentation at the time I wrote it). https://www.keithf4.com/
> postgresql-10-built-in-partitioning/
>
> keith@keith=# drop table cities;
> DROP TABLE
> Time: 6.055 ms
> keith@keith=# CREATE TABLE cities (
>     city_id         bigserial not null,
>     name         text not null,
>     population   int
> ) PARTITION BY LIST (initcap(name));
> CREATE TABLE
> Time: 7.130 ms
> keith@keith=# CREATE TABLE cities_west
>     PARTITION OF cities (
>     CONSTRAINT city_id_nonzero CHECK (city_id != 0)
> ) FOR VALUES IN ('Los Angeles', 'San Francisco');
> CREATE TABLE
> Time: 6.690 ms
> keith@keith=# CREATE TABLE cities_default
> keith-#     PARTITION OF cities FOR VALUES IN (DEFAULT);
> 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: WARNING:
> terminating connection because of crash of another server process
> DETAIL:  The postmaster has commanded this server process to roll back the
> current transaction and exit, because another server process exited
> abnormally and possibly corrupted shared memory.
> HINT:  In a moment you should be able to reconnect to the database and
> repeat your command.
> Failed.
> Time: 387.887 ms
>
> After reading responses, I think I'd be fine with how Rahila implemented
> this with disallowing the child until the data is removed from the default
> if this would allow it to be included in v10. As was mentioned, there just
> doesn't seem to be a way to easily handle the data conflicts cleanly at
> this time, but I think the value of the default to be able to catch
> accidental data vs returning an error is worth it. It also at least gives a
> slightly easier migration path vs having to migrate to a completely new
> table. Any chance this could be adapted for range partitioning as well? I'd
> be happy to create some pgtap tests with pg_partman for this then to make
> sure it works.
>
> Only issue I see with this, and I'm not sure if it is an issue, is what
> happens to that default constraint clause when 1000s of partitions start
> getting added? From what I gather the default's constraint is built based
> off the cumulative opposite of all other child constraints. I don't
> understand the code well enough to see what it's actually doing, but if
> there are no gaps, is the method used smart enough to aggregate all the
> child constraints to make a simpler constraint that is simply outside the
> current min/max boundaries? If so, for serial/time range partitioning this
> should typically work out fine since there are rarely gaps. This actually
> seems more of an issue for list partitioning where each child is a distinct
> value or range of values that are completely arbitrary. Won't that check
> and re-evaluation of the default's constraint just get worse and worse as
> more children are added? Is there really even a need for the default to
> have an opposite constraint like this? Not sure on how the planner works
> with partitioning now, but wouldn't it be better to first check all
> non-default children for a match the same as it does now without a default
> and, failing that, then route to the default if one is declared? The
> default should accept any data then so I don't see the need for the
> constraint unless it's required for the current implementation. If that's
> the case, could that be changed?
>
> Keith
>

Re: Adding support for Default partition in partitioning

Keith Fiske <keith@omniti.com> — 2017-04-06T14:46:12Z

On Thu, Apr 6, 2017 at 1:18 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
wrote:

> On 2017/04/06 13:08, Keith Fiske wrote:
> > On Wed, Apr 5, 2017 at 2:51 PM, Keith Fiske wrote:
> >> Only issue I see with this, and I'm not sure if it is an issue, is what
> >> happens to that default constraint clause when 1000s of partitions start
> >> getting added? From what I gather the default's constraint is built
> based
> >> off the cumulative opposite of all other child constraints. I don't
> >> understand the code well enough to see what it's actually doing, but if
> >> there are no gaps, is the method used smart enough to aggregate all the
> >> child constraints to make a simpler constraint that is simply outside
> the
> >> current min/max boundaries? If so, for serial/time range partitioning
> this
> >> should typically work out fine since there are rarely gaps. This
> actually
> >> seems more of an issue for list partitioning where each child is a
> distinct
> >> value or range of values that are completely arbitrary. Won't that check
> >> and re-evaluation of the default's constraint just get worse and worse
> as
> >> more children are added? Is there really even a need for the default to
> >> have an opposite constraint like this? Not sure on how the planner works
> >> with partitioning now, but wouldn't it be better to first check all
> >> non-default children for a match the same as it does now without a
> default
> >> and, failing that, then route to the default if one is declared? The
> >> default should accept any data then so I don't see the need for the
> >> constraint unless it's required for the current implementation. If
> that's
> >> the case, could that be changed?
>
> Unless I misread your last sentence, I think there might be some
> confusion.  Currently, the partition constraint (think of these as you
> would of user-defined check constraints) is needed for two reasons: 1. to
> prevent direct insertion of rows into the default partition for which a
> non-default partition exists; no two partitions should ever have duplicate
> rows.  2. so that planner can use the constraint to determine if the
> default partition needs to be scanned for a query using constraint
> exclusion; no need, for example, to scan the default partition if the
> query requests only key=3 rows and a partition for the same exists (no
> other partition should have key=3 rows by definition, not even the
> default).  As things stand today, planner needs to look at every partition
> individually for using constraint exclusion to possibly exclude it, *even*
> with declarative partitioning and that would include the default partition.
>

Forgot about constraint exclusion. My follow up email that you answered
below was addressing the prevention of data to the default if there was no
constraint on the default. I guess my main concern was with how manageable
that cumulative opposite constraint of the default would be over time,
especially with list partitioning. And also that it's smart enough to
consolidate constraint conditions to simplify things if it's found that two
or more conditions cover a continuous range.


>
> > Actually, thinking on this more, I realized this does again come back to
> > the lack of a global index. Without the constraint, data could be put
> > directly into the default that could technically conflict with the
> > partition scheme elsewhere. Perhaps, instead of the constraint, inserts
> > directly to the default could be prevented on the user level. Writing to
> > valid children directly certainly has its place, but been thinking about
> > it, and I can't see any reason why one would ever want to write directly
> to
> > the default. It's use case seems to be around being a sort of temporary
> > storage until that data can be moved to a valid location. Would still
> need
> > to allow removal of data, though.
>
> As mentioned above, the default partition will not allow directly
> inserting a row whose key maps to some existing (non-default) partition.
>
> As far as tuple-routing is concerned, it will choose the default partition
> only if no other partition is found for the key.  Tuple-routing doesn't
> use the partition constraints directly per se, like one of the two things
> mentioned above do.  One could say that tuple-routing assigns the incoming
> rows to partitions such that their individual partition constraints are
> not violated.
>
>
Finally, we don't yet offer global guarantees for constraints like unique.
>  The only guarantee that's in place is that no two partitions can contain
> the same partition key.
>
> Thanks,
> Amit
>
>
>

Re: Adding support for Default partition in partitioning

Keith Fiske <keith@omniti.com> — 2017-04-06T16:07:57Z

On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:

> Hello,
>
> Thanks a lot for testing and reporting this. Please find attached an
> updated patch with the fix. The patch also contains a fix
> regarding operator used at the time of creating expression as default
> partition constraint. This was notified offlist by Amit Langote.
>
> Thank you,
> Rahila Syed
>
>
Could probably use some more extensive testing, but all examples I had on
my previously mentioned blog post are now working.

Keith

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-04-10T14:42:43Z

Hi Rahila,


With your latest patch:

Consider a case when a table is partitioned on a boolean key.

Even when there are existing separate partitions for 'true' and

'false', still default partition can be created.


I think this should not be allowed.


Consider following case:


postgres=# CREATE TABLE list_partitioned (

    a bool

) PARTITION BY LIST (a);

CREATE TABLE

postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
('false');

CREATE TABLE

postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN
('true');

CREATE TABLE

postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
VALUES IN (DEFAULT);

CREATE TABLE


The creation of table part_default should have failed instead.


Thanks,

Jeevan Ladhe



On Thu, Apr 6, 2017 at 9:37 PM, Keith Fiske <keith@omniti.com> wrote:

>
> On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed <rahilasyed90@gmail.com>
> wrote:
>
>> Hello,
>>
>> Thanks a lot for testing and reporting this. Please find attached an
>> updated patch with the fix. The patch also contains a fix
>> regarding operator used at the time of creating expression as default
>> partition constraint. This was notified offlist by Amit Langote.
>>
>> Thank you,
>> Rahila Syed
>>
>>
> Could probably use some more extensive testing, but all examples I had on
> my previously mentioned blog post are now working.
>
> Keith
>
>

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-04-10T14:51:32Z

Hi Rahila,

I tried to review the code, and here are some of my early comments:

1.
When I configure using "-Werror", I see unused variable in function
DefineRelation:

tablecmds.c: In function ‘DefineRelation’:
tablecmds.c:761:17: error: unused variable ‘partdesc’
[-Werror=unused-variable]
   PartitionDesc partdesc;
                 ^

2.
Typo in comment:
+ /*
+ * When adding a list partition after default partition, scan the
+ * default partiton for rows satisfying the new partition
+ * constraint. If found dont allow addition of a new partition.
+ * Otherwise continue with the creation of new  partition.
+ */

partition
don't

3.
I think instead of a period '.', it will be good if you can use semicolon
';'
in following declaration similar to the comment for 'null_index'.

+ int def_index; /* Index of the default list partition. -1 for
+ * range partitioned tables */

4.
You may want to consider 80 column alignment for changes done in function
get_qual_from_partbound, and other places as applicable.

5.
It would be good if the patch has some test coverage that explains what is
being achieved, what kind of error handling is done etc.

6.
There are some places having code like following:

+ Node *value = lfirst(c);
  Const   *val = lfirst(c);
  PartitionListValue *list_value = NULL;

+ if (IsA(value, DefElem))

The additional variable is not needed and you can call IsA on val itself.

7.
Also, in places like below where you are just trying to check for node is a
DefaultElem, you can avoid an extra variable:

+ foreach(cell1, bspec->listdatums)
+ {
+ Node *value = lfirst(cell1);
+ if (IsA(value, DefElem))
+ {
+ def_elem = true;
+ *defid = inhrelid;
+ }

Can be written as:
+ foreach(cell1, bspec->listdatums)
+ {
+ if (IsA(lfirst(cell1), DefElem))
+ {
+ def_elem = true;
+ *defid = inhrelid;
+ }
+ }


Regards,
Jeevan Ladhe



On Mon, Apr 10, 2017 at 8:12 PM, Jeevan Ladhe <jeevan.ladhe@enterprisedb.com
> wrote:

> Hi Rahila,
>
>
> With your latest patch:
>
> Consider a case when a table is partitioned on a boolean key.
>
> Even when there are existing separate partitions for 'true' and
>
> 'false', still default partition can be created.
>
>
> I think this should not be allowed.
>
>
> Consider following case:
>
>
> postgres=# CREATE TABLE list_partitioned (
>
>     a bool
>
> ) PARTITION BY LIST (a);
>
> CREATE TABLE
>
> postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
> ('false');
>
> CREATE TABLE
>
> postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN
> ('true');
>
> CREATE TABLE
>
> postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
> VALUES IN (DEFAULT);
>
> CREATE TABLE
>
>
> The creation of table part_default should have failed instead.
>
>
> Thanks,
>
> Jeevan Ladhe
>
>
>
> On Thu, Apr 6, 2017 at 9:37 PM, Keith Fiske <keith@omniti.com> wrote:
>
>>
>> On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed <rahilasyed90@gmail.com>
>> wrote:
>>
>>> Hello,
>>>
>>> Thanks a lot for testing and reporting this. Please find attached an
>>> updated patch with the fix. The patch also contains a fix
>>> regarding operator used at the time of creating expression as default
>>> partition constraint. This was notified offlist by Amit Langote.
>>>
>>> Thank you,
>>> Rahila Syed
>>>
>>>
>> Could probably use some more extensive testing, but all examples I had on
>> my previously mentioned blog post are now working.
>>
>> Keith
>>
>>
>

Re: Adding support for Default partition in partitioning

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-04-11T12:32:30Z

On Mon, Apr 10, 2017 at 8:12 PM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> Hi Rahila,
>
>
> With your latest patch:
>
> Consider a case when a table is partitioned on a boolean key.
>
> Even when there are existing separate partitions for 'true' and
>
> 'false', still default partition can be created.
>
>
> I think this should not be allowed.

Well, boolean columns can have "NULL" values which will go into
default partition if no NULL partition exists. So, probably we should
add check for NULL partition there.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-04-11T13:41:33Z

Hi Ashutosh,


On Tue, Apr 11, 2017 at 6:02 PM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:

> On Mon, Apr 10, 2017 at 8:12 PM, Jeevan Ladhe
> <jeevan.ladhe@enterprisedb.com> wrote:
> > Hi Rahila,
> >
> >
> > With your latest patch:
> >
> > Consider a case when a table is partitioned on a boolean key.
> >
> > Even when there are existing separate partitions for 'true' and
> >
> > 'false', still default partition can be created.
> >
> >
> > I think this should not be allowed.
>
> Well, boolean columns can have "NULL" values which will go into
> default partition if no NULL partition exists. So, probably we should
> add check for NULL partition there.


I have checked for NULLs too, and the default partition can be created even
when there are partitions for each TRUE, FALSE and NULL.

Consider the example below:

postgres=# CREATE TABLE list_partitioned (
    a bool
) PARTITION BY LIST (a);
CREATE TABLE
postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
('false');
CREATE TABLE
postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN
('true');
CREATE TABLE
postgres=# CREATE TABLE part_3 PARTITION OF list_partitioned FOR VALUES IN
(null);
CREATE TABLE
postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
VALUES IN (DEFAULT);
CREATE TABLE

Regards,
Jeevan Ladhe

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-04-12T18:39:11Z

On Tue, Apr 11, 2017 at 9:41 AM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> I have checked for NULLs too, and the default partition can be created even
> when there are partitions for each TRUE, FALSE and NULL.
>
> Consider the example below:
>
> postgres=# CREATE TABLE list_partitioned (
>     a bool
> ) PARTITION BY LIST (a);
> CREATE TABLE
> postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
> ('false');
> CREATE TABLE
> postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN
> ('true');
> CREATE TABLE
> postgres=# CREATE TABLE part_3 PARTITION OF list_partitioned FOR VALUES IN
> (null);
> CREATE TABLE
> postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
> VALUES IN (DEFAULT);
> CREATE TABLE

In my opinion, that's absolutely fine, and it would be very strange to
try to prevent it.  The partitioning method shouldn't have specific
knowledge of the properties of individual data types.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-04-12T18:41:34Z

On Thu, Apr 6, 2017 at 1:17 AM, Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
> I like the idea about having DEFAULT partition for the range partition. With
> the
> way partition is designed it can have holes into range partition. I think
> DEFAULT
> for the range partition is a good idea, generally when the range having
> holes. When
> range is serial then of course DEFAULT partition doen't much sense.

Yes, I like that idea, too.  I think the DEFAULT partition should be
allowed to be created for either range or list partitioning regardless
of whether we think there are any holes, but if you create a DEFAULT
partition when there are no holes, you just won't be able to put any
data into it.  It's silly, but it's not worth the code that it would
take to try to prevent it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-04-12T19:18:38Z

On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
> Thanks a lot for testing and reporting this. Please find attached an updated
> patch with the fix. The patch also contains a fix
> regarding operator used at the time of creating expression as default
> partition constraint. This was notified offlist by Amit Langote.

I think that the syntax for this patch should probably be revised.
Right now the proposal is for:

CREATE TABLE .. PARTITION OF ... FOR VALUES IN (DEFAULT);

But that's not a good idea for several reasons.  For one thing, you
can also write FOR VALUES IN (DEFAULT, 5) or which isn't sensible.
For another thing, this kind of syntax won't generalize to range
partitioning, which we've talked about making this feature support.
Maybe something like:

CREATE TABLE .. PARTITION OF .. DEFAULT;

This patch makes the assumption throughout that any DefElem represents
the word DEFAULT, which is true in the patch as written but doesn't
seem very future-proof.  I think the "def" in "DefElem" stands for
"definition" or "define" or something like that, so this is actually
pretty confusing.  Maybe we should introduce a dedicated node type to
represent a default-specification in the parser grammar.  If not, then
let's at least encapsulate the test a little better, e.g. by adding
isDefaultPartitionBound() which tests not only IsA(..., DefElem) but
also whether the name is DEFAULT as expected.  BTW, we typically use
lower-case internally, so if we stick with this representation it
should really be "default" not "DEFAULT".

Useless hunk:

+    bool        has_def;        /* Is there a default partition?
Currently false
+                                 * for a range partitioned table */
+    int            def_index;        /* Index of the default list
partition. -1 for
+                                 * range partitioned tables */

Why abbreviate "default" to def here?  Seems pointless.

+                    if (found_def)
+                    {
+                        if (mapping[def_index] == -1)
+                            mapping[def_index] = next_index++;
+                    }

Consider &&

@@ -717,7 +754,6 @@ check_new_partition_bound(char *relname, Relation
parent, Node *bound)
                         }
                     }
                 }
-
                 break;
             }

+     * default partiton for rows satisfying the new partition

Spelling.

+     * constraint. If found dont allow addition of a new partition.

Missing apostrophe.

+        defrel = heap_open(defid, AccessShareLock);
+        tupdesc = CreateTupleDescCopy(RelationGetDescr(defrel));
+
+        /* Build expression execution states for partition check quals */
+        partqualstate = ExecPrepareCheck(partConstraint,
+                        estate);
+
+        econtext = GetPerTupleExprContext(estate);
+        snapshot = RegisterSnapshot(GetLatestSnapshot());

Definitely not safe against concurrency, since AccessShareLock won't
exclude somebody else's update.  In fact, it won't even cover somebody
else's already-in-flight transaction.

+                errmsg("new default partition constraint is violated
by some row")));

Normally in such cases we try to give more detail using
ExecBuildSlotValueDescription.

+    bool        is_def = true;

This variable starts out true and is never set to any value other than
true.  Just get rid of it and, in the one place where it is currently
used, write "true".  That's shorter and clearer.

+    inhoids = find_inheritance_children(RelationGetRelid(parent), NoLock);

If it's actually safe to do this with no lock, there ought to be a
comment with a very compelling explanation of why it's safe.

+        boundspec = (Node *) stringToNode(TextDatumGetCString(datum));
+        bspec = (PartitionBoundSpec *)boundspec;

There's not really a reason to cast the result of stringToNode() to
Node * and then turn around and cast it to PartitionBoundSpec *.  Just
cast it directly to whatever it needs to be.  And use the new castNode
macro.

+        foreach(cell1, bspec->listdatums)
+        {
+            Node *value = lfirst(cell1);
+            if (IsA(value, DefElem))
+            {
+                def_elem = true;
+                *defid = inhrelid;
+            }
+        }
+        if (def_elem)
+        {
+            ReleaseSysCache(tuple);
+            continue;
+        }
+        foreach(cell3, bspec->listdatums)
+        {
+            Node *value = lfirst(cell3);
+            boundspecs = lappend(boundspecs, value);
+        }
+        ReleaseSysCache(tuple);
+    }
+    foreach(cell4, spec->listdatums)
+    {
+        Node *value = lfirst(cell4);
+        boundspecs = lappend(boundspecs, value);
+    }

cell1, cell2, cell3, and cell4 are not very clear variable names.
Between that and the lack of comments, this is not easy to understand.
It's sort of spaghetti logic, too.  The if (def_elem) test continues
early, but if the point is that the loop using cell3 shouldn't execute
in that case, why not just put if (!def_elem) { foreach(cell3, ...) {
... } } instead of reiterating the ReleaseSysCache in two places?

+                /* Collect bound spec nodes in a list. This is done
if the partition is
+                 * a default partition. In case of default partition,
constraint is formed
+                 * by performing <> operation over the partition
constraints of the
+                 * existing partitions.
+                 */

I doubt that handles NULLs properly.

+                inhoids =
find_inheritance_children(RelationGetRelid(parent), NoLock);

Again, no lock?  Really?

The logic which follows looks largely cut-and-pasted, which makes me
think you need to do some refactoring here to make it more clear
what's going on, so that you have the relevant logic in just one
place.  It seems wrong anyway to shove all of this logic specific to
the default case into get_qual_from_partbound() when the logic for the
non-default case is inside get_qual_for_list.  Where there were 2
lines of code before you've now got something like 30.

+        if(get_negator(operoid) == InvalidOid)
+            elog(ERROR, "no negator found for partition operator %u",
+                 operoid);

I really doubt that's OK.  elog() shouldn't be reachable, but this
will be reachable if the partitioning operator does not have a
negator.  And there's the NULL-handling issue I mentioned above, too.

+            if (partdesc->boundinfo->has_def && key->strategy
+                == PARTITION_STRATEGY_LIST)
+                result = parent->indexes[partdesc->boundinfo->def_index];

Testing for PARTITION_STRATEGY_LIST here seems unnecessary.  If
has_def (or has_default, as it probably should be) isn't allowed for
range partitions, then it's redundant; if it is allowed, then that
case should be handled too.  Also, at this point we've already set
*failed_at and *failed_slot; presumably you'd want to make this check
before you get to that point.

I suspect there are quite a few more problems here in addition to the
ones mentioned above, but I don't think it makes sense to spend too
much time searching for them until some of this basic stuff is cleaned
up.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-04-24T09:10:24Z

Hello,

Thank you for reviewing.

>But that's not a good idea for several reasons.  For one thing, you
>can also write FOR VALUES IN (DEFAULT, 5) or which isn't sensible.
>For another thing, this kind of syntax won't generalize to range
>partitioning, which we've talked about making this feature support.
>Maybe something like:

>CREATE TABLE .. PARTITION OF .. DEFAULT;

I agree that the syntax should be changed to also support range
partitioning.

Following can also be considered as it specifies more clearly that the
partition holds default values.

CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;

>Maybe we should introduce a dedicated node type to
>represent a default-specification in the parser grammar.  If not, then
>let's at least encapsulate the test a little better, e.g. by adding
>isDefaultPartitionBound() which tests not only IsA(..., DefElem) but
>also whether the name is DEFAULT as expected.  BTW, we typically use
>lower-case internally, so if we stick with this representation it
>should really be "default" not "DEFAULT".

isDefaultPartitionBound() function is created in the attached patch which
checks for both node type and name.

>Why abbreviate "default" to def here?  Seems pointless.
Corrected in the attached.

>Consider &&
Fixed.

>+     * default partiton for rows satisfying the new partition
>Spelling.
Fixed.

>Missing apostrophe
Fixed.

>Definitely not safe against concurrency, since AccessShareLock won't
>exclude somebody else's update.  In fact, it won't even cover somebody
>else's already-in-flight transaction
Changed it to AccessExclusiveLock

>Normally in such cases we try to give more detail using
>ExecBuildSlotValueDescription.
This function is used in execMain.c and the error is being
reported in partition.c.
Do you mean the error reporting should be moved into execMain.c
to use ExecBuildSlotValueDescription?

>This variable starts out true and is never set to any value other than
>true.  Just get rid of it and, in the one place where it is currently
>used, write "true".  That's shorter and clearer.
Fixed.

>There's not really a reason to cast the result of stringToNode() to
>Node * and then turn around and cast it to PartitionBoundSpec *.  Just
>cast it directly to whatever it needs to be.  And use the new castNode
>macro
Fixed. castNode macro takes as input Node * whereas stringToNode() takes
string.
IIUC, castNode cant be used here.

>The if (def_elem) test continues
>early, but if the point is that the loop using cell3 shouldn't execute
>in that case, why not just put if (!def_elem) { foreach(cell3, ...) {
>... } } instead of reiterating the ReleaseSysCache in two places?
Fixed in the attached.

I will respond to further comments in following email.


On Thu, Apr 13, 2017 at 12:48 AM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed <rahilasyed90@gmail.com>
> wrote:
> > Thanks a lot for testing and reporting this. Please find attached an
> updated
> > patch with the fix. The patch also contains a fix
> > regarding operator used at the time of creating expression as default
> > partition constraint. This was notified offlist by Amit Langote.
>
> I think that the syntax for this patch should probably be revised.
> Right now the proposal is for:
>
> CREATE TABLE .. PARTITION OF ... FOR VALUES IN (DEFAULT);
>
> But that's not a good idea for several reasons.  For one thing, you
> can also write FOR VALUES IN (DEFAULT, 5) or which isn't sensible.
> For another thing, this kind of syntax won't generalize to range
> partitioning, which we've talked about making this feature support.
> Maybe something like:
>
> CREATE TABLE .. PARTITION OF .. DEFAULT;
>
> This patch makes the assumption throughout that any DefElem represents
> the word DEFAULT, which is true in the patch as written but doesn't
> seem very future-proof.  I think the "def" in "DefElem" stands for
> "definition" or "define" or something like that, so this is actually
> pretty confusing.  Maybe we should introduce a dedicated node type to
> represent a default-specification in the parser grammar.  If not, then
> let's at least encapsulate the test a little better, e.g. by adding
> isDefaultPartitionBound() which tests not only IsA(..., DefElem) but
> also whether the name is DEFAULT as expected.  BTW, we typically use
> lower-case internally, so if we stick with this representation it
> should really be "default" not "DEFAULT".
>
> Useless hunk:
>
> +    bool        has_def;        /* Is there a default partition?
> Currently false
> +                                 * for a range partitioned table */
> +    int            def_index;        /* Index of the default list
> partition. -1 for
> +                                 * range partitioned tables */
>
> Why abbreviate "default" to def here?  Seems pointless.
>
> +                    if (found_def)
> +                    {
> +                        if (mapping[def_index] == -1)
> +                            mapping[def_index] = next_index++;
> +                    }
>
> Consider &&
>
> @@ -717,7 +754,6 @@ check_new_partition_bound(char *relname, Relation
> parent, Node *bound)
>                          }
>                      }
>                  }
> -
>                  break;
>              }
>
> +     * default partiton for rows satisfying the new partition
>
> Spelling.
>
> +     * constraint. If found dont allow addition of a new partition.
>
> Missing apostrophe.
>
> +        defrel = heap_open(defid, AccessShareLock);
> +        tupdesc = CreateTupleDescCopy(RelationGetDescr(defrel));
> +
> +        /* Build expression execution states for partition check quals */
> +        partqualstate = ExecPrepareCheck(partConstraint,
> +                        estate);
> +
> +        econtext = GetPerTupleExprContext(estate);
> +        snapshot = RegisterSnapshot(GetLatestSnapshot());
>
> Definitely not safe against concurrency, since AccessShareLock won't
> exclude somebody else's update.  In fact, it won't even cover somebody
> else's already-in-flight transaction.
>
> +                errmsg("new default partition constraint is violated
> by some row")));
>
> Normally in such cases we try to give more detail using
> ExecBuildSlotValueDescription.
>
> +    bool        is_def = true;
>
> This variable starts out true and is never set to any value other than
> true.  Just get rid of it and, in the one place where it is currently
> used, write "true".  That's shorter and clearer.
>
> +    inhoids = find_inheritance_children(RelationGetRelid(parent),
> NoLock);
>
> If it's actually safe to do this with no lock, there ought to be a
> comment with a very compelling explanation of why it's safe.
>
> +        boundspec = (Node *) stringToNode(TextDatumGetCString(datum));
> +        bspec = (PartitionBoundSpec *)boundspec;
>
> There's not really a reason to cast the result of stringToNode() to
> Node * and then turn around and cast it to PartitionBoundSpec *.  Just
> cast it directly to whatever it needs to be.  And use the new castNode
> macro.
>
> +        foreach(cell1, bspec->listdatums)
> +        {
> +            Node *value = lfirst(cell1);
> +            if (IsA(value, DefElem))
> +            {
> +                def_elem = true;
> +                *defid = inhrelid;
> +            }
> +        }
> +        if (def_elem)
> +        {
> +            ReleaseSysCache(tuple);
> +            continue;
> +        }
> +        foreach(cell3, bspec->listdatums)
> +        {
> +            Node *value = lfirst(cell3);
> +            boundspecs = lappend(boundspecs, value);
> +        }
> +        ReleaseSysCache(tuple);
> +    }
> +    foreach(cell4, spec->listdatums)
> +    {
> +        Node *value = lfirst(cell4);
> +        boundspecs = lappend(boundspecs, value);
> +    }
>
> cell1, cell2, cell3, and cell4 are not very clear variable names.
> Between that and the lack of comments, this is not easy to understand.
> It's sort of spaghetti logic, too.  The if (def_elem) test continues
> early, but if the point is that the loop using cell3 shouldn't execute
> in that case, why not just put if (!def_elem) { foreach(cell3, ...) {
> ... } } instead of reiterating the ReleaseSysCache in two places?
>
> +                /* Collect bound spec nodes in a list. This is done
> if the partition is
> +                 * a default partition. In case of default partition,
> constraint is formed
> +                 * by performing <> operation over the partition
> constraints of the
> +                 * existing partitions.
> +                 */
>
> I doubt that handles NULLs properly.
>
> +                inhoids =
> find_inheritance_children(RelationGetRelid(parent), NoLock);
>
> Again, no lock?  Really?
>
> The logic which follows looks largely cut-and-pasted, which makes me
> think you need to do some refactoring here to make it more clear
> what's going on, so that you have the relevant logic in just one
> place.  It seems wrong anyway to shove all of this logic specific to
> the default case into get_qual_from_partbound() when the logic for the
> non-default case is inside get_qual_for_list.  Where there were 2
> lines of code before you've now got something like 30.
>
> +        if(get_negator(operoid) == InvalidOid)
> +            elog(ERROR, "no negator found for partition operator %u",
> +                 operoid);
>
> I really doubt that's OK.  elog() shouldn't be reachable, but this
> will be reachable if the partitioning operator does not have a
> negator.  And there's the NULL-handling issue I mentioned above, too.
>
> +            if (partdesc->boundinfo->has_def && key->strategy
> +                == PARTITION_STRATEGY_LIST)
> +                result = parent->indexes[partdesc->boundinfo->def_index];
>
> Testing for PARTITION_STRATEGY_LIST here seems unnecessary.  If
> has_def (or has_default, as it probably should be) isn't allowed for
> range partitions, then it's redundant; if it is allowed, then that
> case should be handled too.  Also, at this point we've already set
> *failed_at and *failed_slot; presumably you'd want to make this check
> before you get to that point.
>
> I suspect there are quite a few more problems here in addition to the
> ones mentioned above, but I don't think it makes sense to spend too
> much time searching for them until some of this basic stuff is cleaned
> up.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-04-24T10:54:00Z

On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
> Following can also be considered as it specifies more clearly that the
> partition holds default values.
>
> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;

Yes, that could be done.  But I don't think it's correct to say that
the partition holds default values.  Let's back up and ask what the
word "default" means.  The relevant definition (according to Google or
whoever they stole it from) is:

a preselected option adopted by a computer program or other mechanism
when no alternative is specified by the user or programmer.

So, a default *value* is the value that is used when no alternative is
specified by the user or programmer. We have that concept, but it's
not what we're talking about here: that's configured by applying the
DEFAULT property to a column.  Here, we're talking about the default
*partition*, or in other words the *partition* that is used when no
alternative is specified by the user or programmer.  So, that's why I
proposed the syntax I did.  The partition doesn't contain default
values; it is itself a default.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-04-24T12:14:57Z

On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>> Following can also be considered as it specifies more clearly that the
>> partition holds default values.
>>
>> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
>
> Yes, that could be done.  But I don't think it's correct to say that
> the partition holds default values.  Let's back up and ask what the
> word "default" means.  The relevant definition (according to Google or
> whoever they stole it from) is:
>
> a preselected option adopted by a computer program or other mechanism
> when no alternative is specified by the user or programmer.
>
> So, a default *value* is the value that is used when no alternative is
> specified by the user or programmer. We have that concept, but it's
> not what we're talking about here: that's configured by applying the
> DEFAULT property to a column.  Here, we're talking about the default
> *partition*, or in other words the *partition* that is used when no
> alternative is specified by the user or programmer.  So, that's why I
> proposed the syntax I did.  The partition doesn't contain default
> values; it is itself a default.

Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more natural.



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-04-24T18:38:30Z

Hi Rahila,

I tried to go through your v7 patch, and following are my comments:

1.
With -Werrors I see following compilation failure:

parse_utilcmd.c: In function ‘transformPartitionBound’:
parse_utilcmd.c:3309:4: error: implicit declaration of function
‘isDefaultPartitionBound’ [-Werror=implicit-function-declaration]
    if (!(isDefaultPartitionBound(value)))
    ^
cc1: all warnings being treated as errors

You need to include, "catalog/partitions.h".

2.
Once I made above change pass, I see following error:
tablecmds.c: In function ‘DefineRelation’:
tablecmds.c:762:17: error: unused variable ‘partdesc’
[-Werror=unused-variable]
   PartitionDesc partdesc;
                 ^
cc1: all warnings being treated as errors

3.
Please remove the extra line at the end of the function
check_new_partition_bound:
+ MemoryContextSwitchTo(oldCxt);
+ heap_endscan(scan);
+ UnregisterSnapshot(snapshot);
+ heap_close(defrel, AccessExclusiveLock);
+ ExecDropSingleTupleTableSlot(tupslot);
+ }
+
 }

4.
In generate_qual_for_defaultpart() you do not need 2 pointers for looping
over
bound specs:
+ ListCell   *cell1;
+ ListCell   *cell3;
You can iterate twice using one pointer itself.

Same is for:
+ ListCell   *cell2;
+ ListCell   *cell4;

Similarly, in get_qual_from_partbound(), you can use one pointer below,
instead of cell1 and cell3:
+ PartitionBoundSpec *bspec;
+ ListCell *cell1;
+ ListCell *cell3;

5.
Should this have a break in if block?
+ foreach(cell1, bspec->listdatums)
+ {
+ Node *value = lfirst(cell1);
+ if (isDefaultPartitionBound(value))
+ {
+ def_elem = true;
+ *defid = inhrelid;
+ }
+ }

6.
I am wondering, isn't it possible to retrieve the has_default and
default_index
here to find out if default partition exists and if exist then find it's oid
using rd_partdesc, that would avoid above(7) loop to check if partition
bound is
default.

7.
The output of describe needs to be improved.
Consider following case:
postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
(4,5,4,4,4,6,2);
ERROR:  relation "list_partitioned" does not exist
postgres=# CREATE TABLE list_partitioned (
    a int
) PARTITION BY LIST (a);
CREATE TABLE
postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
(4,5,4,4,4,6,2);
CREATE TABLE
postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
VALUES IN (DEFAULT, 3, DEFAULT, 3, DEFAULT);
CREATE TABLE
postgres=# \d+ part_1;
                                  Table "public.part_1"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
 a      | integer |           |          |         | plain   |
 |
Partition of: list_partitioned FOR VALUES IN (4, 5, 6, 2)

postgres=# \d+ part_default;
                               Table "public.part_default"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
 a      | integer |           |          |         | plain   |
 |
Partition of: list_partitioned FOR VALUES IN (DEFAULT3DEFAULTDEFAULT)

As you can see in above example, part_1 has multiple entries for 4 while
creating the partition, but describe shows only one entry for 4 in values
set.
Similarly, part_default has multiple entries for 3 and DEFAULT while
creating
the partition, but the describe shows a weired output. Instead, we should
have
just one entry saying "VALUES IN (DEFAULT, 3)":

postgres=# \d+ part_default;
                               Table "public.part_default"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
 a      | integer |           |          |         | plain   |
 |
Partition of: list_partitioned FOR VALUES IN (DEFAULT, 3)

8.
Following call to find_inheritance_children() in
generate_qual_for_defaultpart()
is an overhead, instead we can simply use an array of oids in rd_partdesc.

+ spec = (PartitionBoundSpec *) bound;
+
+ inhoids = find_inheritance_children(RelationGetRelid(parent), NoLock);
+
+ foreach(cell2, inhoids)

Same is for the call in get_qual_from_partbound:

+ /* Collect bound spec nodes in a list. This is done if the partition is
+ * a default partition. In case of default partition, constraint is formed
+ * by performing <> operation over the partition constraints of the
+ * existing partitions.
+ */
+ inhoids = find_inheritance_children(RelationGetRelid(parent), NoLock);
+ foreach(cell2, inhoids)

9.
How about rephrasing following error message:
postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN
(14);
ERROR:  new default partition constraint is violated by some row

To,
"ERROR: some existing row in default partition violates new default
partition constraint"

10.
Additionally, I did test your given sample test in first post and the one
mentioned by Keith; both of them are passing without errors.
Also, I did a pg_dump test and it is dumping the partitions and data
correctly.
But as mentioned earlier, it would be good if you have them in your patch.

I will do further review and let you know comments if any.

Regards,
Jeevan Ladhe

On Mon, Apr 24, 2017 at 5:44 PM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:

> On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas <robertmhaas@gmail.com>
> wrote:
> > On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed <rahilasyed90@gmail.com>
> wrote:
> >> Following can also be considered as it specifies more clearly that the
> >> partition holds default values.
> >>
> >> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
> >
> > Yes, that could be done.  But I don't think it's correct to say that
> > the partition holds default values.  Let's back up and ask what the
> > word "default" means.  The relevant definition (according to Google or
> > whoever they stole it from) is:
> >
> > a preselected option adopted by a computer program or other mechanism
> > when no alternative is specified by the user or programmer.
> >
> > So, a default *value* is the value that is used when no alternative is
> > specified by the user or programmer. We have that concept, but it's
> > not what we're talking about here: that's configured by applying the
> > DEFAULT property to a column.  Here, we're talking about the default
> > *partition*, or in other words the *partition* that is used when no
> > alternative is specified by the user or programmer.  So, that's why I
> > proposed the syntax I did.  The partition doesn't contain default
> > values; it is itself a default.
>
> Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more
> natural.
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-04-24T18:39:14Z

On Mon, Apr 24, 2017 at 5:44 PM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:

> On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas <robertmhaas@gmail.com>
> wrote:
> > On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed <rahilasyed90@gmail.com>
> wrote:
> >> Following can also be considered as it specifies more clearly that the
> >> partition holds default values.
> >>
> >> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
> >
> > Yes, that could be done.  But I don't think it's correct to say that
> > the partition holds default values.  Let's back up and ask what the
> > word "default" means.  The relevant definition (according to Google or
> > whoever they stole it from) is:
> >
> > a preselected option adopted by a computer program or other mechanism
> > when no alternative is specified by the user or programmer.
> >
> > So, a default *value* is the value that is used when no alternative is
> > specified by the user or programmer. We have that concept, but it's
> > not what we're talking about here: that's configured by applying the
> > DEFAULT property to a column.  Here, we're talking about the default
> > *partition*, or in other words the *partition* that is used when no
> > alternative is specified by the user or programmer.  So, that's why I
> > proposed the syntax I did.  The partition doesn't contain default
> > values; it is itself a default.
>
> Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more
> natural.
>

+1

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-04-24T20:16:36Z

On Mon, Apr 24, 2017 at 8:14 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>>> Following can also be considered as it specifies more clearly that the
>>> partition holds default values.
>>>
>>> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
>>
>> The partition doesn't contain default values; it is itself a default.
>
> Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more natural.

I suspect it could be done as of now, but I'm a little worried that it
might create grammar conflicts in the future as we extend the syntax
further.  If we use CREATE TABLE ... PARTITION OF .. DEFAULT, then the
word DEFAULT appears in the same position where we'd normally have FOR
VALUES, and so the parser will definitely be able to figure out what's
going on.  When it gets to that position, it will see FOR or it will
see DEFAULT, and all is clear.  OTOH, if we use CREATE TABLE ...
DEFAULT PARTITION OF ..., then we have action at a distance: whether
or not the word DEFAULT is present before PARTITION affects which
tokens are legal after the parent table name.  bison isn't always very
smart about that kind of thing.  No particular dangers come to mind at
the moment, but it makes me nervous anyway.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-04-25T00:29:50Z

On 2017/04/25 5:16, Robert Haas wrote:
> On Mon, Apr 24, 2017 at 8:14 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>>>> Following can also be considered as it specifies more clearly that the
>>>> partition holds default values.
>>>>
>>>> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
>>>
>>> The partition doesn't contain default values; it is itself a default.
>>
>> Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more natural.
> 
> I suspect it could be done as of now, but I'm a little worried that it
> might create grammar conflicts in the future as we extend the syntax
> further.  If we use CREATE TABLE ... PARTITION OF .. DEFAULT, then the
> word DEFAULT appears in the same position where we'd normally have FOR
> VALUES, and so the parser will definitely be able to figure out what's
> going on.  When it gets to that position, it will see FOR or it will
> see DEFAULT, and all is clear.  OTOH, if we use CREATE TABLE ...
> DEFAULT PARTITION OF ..., then we have action at a distance: whether
> or not the word DEFAULT is present before PARTITION affects which
> tokens are legal after the parent table name.  bison isn't always very
> smart about that kind of thing.  No particular dangers come to mind at
> the moment, but it makes me nervous anyway.

+1 to CREATE TABLE .. PARTITION OF .. DEFAULT

Thanks,
Amit

Re: Adding support for Default partition in partitioning

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-04-25T05:20:26Z

On Tue, Apr 25, 2017 at 1:46 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Apr 24, 2017 at 8:14 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>>>> Following can also be considered as it specifies more clearly that the
>>>> partition holds default values.
>>>>
>>>> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
>>>
>>> The partition doesn't contain default values; it is itself a default.
>>
>> Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more natural.
>
> I suspect it could be done as of now, but I'm a little worried that it
> might create grammar conflicts in the future as we extend the syntax
> further.  If we use CREATE TABLE ... PARTITION OF .. DEFAULT, then the
> word DEFAULT appears in the same position where we'd normally have FOR
> VALUES, and so the parser will definitely be able to figure out what's
> going on.  When it gets to that position, it will see FOR or it will
> see DEFAULT, and all is clear.  OTOH, if we use CREATE TABLE ...
> DEFAULT PARTITION OF ..., then we have action at a distance: whether
> or not the word DEFAULT is present before PARTITION affects which
> tokens are legal after the parent table name.

As long as we handle this at the transformation stage, it shouldn't be
a problem. The grammar would be something like
CREATE TABLE ... optDefault PARTITION OF ...

If user specifies DEFAULT PARTITION OF t1 FOR VALUES ..., parser will
allow that but in transformation stage, we will detect it and throw an
error "DEFAULT partitions can not contains partition bound clause" or
something like that. Also, documentation would say that DEFAULT and
partition bound specification are not allowed together.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: Adding support for Default partition in partitioning

Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-04-25T06:03:42Z

On 2017/04/25 14:20, Ashutosh Bapat wrote:
> On Tue, Apr 25, 2017 at 1:46 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Mon, Apr 24, 2017 at 8:14 AM, Ashutosh Bapat
>> <ashutosh.bapat@enterprisedb.com> wrote:
>>> On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>> On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>>>>> Following can also be considered as it specifies more clearly that the
>>>>> partition holds default values.
>>>>>
>>>>> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
>>>>
>>>> The partition doesn't contain default values; it is itself a default.
>>>
>>> Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more natural.
>>
>> I suspect it could be done as of now, but I'm a little worried that it
>> might create grammar conflicts in the future as we extend the syntax
>> further.  If we use CREATE TABLE ... PARTITION OF .. DEFAULT, then the
>> word DEFAULT appears in the same position where we'd normally have FOR
>> VALUES, and so the parser will definitely be able to figure out what's
>> going on.  When it gets to that position, it will see FOR or it will
>> see DEFAULT, and all is clear.  OTOH, if we use CREATE TABLE ...
>> DEFAULT PARTITION OF ..., then we have action at a distance: whether
>> or not the word DEFAULT is present before PARTITION affects which
>> tokens are legal after the parent table name.
> 
> As long as we handle this at the transformation stage, it shouldn't be
> a problem. The grammar would be something like
> CREATE TABLE ... optDefault PARTITION OF ...
> 
> If user specifies DEFAULT PARTITION OF t1 FOR VALUES ..., parser will
> allow that but in transformation stage, we will detect it and throw an
> error "DEFAULT partitions can not contains partition bound clause" or
> something like that. Also, documentation would say that DEFAULT and
> partition bound specification are not allowed together.

FWIW, one point to like about PARTITION OF .. DEFAULT is that it wouldn't
need us to do things you mention we could do.  A point to not like it may
be that it might read backwards to some users, but then the DEFAULT
PARTITION OF have all those possibilities of error-causing user input.

Thanks,
Amit

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-04-25T17:53:50Z

On Tue, Apr 25, 2017 at 1:20 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
>> I suspect it could be done as of now, but I'm a little worried that it
>> might create grammar conflicts in the future as we extend the syntax
>> further.  If we use CREATE TABLE ... PARTITION OF .. DEFAULT, then the
>> word DEFAULT appears in the same position where we'd normally have FOR
>> VALUES, and so the parser will definitely be able to figure out what's
>> going on.  When it gets to that position, it will see FOR or it will
>> see DEFAULT, and all is clear.  OTOH, if we use CREATE TABLE ...
>> DEFAULT PARTITION OF ..., then we have action at a distance: whether
>> or not the word DEFAULT is present before PARTITION affects which
>> tokens are legal after the parent table name.
>
> As long as we handle this at the transformation stage, it shouldn't be
> a problem. The grammar would be something like
> CREATE TABLE ... optDefault PARTITION OF ...
>
> If user specifies DEFAULT PARTITION OF t1 FOR VALUES ..., parser will
> allow that but in transformation stage, we will detect it and throw an
> error "DEFAULT partitions can not contains partition bound clause" or
> something like that. Also, documentation would say that DEFAULT and
> partition bound specification are not allowed together.

That's not what I'm concerned about.  I'm concerned about future
syntax additions resulting in difficult-to-resolve grammar conflicts.
For an example what of what I mean, consider this example:

http://postgr.es/m/9253.1295031520@sss.pgh.pa.us

The whole thread is worth a read.  In brief, I wanted to add syntax
like LOCK VIEW xyz, and it wasn't possible to do that without breaking
backward compatibility.  In a nutshell, the problem with making that
syntax work was that LOCK VIEW NOWAIT would then potentially mean
either lock a table called VIEW with the NOWAIT option, or else it
might mean lock a view called NOWAIT.  If the NOWAIT key word were not
allowed at the end or if the TABLE keyword were mandatory, then it
would be possible to make it work, but because we already decided both
to make the TABLE keyword optional and allow an optional NOWAIT
keyword at the end, the syntax couldn't be further extended in the way
that I wanted to extend it without confusing the parser.  The problem
was basically unfixable without breaking backward compatibility, and
we gave up.  I don't want to make the same mistake with the default
partition syntax that we made with the LOCK TABLE syntax.

Aside from unfixable grammar conflicts, there's another way that this
kind of syntax can become problematic, which is when you end up with
multiple optional keywords in the same part of the syntax.  For an
example of that, see
http://postgr.es/m/603c8f070905231747j2e099c23hef8eafbf26682e5f@mail.gmail.com
- that discusses the problems with EXPLAIN; we later ran into the same
problem with VACUUM.  Users can't remember whether they are supposed
to type VACUUM FULL VERBOSE or VACUUM VERBOSE FULL and trying to
support both creates parser problems and tends to involve adding too
many keywords, so we switched to a new and more extensible syntax for
future options.

Now, you may think that that's never going to happen in this case.
What optional keyword other than DEFAULT could we possibly want to add
just before PARTITION OF?  TBH, I don't know.  I can't think of
anything else we might want to put in that position right now.  But
considering that it's been less than six months since the original
syntax was committed and we've already thought of ONE thing we might
want to put there, it seems hard to rule out the possibility that we
might eventually think of more, and then we will have exactly the same
kind of problem that we've had in the past with other commands.  Let's
head the problem off at the pass and pick a syntax which isn't
vulnerable to that sort of issue.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-04-26T05:09:40Z

On Tue, Apr 25, 2017 at 11:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Apr 25, 2017 at 1:20 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>>> I suspect it could be done as of now, but I'm a little worried that it
>>> might create grammar conflicts in the future as we extend the syntax
>>> further.  If we use CREATE TABLE ... PARTITION OF .. DEFAULT, then the
>>> word DEFAULT appears in the same position where we'd normally have FOR
>>> VALUES, and so the parser will definitely be able to figure out what's
>>> going on.  When it gets to that position, it will see FOR or it will
>>> see DEFAULT, and all is clear.  OTOH, if we use CREATE TABLE ...
>>> DEFAULT PARTITION OF ..., then we have action at a distance: whether
>>> or not the word DEFAULT is present before PARTITION affects which
>>> tokens are legal after the parent table name.
>>
>> As long as we handle this at the transformation stage, it shouldn't be
>> a problem. The grammar would be something like
>> CREATE TABLE ... optDefault PARTITION OF ...
>>
>> If user specifies DEFAULT PARTITION OF t1 FOR VALUES ..., parser will
>> allow that but in transformation stage, we will detect it and throw an
>> error "DEFAULT partitions can not contains partition bound clause" or
>> something like that. Also, documentation would say that DEFAULT and
>> partition bound specification are not allowed together.
>
> That's not what I'm concerned about.  I'm concerned about future
> syntax additions resulting in difficult-to-resolve grammar conflicts.
> For an example what of what I mean, consider this example:
>
> http://postgr.es/m/9253.1295031520@sss.pgh.pa.us
>
> The whole thread is worth a read.  In brief, I wanted to add syntax
> like LOCK VIEW xyz, and it wasn't possible to do that without breaking
> backward compatibility.  In a nutshell, the problem with making that
> syntax work was that LOCK VIEW NOWAIT would then potentially mean
> either lock a table called VIEW with the NOWAIT option, or else it
> might mean lock a view called NOWAIT.  If the NOWAIT key word were not
> allowed at the end or if the TABLE keyword were mandatory, then it
> would be possible to make it work, but because we already decided both
> to make the TABLE keyword optional and allow an optional NOWAIT
> keyword at the end, the syntax couldn't be further extended in the way
> that I wanted to extend it without confusing the parser.  The problem
> was basically unfixable without breaking backward compatibility, and
> we gave up.  I don't want to make the same mistake with the default
> partition syntax that we made with the LOCK TABLE syntax.
>
> Aside from unfixable grammar conflicts, there's another way that this
> kind of syntax can become problematic, which is when you end up with
> multiple optional keywords in the same part of the syntax.  For an
> example of that, see
> http://postgr.es/m/603c8f070905231747j2e099c23hef8eafbf26682e5f@mail.gmail.com
> - that discusses the problems with EXPLAIN; we later ran into the same
> problem with VACUUM.  Users can't remember whether they are supposed
> to type VACUUM FULL VERBOSE or VACUUM VERBOSE FULL and trying to
> support both creates parser problems and tends to involve adding too
> many keywords, so we switched to a new and more extensible syntax for
> future options.
>

Thanks for taking out time for detailed explanation.

> Now, you may think that that's never going to happen in this case.
> What optional keyword other than DEFAULT could we possibly want to add
> just before PARTITION OF?

Since the grammar before PARTITION OF is shared with CREATE TABLE ()
there is high chance that we will have an optional keyword unrelated
to partitioning there. I take back my proposal for that syntax.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-04-26T13:42:49Z

Hello Jeevan,

Thank you for comments.

I will include your comments in the updated patch.

>7.The output of describe needs to be improved.

The syntax for DEFAULT partitioning is still under discussion. This comment
wont be
applicable if the syntax is changed.

>6.
>I am wondering, isn't it possible to retrieve the has_default and
default_index
>here to find out if default partition exists and if exist then find it's
oid
>using rd_partdesc, that would avoid above(7) loop to check if partition
bound is
>default
The checks are used to find the default partition bound and
exclude it from the list of partition bounds to form the partition
constraint.
This cant be accomplished by using has_default flag.
isDefaultPartitionBound() is written to accomplish that.


>8.
>Following call to find_inheritance_children() in
generate_qual_for_defaultpart()
>is an overhead, instead we can simply use an array of oids in rd_partdesc.
I think using find_inheritance_children() will take into consideration
concurrent
drop of a partition which the value in rd_partdesc will not.

Thank you,
Rahila Syed

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-04-27T12:49:44Z

>I suspect it could be done as of now, but I'm a little worried that it
>might create grammar conflicts in the future as we extend the syntax
>further.  If we use CREATE TABLE ... PARTITION OF .. DEFAULT, then the
>word DEFAULT appears in the same position where we'd normally have FOR
>VALUES, and so the parser will definitely be able to figure out what's
>going on.  When it gets to that position, it will see FOR or it will
>see DEFAULT, and all is clear.  OTOH, if we use CREATE TABLE ...
>DEFAULT PARTITION OF ..., then we have action at a distance: whether
>or not the word DEFAULT is present before PARTITION affects which
>tokens are legal after the parent table name.  bison isn't always very
>smart about that kind of thing.  No particular dangers come to mind at
>the moment, but it makes me nervous anyway.

+1 for CREATE TABLE..PARTITION OF...DEFAULT  syntax.
I think substituting DEFAULT for FOR VALUES is appropriate as
both cases are mutually exclusive.

One more thing that needs consideration is should default partitions be
partitioned further? Other databases allow default partitions to be
partitioned further. I think, its normal for users to expect the data in
default partitions to also be divided into sub partitions.  So
it should be supported.
My colleague Rajkumar Raghuwanshi brought to my notice the current patch
does not handle this correctly.
I will include this in the updated patch if there is no objection.

On the other hand if sub partitions of a default partition is to be
prohibited,
an error should be thrown if PARTITION BY is specified after DEFAULT.


Thank you,
Rahila Syed



On Tue, Apr 25, 2017 at 1:46 AM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Mon, Apr 24, 2017 at 8:14 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
> > On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas <robertmhaas@gmail.com>
> wrote:
> >> On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed <rahilasyed90@gmail.com>
> wrote:
> >>> Following can also be considered as it specifies more clearly that the
> >>> partition holds default values.
> >>>
> >>> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
> >>
> >> The partition doesn't contain default values; it is itself a default.
> >
> > Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more
> natural.
>
> I suspect it could be done as of now, but I'm a little worried that it
> might create grammar conflicts in the future as we extend the syntax
> further.  If we use CREATE TABLE ... PARTITION OF .. DEFAULT, then the
> word DEFAULT appears in the same position where we'd normally have FOR
> VALUES, and so the parser will definitely be able to figure out what's
> going on.  When it gets to that position, it will see FOR or it will
> see DEFAULT, and all is clear.  OTOH, if we use CREATE TABLE ...
> DEFAULT PARTITION OF ..., then we have action at a distance: whether
> or not the word DEFAULT is present before PARTITION affects which
> tokens are legal after the parent table name.  bison isn't always very
> smart about that kind of thing.  No particular dangers come to mind at
> the moment, but it makes me nervous anyway.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-04-27T13:07:26Z

On Thu, Apr 27, 2017 at 8:49 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>>I suspect it could be done as of now, but I'm a little worried that it
>>might create grammar conflicts in the future as we extend the syntax
>>further.  If we use CREATE TABLE ... PARTITION OF .. DEFAULT, then the
>>word DEFAULT appears in the same position where we'd normally have FOR
>>VALUES, and so the parser will definitely be able to figure out what's
>>going on.  When it gets to that position, it will see FOR or it will
>>see DEFAULT, and all is clear.  OTOH, if we use CREATE TABLE ...
>>DEFAULT PARTITION OF ..., then we have action at a distance: whether
>>or not the word DEFAULT is present before PARTITION affects which
>>tokens are legal after the parent table name.  bison isn't always very
>>smart about that kind of thing.  No particular dangers come to mind at
>>the moment, but it makes me nervous anyway.
>
> +1 for CREATE TABLE..PARTITION OF...DEFAULT  syntax.
> I think substituting DEFAULT for FOR VALUES is appropriate as
> both cases are mutually exclusive.
>
> One more thing that needs consideration is should default partitions be
> partitioned further? Other databases allow default partitions to be
> partitioned further. I think, its normal for users to expect the data in
> default partitions to also be divided into sub partitions.  So
> it should be supported.
> My colleague Rajkumar Raghuwanshi brought to my notice the current patch
> does not handle this correctly.
> I will include this in the updated patch if there is no objection.
>
> On the other hand if sub partitions of a default partition is to be
> prohibited,
> an error should be thrown if PARTITION BY is specified after DEFAULT.

I see no reason to prohibit it.  You can further partition any other
kind of partition, so there seems to be no reason to disallow it in
this one case.

Are you also working on extending this to work with range
partitioning?  Because I think that would be good to do.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Sven R. Kunze <srkunze@mail.de> — 2017-04-27T19:15:30Z

On 27.04.2017 15:07, Robert Haas wrote:
> On Thu, Apr 27, 2017 at 8:49 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>> +1 for CREATE TABLE..PARTITION OF...DEFAULT syntax.
>> I think substituting DEFAULT for FOR VALUES is appropriate as
>> both cases are mutually exclusive.

Just to make sound a little rounder:

CREATE TABLE ... PARTITION OF ... AS DEFAULT
CREATE TABLE ... PARTITION OF ... AS FALLBACK

or

CREATE TABLE ... PARTITION OF ... AS DEFAULT PARTITION
CREATE TABLE ... PARTITION OF ... AS FALLBACK PARTITION

Could any of these be feasible?

Sven

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-04-27T19:41:21Z

Hi,

On Apr 27, 2017 18:37, "Robert Haas" <robertmhaas@gmail.com> wrote:

>
>
> Are you also working on extending this to work with range
> partitioning?  Because I think that would be good to do.
>
>
> Currently I am working on review comments and bug fixes for the
default list partitioning patch. After that I can start with default
partition for range partitioning.

Thank you,
Rahila Syed

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-04-27T20:21:05Z

On Thu, Apr 27, 2017 at 3:15 PM, Sven R. Kunze <srkunze@mail.de> wrote:
> On 27.04.2017 15:07, Robert Haas wrote:
>> On Thu, Apr 27, 2017 at 8:49 AM, Rahila Syed <rahilasyed90@gmail.com>
>> wrote:
>>>
>>> +1 for CREATE TABLE..PARTITION OF...DEFAULT syntax.
>>> I think substituting DEFAULT for FOR VALUES is appropriate as
>>> both cases are mutually exclusive.
>
> Just to make sound a little rounder:
>
> CREATE TABLE ... PARTITION OF ... AS DEFAULT
> CREATE TABLE ... PARTITION OF ... AS FALLBACK
>
> or
>
> CREATE TABLE ... PARTITION OF ... AS DEFAULT PARTITION
> CREATE TABLE ... PARTITION OF ... AS FALLBACK PARTITION
>
> Could any of these be feasible?

FALLBACK wouldn't be a good choice because it's not an existing parser
keyword.  We could probably insert AS before DEFAULT and/or PARTITION
afterwards, but they sort of seem like noise words.  SQL seems to have
been invented by people who didn't have any trouble remembering really
long command strings, but brevity is not without some merit.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Sven R. Kunze <srkunze@mail.de> — 2017-04-30T19:27:02Z

On 27.04.2017 22:21, Robert Haas wrote:
> On Thu, Apr 27, 2017 at 3:15 PM, Sven R. Kunze <srkunze@mail.de> wrote:
>> Just to make sound a little rounder:
>>
>> CREATE TABLE ... PARTITION OF ... AS DEFAULT
>> CREATE TABLE ... PARTITION OF ... AS FALLBACK
>>
>> or
>>
>> CREATE TABLE ... PARTITION OF ... AS DEFAULT PARTITION
>> CREATE TABLE ... PARTITION OF ... AS FALLBACK PARTITION
>>
>> Could any of these be feasible?
> FALLBACK wouldn't be a good choice because it's not an existing parser
> keyword.  We could probably insert AS before DEFAULT and/or PARTITION
> afterwards, but they sort of seem like noise words.

You are right. I just thought it would make this variant more acceptable 
as people expressed concerns about understandability of the command.

> SQL seems to have
> been invented by people who didn't have any trouble remembering really
> long command strings, but brevity is not without some merit.

For me, it's exactly the thing I like about SQL. It makes for an easy 
learning curve.


Sven

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-05-02T16:03:03Z

Please find attached updated patch with review comments by Robert and
Jeevan implemented.

The newly proposed syntax
CREATE TABLE .. PARTITION OF .. DEFAULT has got most votes on this thread.

If there is no more objection, I will go ahead and include that in the
patch.

Thank you,
Rahila Syed

On Mon, Apr 24, 2017 at 2:40 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:

> Hello,
>
> Thank you for reviewing.
>
> >But that's not a good idea for several reasons.  For one thing, you
> >can also write FOR VALUES IN (DEFAULT, 5) or which isn't sensible.
> >For another thing, this kind of syntax won't generalize to range
> >partitioning, which we've talked about making this feature support.
> >Maybe something like:
>
> >CREATE TABLE .. PARTITION OF .. DEFAULT;
>
> I agree that the syntax should be changed to also support range
> partitioning.
>
> Following can also be considered as it specifies more clearly that the
> partition holds default values.
>
> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
>
> >Maybe we should introduce a dedicated node type to
> >represent a default-specification in the parser grammar.  If not, then
> >let's at least encapsulate the test a little better, e.g. by adding
> >isDefaultPartitionBound() which tests not only IsA(..., DefElem) but
> >also whether the name is DEFAULT as expected.  BTW, we typically use
> >lower-case internally, so if we stick with this representation it
> >should really be "default" not "DEFAULT".
>
> isDefaultPartitionBound() function is created in the attached patch which
> checks for both node type and name.
>
> >Why abbreviate "default" to def here?  Seems pointless.
> Corrected in the attached.
>
> >Consider &&
> Fixed.
>
> >+     * default partiton for rows satisfying the new partition
> >Spelling.
> Fixed.
>
> >Missing apostrophe
> Fixed.
>
> >Definitely not safe against concurrency, since AccessShareLock won't
> >exclude somebody else's update.  In fact, it won't even cover somebody
> >else's already-in-flight transaction
> Changed it to AccessExclusiveLock
>
> >Normally in such cases we try to give more detail using
> >ExecBuildSlotValueDescription.
> This function is used in execMain.c and the error is being
> reported in partition.c.
> Do you mean the error reporting should be moved into execMain.c
> to use ExecBuildSlotValueDescription?
>
> >This variable starts out true and is never set to any value other than
> >true.  Just get rid of it and, in the one place where it is currently
> >used, write "true".  That's shorter and clearer.
> Fixed.
>
> >There's not really a reason to cast the result of stringToNode() to
> >Node * and then turn around and cast it to PartitionBoundSpec *.  Just
> >cast it directly to whatever it needs to be.  And use the new castNode
> >macro
> Fixed. castNode macro takes as input Node * whereas stringToNode() takes
> string.
> IIUC, castNode cant be used here.
>
> >The if (def_elem) test continues
> >early, but if the point is that the loop using cell3 shouldn't execute
> >in that case, why not just put if (!def_elem) { foreach(cell3, ...) {
> >... } } instead of reiterating the ReleaseSysCache in two places?
> Fixed in the attached.
>
> I will respond to further comments in following email.
>
>
> On Thu, Apr 13, 2017 at 12:48 AM, Robert Haas <robertmhaas@gmail.com>
> wrote:
>
>> On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed <rahilasyed90@gmail.com>
>> wrote:
>> > Thanks a lot for testing and reporting this. Please find attached an
>> updated
>> > patch with the fix. The patch also contains a fix
>> > regarding operator used at the time of creating expression as default
>> > partition constraint. This was notified offlist by Amit Langote.
>>
>> I think that the syntax for this patch should probably be revised.
>> Right now the proposal is for:
>>
>> CREATE TABLE .. PARTITION OF ... FOR VALUES IN (DEFAULT);
>>
>> But that's not a good idea for several reasons.  For one thing, you
>> can also write FOR VALUES IN (DEFAULT, 5) or which isn't sensible.
>> For another thing, this kind of syntax won't generalize to range
>> partitioning, which we've talked about making this feature support.
>> Maybe something like:
>>
>> CREATE TABLE .. PARTITION OF .. DEFAULT;
>>
>> This patch makes the assumption throughout that any DefElem represents
>> the word DEFAULT, which is true in the patch as written but doesn't
>> seem very future-proof.  I think the "def" in "DefElem" stands for
>> "definition" or "define" or something like that, so this is actually
>> pretty confusing.  Maybe we should introduce a dedicated node type to
>> represent a default-specification in the parser grammar.  If not, then
>> let's at least encapsulate the test a little better, e.g. by adding
>> isDefaultPartitionBound() which tests not only IsA(..., DefElem) but
>> also whether the name is DEFAULT as expected.  BTW, we typically use
>> lower-case internally, so if we stick with this representation it
>> should really be "default" not "DEFAULT".
>>
>> Useless hunk:
>>
>> +    bool        has_def;        /* Is there a default partition?
>> Currently false
>> +                                 * for a range partitioned table */
>> +    int            def_index;        /* Index of the default list
>> partition. -1 for
>> +                                 * range partitioned tables */
>>
>> Why abbreviate "default" to def here?  Seems pointless.
>>
>> +                    if (found_def)
>> +                    {
>> +                        if (mapping[def_index] == -1)
>> +                            mapping[def_index] = next_index++;
>> +                    }
>>
>> Consider &&
>>
>> @@ -717,7 +754,6 @@ check_new_partition_bound(char *relname, Relation
>> parent, Node *bound)
>>                          }
>>                      }
>>                  }
>> -
>>                  break;
>>              }
>>
>> +     * default partiton for rows satisfying the new partition
>>
>> Spelling.
>>
>> +     * constraint. If found dont allow addition of a new partition.
>>
>> Missing apostrophe.
>>
>> +        defrel = heap_open(defid, AccessShareLock);
>> +        tupdesc = CreateTupleDescCopy(RelationGetDescr(defrel));
>> +
>> +        /* Build expression execution states for partition check quals */
>> +        partqualstate = ExecPrepareCheck(partConstraint,
>> +                        estate);
>> +
>> +        econtext = GetPerTupleExprContext(estate);
>> +        snapshot = RegisterSnapshot(GetLatestSnapshot());
>>
>> Definitely not safe against concurrency, since AccessShareLock won't
>> exclude somebody else's update.  In fact, it won't even cover somebody
>> else's already-in-flight transaction.
>>
>> +                errmsg("new default partition constraint is violated
>> by some row")));
>>
>> Normally in such cases we try to give more detail using
>> ExecBuildSlotValueDescription.
>>
>> +    bool        is_def = true;
>>
>> This variable starts out true and is never set to any value other than
>> true.  Just get rid of it and, in the one place where it is currently
>> used, write "true".  That's shorter and clearer.
>>
>> +    inhoids = find_inheritance_children(RelationGetRelid(parent),
>> NoLock);
>>
>> If it's actually safe to do this with no lock, there ought to be a
>> comment with a very compelling explanation of why it's safe.
>>
>> +        boundspec = (Node *) stringToNode(TextDatumGetCString(datum));
>> +        bspec = (PartitionBoundSpec *)boundspec;
>>
>> There's not really a reason to cast the result of stringToNode() to
>> Node * and then turn around and cast it to PartitionBoundSpec *.  Just
>> cast it directly to whatever it needs to be.  And use the new castNode
>> macro.
>>
>> +        foreach(cell1, bspec->listdatums)
>> +        {
>> +            Node *value = lfirst(cell1);
>> +            if (IsA(value, DefElem))
>> +            {
>> +                def_elem = true;
>> +                *defid = inhrelid;
>> +            }
>> +        }
>> +        if (def_elem)
>> +        {
>> +            ReleaseSysCache(tuple);
>> +            continue;
>> +        }
>> +        foreach(cell3, bspec->listdatums)
>> +        {
>> +            Node *value = lfirst(cell3);
>> +            boundspecs = lappend(boundspecs, value);
>> +        }
>> +        ReleaseSysCache(tuple);
>> +    }
>> +    foreach(cell4, spec->listdatums)
>> +    {
>> +        Node *value = lfirst(cell4);
>> +        boundspecs = lappend(boundspecs, value);
>> +    }
>>
>> cell1, cell2, cell3, and cell4 are not very clear variable names.
>> Between that and the lack of comments, this is not easy to understand.
>> It's sort of spaghetti logic, too.  The if (def_elem) test continues
>> early, but if the point is that the loop using cell3 shouldn't execute
>> in that case, why not just put if (!def_elem) { foreach(cell3, ...) {
>> ... } } instead of reiterating the ReleaseSysCache in two places?
>>
>> +                /* Collect bound spec nodes in a list. This is done
>> if the partition is
>> +                 * a default partition. In case of default partition,
>> constraint is formed
>> +                 * by performing <> operation over the partition
>> constraints of the
>> +                 * existing partitions.
>> +                 */
>>
>> I doubt that handles NULLs properly.
>>
>> +                inhoids =
>> find_inheritance_children(RelationGetRelid(parent), NoLock);
>>
>> Again, no lock?  Really?
>>
>> The logic which follows looks largely cut-and-pasted, which makes me
>> think you need to do some refactoring here to make it more clear
>> what's going on, so that you have the relevant logic in just one
>> place.  It seems wrong anyway to shove all of this logic specific to
>> the default case into get_qual_from_partbound() when the logic for the
>> non-default case is inside get_qual_for_list.  Where there were 2
>> lines of code before you've now got something like 30.
>>
>> +        if(get_negator(operoid) == InvalidOid)
>> +            elog(ERROR, "no negator found for partition operator %u",
>> +                 operoid);
>>
>> I really doubt that's OK.  elog() shouldn't be reachable, but this
>> will be reachable if the partitioning operator does not have a
>> negator.  And there's the NULL-handling issue I mentioned above, too.
>>
>> +            if (partdesc->boundinfo->has_def && key->strategy
>> +                == PARTITION_STRATEGY_LIST)
>> +                result = parent->indexes[partdesc->boun
>> dinfo->def_index];
>>
>> Testing for PARTITION_STRATEGY_LIST here seems unnecessary.  If
>> has_def (or has_default, as it probably should be) isn't allowed for
>> range partitions, then it's redundant; if it is allowed, then that
>> case should be handled too.  Also, at this point we've already set
>> *failed_at and *failed_slot; presumably you'd want to make this check
>> before you get to that point.
>>
>> I suspect there are quite a few more problems here in addition to the
>> ones mentioned above, but I don't think it makes sense to spend too
>> much time searching for them until some of this basic stuff is cleaned
>> up.
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>

Re: Adding support for Default partition in partitioning

Amul Sul <sulamul@gmail.com> — 2017-05-04T10:32:32Z

On Tue, May 2, 2017 at 9:33 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:
> Please find attached updated patch with review comments by Robert and Jeevan
> implemented.
>
Patch v8 got clean apply on latest head but server got crash at data
insert in the following test:

-- Create test table
CREATE TABLE test ( a int, b date) PARTITION BY LIST (a);
CREATE TABLE p1 PARTITION OF test FOR VALUES IN  (DEFAULT) PARTITION BY LIST(b);
CREATE TABLE p11 PARTITION OF p1 FOR VALUES IN (DEFAULT);

-- crash
INSERT INTO test VALUES (210,'1/1/2002');

Regards,
Amul

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-05-04T11:44:02Z

Hello Amul,

Thanks for reporting. Please find attached an updated patch which fixes the
above.
Also, the attached patch includes changes in syntax proposed upthread.

The syntax implemented in this patch is as follows,

CREATE TABLE p11 PARTITION OF p1 DEFAULT;

Thank you,
Rahila Syed

On Thu, May 4, 2017 at 4:02 PM, amul sul <sulamul@gmail.com> wrote:

> On Tue, May 2, 2017 at 9:33 PM, Rahila Syed <rahilasyed90@gmail.com>
> wrote:
> > Please find attached updated patch with review comments by Robert and
> Jeevan
> > implemented.
> >
> Patch v8 got clean apply on latest head but server got crash at data
> insert in the following test:
>
> -- Create test table
> CREATE TABLE test ( a int, b date) PARTITION BY LIST (a);
> CREATE TABLE p1 PARTITION OF test FOR VALUES IN  (DEFAULT) PARTITION BY
> LIST(b);
> CREATE TABLE p11 PARTITION OF p1 FOR VALUES IN (DEFAULT);
>
> -- crash
> INSERT INTO test VALUES (210,'1/1/2002');
>
> Regards,
> Amul
>

Re: Adding support for Default partition in partitioning

Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> — 2017-05-04T12:39:28Z

On Thu, May 4, 2017 at 5:14 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:

> The syntax implemented in this patch is as follows,
>
> CREATE TABLE p11 PARTITION OF p1 DEFAULT;
>
> Applied v9 patches, table description still showing old pattern of default
partition. Is it expected?

create table lpd (a int, b int, c varchar) partition by list(a);
create table lpd_d partition of lpd DEFAULT;

\d+ lpd
                                         Table "public.lpd"
 Column |       Type        | Collation | Nullable | Default | Storage  |
Stats target | Description
--------+-------------------+-----------+----------+---------+----------+--------------+-------------
 a      | integer           |           |          |         | plain
|              |
 b      | integer           |           |          |         | plain
|              |
 c      | character varying |           |          |         | extended
|              |
Partition key: LIST (a)
Partitions: lpd_d FOR VALUES IN (DEFAULT)

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-04T20:00:40Z

Hi Rahila,

I have started reviewing your latest patch, and here are my initial
comments:

1.
In following block, we can just do with def_index, and we do not need
found_def
flag. We can check if def_index is -1 or not to decide if default partition
is
present.

@@ -166,6 +172,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;

2.
In check_new_partition_bound, in case of PARTITION_STRATEGY_LIST, remove
following duplicate declaration of boundinfo, because it is confusing and
after
your changes it is not needed as its not getting overridden in the if block
locally.
if (partdesc->nparts > 0)
{
PartitionBoundInfo boundinfo = partdesc->boundinfo;
ListCell   *cell;

3.
In following function isDefaultPartitionBound, first statement "return
false"
is not needed.

+ * Returns true if the partition bound is default
+ */
+bool
+isDefaultPartitionBound(Node *value)
+{
+ if (IsA(value, DefElem))
+ {
+ DefElem *defvalue = (DefElem *) value;
+ if(!strcmp(defvalue->defname, "DEFAULT"))
+ return true;
+ return false;
+ }
+ return false;
+}

4.
As mentioned in my previous set of comments, following if block inside a
loop
in get_qual_for_default needs a break:

+ foreach(cell1, bspec->listdatums)
+ {
+ Node *value = lfirst(cell1);
+ if (isDefaultPartitionBound(value))
+ {
+ def_elem = true;
+ *defid  = inhrelid;
+ }
+ }

5.
In the grammar the rule default_part_list is not needed:

+default_partition:
+ DEFAULT  { $$ = (Node *)makeDefElem("DEFAULT", NULL, @1); }
+
+default_part_list:
+ default_partition { $$ = list_make1($1); }
+ ;
+

Instead you can simply declare default_partition as a list and write it as:

default_partition:
DEFAULT
{
Node *def = (Node *)makeDefElem("DEFAULT", NULL, @1);
$$ = list_make1(def);
}

6.
You need to change the output of the describe command, which is currently
as below: postgres=# \d+ test; Table "public.test" Column | Type |
Collation | Nullable | Default | Storage | Stats target | Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
a | integer | | | | plain | | b | date | | | | plain | | Partition key:
LIST (a) Partitions: pd FOR VALUES IN (DEFAULT), test_p1 FOR VALUES IN (4,
5) What about changing the Paritions output as below: Partitions: *pd
DEFAULT,* test_p1 FOR VALUES IN (4, 5)

7.
You need to handle tab completion for DEFAULT.
e.g.
If I partially type following command:
CREATE TABLE pd PARTITION OF test DEFA
and then press tab, I get following completion:
CREATE TABLE pd PARTITION OF test FOR VALUES

I did some primary testing and did not find any problem so far.

I will review and test further and let you know my comments.

Regards,
Jeevan Ladhe

On Thu, May 4, 2017 at 6:09 PM, Rajkumar Raghuwanshi <
rajkumar.raghuwanshi@enterprisedb.com> wrote:

> On Thu, May 4, 2017 at 5:14 PM, Rahila Syed <rahilasyed90@gmail.com>
> wrote:
>
>> The syntax implemented in this patch is as follows,
>>
>> CREATE TABLE p11 PARTITION OF p1 DEFAULT;
>>
>> Applied v9 patches, table description still showing old pattern of
> default partition. Is it expected?
>
> create table lpd (a int, b int, c varchar) partition by list(a);
> create table lpd_d partition of lpd DEFAULT;
>
> \d+ lpd
>                                          Table "public.lpd"
>  Column |       Type        | Collation | Nullable | Default | Storage  |
> Stats target | Description
> --------+-------------------+-----------+----------+--------
> -+----------+--------------+-------------
>  a      | integer           |           |          |         | plain
> |              |
>  b      | integer           |           |          |         | plain
> |              |
>  c      | character varying |           |          |         | extended
> |              |
> Partition key: LIST (a)
> Partitions: lpd_d FOR VALUES IN (DEFAULT)
>

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-04T20:28:19Z

While reviewing the code I was trying to explore more cases, and I here
comes an
open question to my mind:
should we allow the default partition table to be partitioned further?

If we allow it(as in the current case) then observe following case, where I
have defined a default partitioned which is further partitioned on a
different
column.

postgres=# CREATE TABLE test ( a int, b int, c int) PARTITION BY LIST (a);
CREATE TABLE
postgres=# CREATE TABLE test_p1 PARTITION OF test FOR VALUES IN(4, 5, 6, 7,
8);
CREATE TABLE
postgres=# CREATE TABLE test_pd PARTITION OF test DEFAULT PARTITION BY
LIST(b);
CREATE TABLE
postgres=# INSERT INTO test VALUES (20, 24, 12);
ERROR:  no partition of relation "test_pd" found for row
DETAIL:  Partition key of the failing row contains (b) = (24).

Note, that it does not allow inserting the tuple(20, 24, 12) because though
a=20
would fall in default partition i.e. test_pd, table test_pd itself is
further
partitioned and does not have any partition satisfying b=24.
Further if I define a default partition for table test_pd, the the tuple
gets inserted.

Doesn't this sound like the whole purpose of having DEFAULT partition on
test
table is defeated?

Any views?

Regards,
Jeevan Ladhe

Re: Adding support for Default partition in partitioning

Sven R. Kunze <srkunze@mail.de> — 2017-05-04T20:40:17Z

Hi Rahila,

still thinking about the syntax (sorry):

On 04.05.2017 13:44, Rahila Syed wrote:
> [...] The syntax implemented in this patch is as follows,
>
> CREATE TABLE p11 PARTITION OF p1 DEFAULT;

Rewriting the following:

> On Thu, May 4, 2017 at 4:02 PM, amul sul <sulamul@gmail.com 
> <mailto:sulamul@gmail.com>> wrote:
>
>     [...] CREATE TABLE p1 PARTITION OF test FOR VALUES IN  (DEFAULT)
>     PARTITION BY LIST(b); [...]
>

It yields

CREATE TABLE p1 PARTITION OF test DEFAULT PARTITION BY LIST(b);

This reads to me like "DEFAULT PARTITION".

I can imagine a lot of confusion when those queries are encountered in 
the wild. I know this thread is about creating a default partition but I 
were to propose a minor change in the following direction, I think 
confusion would be greatly avoided:

CREATE TABLE p1 PARTITION OF test*AS *DEFAULT PARTITION*ED* BY LIST(b);

I know it's a bit longer but I think those 4 characters might serve 
readability in the long term. It was especially confusing to see 
PARTITION in two positions serving two different functions.

Sven

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-05-08T16:26:21Z

On Thu, May 4, 2017 at 4:28 PM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> While reviewing the code I was trying to explore more cases, and I here
> comes an
> open question to my mind:
> should we allow the default partition table to be partitioned further?

I think yes.  In general, you are allowed to partition a partition,
and I can't see any justification for restricting that for default
partitions when we allow it everywhere else.

> If we allow it(as in the current case) then observe following case, where I
> have defined a default partitioned which is further partitioned on a
> different
> column.
>
> postgres=# CREATE TABLE test ( a int, b int, c int) PARTITION BY LIST (a);
> CREATE TABLE
> postgres=# CREATE TABLE test_p1 PARTITION OF test FOR VALUES IN(4, 5, 6, 7,
> 8);
> CREATE TABLE
> postgres=# CREATE TABLE test_pd PARTITION OF test DEFAULT PARTITION BY
> LIST(b);
> CREATE TABLE
> postgres=# INSERT INTO test VALUES (20, 24, 12);
> ERROR:  no partition of relation "test_pd" found for row
> DETAIL:  Partition key of the failing row contains (b) = (24).
>
> Note, that it does not allow inserting the tuple(20, 24, 12) because though
> a=20
> would fall in default partition i.e. test_pd, table test_pd itself is
> further
> partitioned and does not have any partition satisfying b=24.

Right, that looks like correct behavior.  You would have gotten the
same result if you had tried to insert into test_pd directly.

> Further if I define a default partition for table test_pd, the the tuple
> gets inserted.

That also sounds correct.

> Doesn't this sound like the whole purpose of having DEFAULT partition on
> test
> table is defeated?

Not to me.  It's possible to do lots of silly things with partitioned
tables.  For example, one case that we talked about before is that you
can define a range partition for, say, VALUES (0) TO (100), and then
subpartition it and give the subpartitions bounds which are outside
the range 0-100.  That's obviously silly and will lead to failures
inserting tuples, but we chose not to try to prohibit it because it's
not really broken, just useless.  There are lots of similar cases
involving other features.  For example, you can apply an inherited
CHECK (false) constraint to a table, which makes it impossible for
that table or any of its children to ever contain any rows; that is
probably a dumb configuration.  You can create two unique indexes with
exactly the same definition; unless you're creating a new one with the
intent of dropping the old one, that doesn't make sense.  You can
define a trigger that always throws an ERROR and then another trigger
which runs later that modifies the tuple; the second will never be run
because the first one will always kill the transaction before we get
there.  Those things are all legal, but often unuseful.  Similarly
here.  Defining a default list partition and then subpartitioning it
by list is not likely to be a good schema design, but it doesn't mean
we should try to disallow it.  It is important to distinguish between
things that are actually *broken* (like a partitioning configuration
where the tuples that can be inserted into a partition manually differ
from the ones that are routed to it automatically) and things that are
merely *lame* (like creating a multi-level partitioning hierarchy when
a single level would have done the job just as well).  The former
should be prevented by the code, while the latter is at most a
documentation issue.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-08T18:47:39Z

Hi Robert,

Thanks for your explnation.

On Mon, May 8, 2017 at 9:56 PM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Thu, May 4, 2017 at 4:28 PM, Jeevan Ladhe
> <jeevan.ladhe@enterprisedb.com> wrote:
> > While reviewing the code I was trying to explore more cases, and I here
> > comes an
> > open question to my mind:
> > should we allow the default partition table to be partitioned further?
>
> I think yes.  In general, you are allowed to partition a partition,
> and I can't see any justification for restricting that for default
> partitions when we allow it everywhere else.
>
> > If we allow it(as in the current case) then observe following case,
> where I
> > have defined a default partitioned which is further partitioned on a
> > different
> > column.
> >
> > postgres=# CREATE TABLE test ( a int, b int, c int) PARTITION BY LIST
> (a);
> > CREATE TABLE
> > postgres=# CREATE TABLE test_p1 PARTITION OF test FOR VALUES IN(4, 5, 6,
> 7,
> > 8);
> > CREATE TABLE
> > postgres=# CREATE TABLE test_pd PARTITION OF test DEFAULT PARTITION BY
> > LIST(b);
> > CREATE TABLE
> > postgres=# INSERT INTO test VALUES (20, 24, 12);
> > ERROR:  no partition of relation "test_pd" found for row
> > DETAIL:  Partition key of the failing row contains (b) = (24).
> >
> > Note, that it does not allow inserting the tuple(20, 24, 12) because
> though
> > a=20
> > would fall in default partition i.e. test_pd, table test_pd itself is
> > further
> > partitioned and does not have any partition satisfying b=24.
>
> Right, that looks like correct behavior.  You would have gotten the
> same result if you had tried to insert into test_pd directly.
>
> > Further if I define a default partition for table test_pd, the the tuple
> > gets inserted.
>
> That also sounds correct.
>
> > Doesn't this sound like the whole purpose of having DEFAULT partition on
> > test
> > table is defeated?
>
> Not to me.  It's possible to do lots of silly things with partitioned
> tables.  For example, one case that we talked about before is that you
> can define a range partition for, say, VALUES (0) TO (100), and then
> subpartition it and give the subpartitions bounds which are outside
> the range 0-100.  That's obviously silly and will lead to failures
> inserting tuples, but we chose not to try to prohibit it because it's
> not really broken, just useless.  There are lots of similar cases
> involving other features.  For example, you can apply an inherited
> CHECK (false) constraint to a table, which makes it impossible for
> that table or any of its children to ever contain any rows; that is
> probably a dumb configuration.  You can create two unique indexes with
> exactly the same definition; unless you're creating a new one with the
> intent of dropping the old one, that doesn't make sense.  You can
> define a trigger that always throws an ERROR and then another trigger
> which runs later that modifies the tuple; the second will never be run
> because the first one will always kill the transaction before we get
> there.  Those things are all legal, but often unuseful.  Similarly
> here.  Defining a default list partition and then subpartitioning it
> by list is not likely to be a good schema design, but it doesn't mean
> we should try to disallow it.  It is important to distinguish between
> things that are actually *broken* (like a partitioning configuration
> where the tuples that can be inserted into a partition manually differ
> from the ones that are routed to it automatically) and things that are
> merely *lame* (like creating a multi-level partitioning hierarchy when
> a single level would have done the job just as well).  The former
> should be prevented by the code, while the latter is at most a
> documentation issue.


I agree with you that it is a user perspective on how he decides to do
partitions of already partitioned table, and also we should have a
demarcation between things to be handled by code and things to be
left as common-sense or ability to define a good schema.

I am ok with current behavior, provided we have atleast one-lineer in
documentation alerting the user that partitioning the default partition will
limit the ability of routing the tuples that do not fit in any other
partitions.

Regards,
Jeevan Ladhe

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-05-08T19:43:30Z

On Thu, May 4, 2017 at 4:40 PM, Sven R. Kunze <srkunze@mail.de> wrote:
> It yields
>
> CREATE TABLE p1 PARTITION OF test DEFAULT PARTITION BY LIST(b);
>
> This reads to me like "DEFAULT PARTITION".
>
> I can imagine a lot of confusion when those queries are encountered in the
> wild. I know this thread is about creating a default partition but I were to
> propose a minor change in the following direction, I think confusion would
> be greatly avoided:
>
> CREATE TABLE p1 PARTITION OF test AS DEFAULT PARTITIONED BY LIST(b);
>
> I know it's a bit longer but I think those 4 characters might serve
> readability in the long term. It was especially confusing to see PARTITION
> in two positions serving two different functions.

Well, we certainly can't make that change just for default partitions.
I mean, that would be non-orthogonal, right?  You can't say that the
way to subpartition is to write "PARTITION BY strategy" when the table
unpartitioned or is a non-default partition but "PARTITIONED BY
strategy" when it is a default partition.  That would certainly not be
a good way of confusing users less, and would probably result in a
variety of special cases in places like ruleutils.c or pg_dump, plus
some weasel-wording in the documentation.  We COULD do a general
change from "CREATE TABLE table_name PARTITION BY strategy" to "CREATE
TABLE table_name PARTITIONED BY strategy".  I don't have any
particular arguments against that except that the current syntax is
more like Oracle, which might count for something, and maybe the fact
that we're a month after feature freeze.  Still, if we want to change
that, now would be the time; but I favor leaving it alone.

I don't have a big objection to adding AS.  If that's the majority
vote, fine; if not, that's OK, too.  I can see it might be a bit more
clear in the case you mention, but it might also just be a noise word
that we don't really need.  There don't seem to be many uses of AS
that would pose a risk of actual grammar conflicts here.  I can
imagine someone wanting to use CREATE TABLE ... PARTITION BY ... AS
SELECT ... to create and populate a partition in one command, but that
wouldn't be a conflict because it'd have to go AFTER the partition
specification.  In the DEFAULT case, you'd end up with something like

CREATE TABLE p1 PARTITION OF test AS DEFAULT AS <query>

...which is neither great nor horrible syntax-wise and maybe not such
a good thing to support anyway since it would have to lock the parent
to add the partition and then keep the lock on the parent while
populating the new child (ouch).

So I guess I'm still in favor of the CREATE TABLE p1 PARTITION OF test
DEFAULT syntax, but if it ends up being AS DEFAULT instead, I can live
with that.

Other opinions?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-05-09T07:19:10Z

+1 for AS DEFAULT syntax if it helps in improving readability specially in
following case

CREATE TABLE p1 PARTITION OF test AS DEFAULT PARTITION BY LIST(a);

Thank you,
Rahila Syed

On Tue, May 9, 2017 at 1:13 AM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Thu, May 4, 2017 at 4:40 PM, Sven R. Kunze <srkunze@mail.de> wrote:
> > It yields
> >
> > CREATE TABLE p1 PARTITION OF test DEFAULT PARTITION BY LIST(b);
> >
> > This reads to me like "DEFAULT PARTITION".
> >
> > I can imagine a lot of confusion when those queries are encountered in
> the
> > wild. I know this thread is about creating a default partition but I
> were to
> > propose a minor change in the following direction, I think confusion
> would
> > be greatly avoided:
> >
> > CREATE TABLE p1 PARTITION OF test AS DEFAULT PARTITIONED BY LIST(b);
> >
> > I know it's a bit longer but I think those 4 characters might serve
> > readability in the long term. It was especially confusing to see
> PARTITION
> > in two positions serving two different functions.
>
> Well, we certainly can't make that change just for default partitions.
> I mean, that would be non-orthogonal, right?  You can't say that the
> way to subpartition is to write "PARTITION BY strategy" when the table
> unpartitioned or is a non-default partition but "PARTITIONED BY
> strategy" when it is a default partition.  That would certainly not be
> a good way of confusing users less, and would probably result in a
> variety of special cases in places like ruleutils.c or pg_dump, plus
> some weasel-wording in the documentation.  We COULD do a general
> change from "CREATE TABLE table_name PARTITION BY strategy" to "CREATE
> TABLE table_name PARTITIONED BY strategy".  I don't have any
> particular arguments against that except that the current syntax is
> more like Oracle, which might count for something, and maybe the fact
> that we're a month after feature freeze.  Still, if we want to change
> that, now would be the time; but I favor leaving it alone.
>
> I don't have a big objection to adding AS.  If that's the majority
> vote, fine; if not, that's OK, too.  I can see it might be a bit more
> clear in the case you mention, but it might also just be a noise word
> that we don't really need.  There don't seem to be many uses of AS
> that would pose a risk of actual grammar conflicts here.  I can
> imagine someone wanting to use CREATE TABLE ... PARTITION BY ... AS
> SELECT ... to create and populate a partition in one command, but that
> wouldn't be a conflict because it'd have to go AFTER the partition
> specification.  In the DEFAULT case, you'd end up with something like
>
> CREATE TABLE p1 PARTITION OF test AS DEFAULT AS <query>
>
> ...which is neither great nor horrible syntax-wise and maybe not such
> a good thing to support anyway since it would have to lock the parent
> to add the partition and then keep the lock on the parent while
> populating the new child (ouch).
>
> So I guess I'm still in favor of the CREATE TABLE p1 PARTITION OF test
> DEFAULT syntax, but if it ends up being AS DEFAULT instead, I can live
> with that.
>
> Other opinions?
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: Adding support for Default partition in partitioning

Sven R. Kunze <srkunze@mail.de> — 2017-05-10T14:59:58Z

On 09.05.2017 09:19, Rahila Syed wrote:
> +1 for AS DEFAULT syntax if it helps in improving readability 
> specially in following case
>
> CREATE TABLE p1 PARTITION OF test AS DEFAULT PARTITION BY LIST(a);
>
> Thank you,
> Rahila Syed
>
> On Tue, May 9, 2017 at 1:13 AM, Robert Haas <robertmhaas@gmail.com 
> <mailto:robertmhaas@gmail.com>> wrote:
>
>     On Thu, May 4, 2017 at 4:40 PM, Sven R. Kunze <srkunze@mail.de
>     <mailto:srkunze@mail.de>> wrote:
>     > It yields
>     >
>     > CREATE TABLE p1 PARTITION OF test DEFAULT PARTITION BY LIST(b);
>     >
>     > This reads to me like "DEFAULT PARTITION".
>     >
>     > I can imagine a lot of confusion when those queries are
>     encountered in the
>     > wild. I know this thread is about creating a default partition
>     but I were to
>     > propose a minor change in the following direction, I think
>     confusion would
>     > be greatly avoided:
>     >
>     > CREATE TABLE p1 PARTITION OF test AS DEFAULT PARTITIONED BY LIST(b);
>     >
>     > I know it's a bit longer but I think those 4 characters might serve
>     > readability in the long term. It was especially confusing to see
>     PARTITION
>     > in two positions serving two different functions.
>
>     Well, we certainly can't make that change just for default partitions.
>     I mean, that would be non-orthogonal, right?  You can't say that the
>     way to subpartition is to write "PARTITION BY strategy" when the table
>     unpartitioned or is a non-default partition but "PARTITIONED BY
>     strategy" when it is a default partition.  That would certainly not be
>     a good way of confusing users less, and would probably result in a
>     variety of special cases in places like ruleutils.c or pg_dump, plus
>     some weasel-wording in the documentation.  We COULD do a general
>     change from "CREATE TABLE table_name PARTITION BY strategy" to "CREATE
>     TABLE table_name PARTITIONED BY strategy".  I don't have any
>     particular arguments against that except that the current syntax is
>     more like Oracle, which might count for something, and maybe the fact
>     that we're a month after feature freeze.  Still, if we want to change
>     that, now would be the time; but I favor leaving it alone.
>

You are definitely right. Changing it here would require to change it 
everywhere AND thus to loose syntax parity with Oracle.

I am not in a position to judge this properly whether this would be a 
huge problem. Personally, I don't have an issue with that. But don't 
count me as most important opion on this.

>
>     So I guess I'm still in favor of the CREATE TABLE p1 PARTITION OF test
>     DEFAULT syntax, but if it ends up being AS DEFAULT instead, I can live
>     with that.
>

Is to make it optional an option?

Sven

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-05-10T15:59:58Z

On Wed, May 10, 2017 at 10:59 AM, Sven R. Kunze <srkunze@mail.de> wrote:
> You are definitely right. Changing it here would require to change it
> everywhere AND thus to loose syntax parity with Oracle.

Right.

> I am not in a position to judge this properly whether this would be a huge
> problem. Personally, I don't have an issue with that. But don't count me as
> most important opion on this.

Well, I don't think it would be a HUGE problem, but I think the fact
that Amit chose to implement this with syntax similar to that of
Oracle is probably not a coincidence, but rather a goal, and I think
the readability problem that you're worrying about is really pretty
minor.  I think most people aren't going to subpartition their default
partition, and I think those who do will probably find the syntax
clear enough anyway.   So I don't favor changing it.  Now, if there's
an outcry of support for your position then I'll stand aside but I
don't anticipate that.

>> So I guess I'm still in favor of the CREATE TABLE p1 PARTITION OF test
>> DEFAULT syntax, but if it ends up being AS DEFAULT instead, I can live
>> with that.
>
> Is to make it optional an option?

Optional keywords may not be the root of ALL evil, but they're pretty
evil.  See my posting earlier on this same thread on this topic:

http://postgr.es/m/CA+TgmoZGHgd3vKZvyQ1Qx3e0L3n=voxY57mz9TTncVET-aLK2A@mail.gmail.com

The issues here are more or less the same.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-05-10T16:12:23Z

I'm surprised that there is so much activity in this thread.  Is this
patch being considered for pg10?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-05-10T18:10:56Z

On Wed, May 10, 2017 at 12:12 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> I'm surprised that there is so much activity in this thread.  Is this
> patch being considered for pg10?

Of course not.  Feature freeze was a month ago.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Sven R. Kunze <srkunze@mail.de> — 2017-05-10T20:21:42Z

On 10.05.2017 17:59, Robert Haas wrote:
> Well, I don't think it would be a HUGE problem, but I think the fact
> that Amit chose to implement this with syntax similar to that of
> Oracle is probably not a coincidence, but rather a goal, and I think
> the readability problem that you're worrying about is really pretty
> minor.  I think most people aren't going to subpartition their default
> partition, and I think those who do will probably find the syntax
> clear enough anyway.

I agree here.

> Optional keywords may not be the root of ALL evil, but they're pretty
> evil.  See my posting earlier on this same thread on this topic:
>
> http://postgr.es/m/CA+TgmoZGHgd3vKZvyQ1Qx3e0L3n=voxY57mz9TTncVET-aLK2A@mail.gmail.com
>
> The issues here are more or less the same.

Ah, I see. I didn't draw the conclusion from the optionality of a 
keyword the other day but after re-reading your post, it's exactly the 
same issue.
Let's avoid optional keywords!

Sven

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-05-11T14:07:53Z

Hello,

Please find attached an updated patch with review comments and bugs
reported till date implemented.

>1.
>In following block, we can just do with def_index, and we do not need
found_def
>flag. We can check if def_index is -1 or not to decide if default partition
is
>present.
found_def is used to set boundinfo->has_default which is used at couple
of other places to check if default partition exists. The implementation is
similar
to has_null.

>3.
>In following function isDefaultPartitionBound, first statement "return
false"
>is not needed.
It is needed to return false if the node is not DefElem.

Todo:
Add regression tests
Documentation

Thank you,
Rahila Syed



On Fri, May 5, 2017 at 1:30 AM, Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>
wrote:

> Hi Rahila,
>
> I have started reviewing your latest patch, and here are my initial
> comments:
>
> 1.
> In following block, we can just do with def_index, and we do not need
> found_def
> flag. We can check if def_index is -1 or not to decide if default
> partition is
> present.
>
> @@ -166,6 +172,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;
>
> 2.
> In check_new_partition_bound, in case of PARTITION_STRATEGY_LIST, remove
> following duplicate declaration of boundinfo, because it is confusing and
> after
> your changes it is not needed as its not getting overridden in the if block
> locally.
> if (partdesc->nparts > 0)
> {
> PartitionBoundInfo boundinfo = partdesc->boundinfo;
> ListCell   *cell;
>
>
> 3.
> In following function isDefaultPartitionBound, first statement "return
> false"
> is not needed.
>
> + * Returns true if the partition bound is default
> + */
> +bool
> +isDefaultPartitionBound(Node *value)
> +{
> + if (IsA(value, DefElem))
> + {
> + DefElem *defvalue = (DefElem *) value;
> + if(!strcmp(defvalue->defname, "DEFAULT"))
> + return true;
> + return false;
> + }
> + return false;
> +}
>
> 4.
> As mentioned in my previous set of comments, following if block inside a
> loop
> in get_qual_for_default needs a break:
>
> + foreach(cell1, bspec->listdatums)
> + {
> + Node *value = lfirst(cell1);
> + if (isDefaultPartitionBound(value))
> + {
> + def_elem = true;
> + *defid  = inhrelid;
> + }
> + }
>
> 5.
> In the grammar the rule default_part_list is not needed:
>
> +default_partition:
> + DEFAULT  { $$ = (Node *)makeDefElem("DEFAULT", NULL, @1); }
> +
> +default_part_list:
> + default_partition { $$ = list_make1($1); }
> + ;
> +
>
> Instead you can simply declare default_partition as a list and write it as:
>
> default_partition:
> DEFAULT
> {
> Node *def = (Node *)makeDefElem("DEFAULT", NULL, @1);
> $$ = list_make1(def);
> }
>
> 6.
> You need to change the output of the describe command, which is currently
> as below: postgres=# \d+ test; Table "public.test" Column | Type |
> Collation | Nullable | Default | Storage | Stats target | Description
> --------+---------+-----------+----------+---------+---------+--------------+-------------
> a | integer | | | | plain | | b | date | | | | plain | | Partition key:
> LIST (a) Partitions: pd FOR VALUES IN (DEFAULT), test_p1 FOR VALUES IN (4,
> 5) What about changing the Paritions output as below: Partitions: *pd
> DEFAULT,* test_p1 FOR VALUES IN (4, 5)
>
> 7.
> You need to handle tab completion for DEFAULT.
> e.g.
> If I partially type following command:
> CREATE TABLE pd PARTITION OF test DEFA
> and then press tab, I get following completion:
> CREATE TABLE pd PARTITION OF test FOR VALUES
>
> I did some primary testing and did not find any problem so far.
>
> I will review and test further and let you know my comments.
>
> Regards,
> Jeevan Ladhe
>
> On Thu, May 4, 2017 at 6:09 PM, Rajkumar Raghuwanshi <
> rajkumar.raghuwanshi@enterprisedb.com> wrote:
>
>> On Thu, May 4, 2017 at 5:14 PM, Rahila Syed <rahilasyed90@gmail.com>
>> wrote:
>>
>>> The syntax implemented in this patch is as follows,
>>>
>>> CREATE TABLE p11 PARTITION OF p1 DEFAULT;
>>>
>>> Applied v9 patches, table description still showing old pattern of
>> default partition. Is it expected?
>>
>> create table lpd (a int, b int, c varchar) partition by list(a);
>> create table lpd_d partition of lpd DEFAULT;
>>
>> \d+ lpd
>>                                          Table "public.lpd"
>>  Column |       Type        | Collation | Nullable | Default | Storage  |
>> Stats target | Description
>> --------+-------------------+-----------+----------+--------
>> -+----------+--------------+-------------
>>  a      | integer           |           |          |         | plain
>> |              |
>>  b      | integer           |           |          |         | plain
>> |              |
>>  c      | character varying |           |          |         | extended
>> |              |
>> Partition key: LIST (a)
>> Partitions: lpd_d FOR VALUES IN (DEFAULT)
>>
>
>

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-05-12T02:38:37Z

On Thu, May 11, 2017 at 10:07 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
> Please find attached an updated patch with review comments and bugs reported
> till date implemented.

You haven't done anything about the repeated suggestion that this
should also cover range partitioning.

+            /*
+             * If the partition is the default partition switch
+             * back to PARTITION_STRATEGY_LIST
+             */
+            if (spec->strategy == PARTITION_DEFAULT)
+                result_spec->strategy = PARTITION_STRATEGY_LIST;
+            else
+                ereport(ERROR,
+                        (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+                     errmsg("invalid bound specification for a list
partition"),
                      parser_errposition(pstate, exprLocation(bound))));

This is incredibly ugly.  I don't know exactly what should be done
about it, but I think PARTITION_DEFAULT is a bad idea and has got to
go.  Maybe add a separate isDefault flag to PartitionBoundSpec.

+            /*
+             * Skip if it's a partitioned table.  Only RELKIND_RELATION
+             * relations (ie, leaf partitions) need to be scanned.
+             */
+            if (part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)

What about foreign table partitions?

Doesn't it strike you as a bit strange that get_qual_for_default()
doesn't return a qual?  Functions should generally have names that
describe what they do.

+    bound_datums = list_copy(spec->listdatums);
+
+    boundspecs = get_qual_for_default(parent, defid);
+
+    foreach(cell, bound_datums)
+    {
+        Node *value = lfirst(cell);
+        boundspecs = lappend(boundspecs, value);
+    }

There's an existing function that you can use to concatenate two lists
instead of open-coding it.

Also, I think that before you ask anyone to spend too much more time
and energy reviewing this, you should really add the documentation and
regression tests which you mentioned as a TODO.  And run the code
through pgindent.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-12T06:11:51Z

Hi Rahila,

On Thu, May 11, 2017 at 7:37 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>
> >3.
> >In following function isDefaultPartitionBound, first statement "return
> false"
> >is not needed.
> It is needed to return false if the node is not DefElem.
>

Please have a look at following code:

+ * Returns true if the partition bound is default
+ */
+bool
+isDefaultPartitionBound(Node *value)
+{
+ if (IsA(value, DefElem))
+ {
+ DefElem defvalue = (DefElem ) value;
+ if(!strcmp(defvalue->defname, "DEFAULT"))
+ return true;
+ return false;
+ }
+ return false;
+}

By first return false, I mean to say the return statement inside the
if block "if (IsA(value, DefElem))":

+ if(!strcmp(defvalue->defname, "DEFAULT"))
+ return true;
+ return false;

Even if this "return false" is not present, the control is anyway going to
fall through and will return false from the outermost return statement.

I leave this decision to you, but further this block could be rewritten as
below and also can be defined as a macro:

bool
isDefaultPartitionBound(Node *value)
{
return (IsA(value, DefElem) &&
!strcmp(((DefElem) value)->defname, "DEFAULT"));
}

Regards,
Jeevan Ladhe

Re: Adding support for Default partition in partitioning

Beena Emerson <memissemerson@gmail.com> — 2017-05-12T11:03:17Z

On Thu, May 11, 2017 at 7:37 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:

> Hello,
>
> Please find attached an updated patch with review comments and bugs
> reported till date implemented.
>

Hello Rahila,

Tested on "efa2c18 Doc fix: scale(numeric) returns integer, not numeric."

(1) With the new patch, we allow new partitions when there is overlapping
data with default partition. The entries in default are ignored when
running queries satisfying the new partition.

DROP TABLE list1;
CREATE TABLE list1 (
    a int,
    b int
) PARTITION BY LIST (a);
CREATE TABLE list1_1 (LIKE list1);
ALTER TABLE  list1 ATTACH PARTITION list1_1 FOR VALUES IN (1);
CREATE TABLE list1_def PARTITION OF list1 DEFAULT;
INSERT INTO list1 SELECT generate_series(1,2),1;
-- Partition overlapping with DEF
CREATE TABLE list1_2 PARTITION OF list1 FOR VALUES IN (2);
INSERT INTO list1 SELECT generate_series(2,3),2;

postgres=# SELECT * FROM list1 ORDER BY a,b;
 a | b
---+---
 1 | 1
 2 | 1
 2 | 2
 3 | 2
(4 rows)

postgres=# SELECT * FROM list1 WHERE a=2;
 a | b
---+---
 2 | 2
(1 row)

This ignores the a=2 entries in the DEFAULT.

postgres=# SELECT * FROM list1_def;
 a | b
---+---
 2 | 1
 3 | 2
(2 rows)


(2) I get the following warning:

partition.c: In function ‘check_new_partition_bound’:
partition.c:882:15: warning: ‘boundinfo’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
   && boundinfo->has_default)
               ^
preproc.y:3250.2-8: warning: type clash on default action: <str> != <>


> >1.
> >In following block, we can just do with def_index, and we do not need
> found_def
> >flag. We can check if def_index is -1 or not to decide if default
> partition is
> >present.
> found_def is used to set boundinfo->has_default which is used at couple
> of other places to check if default partition exists. The implementation
> is similar
> to has_null.
>
> >3.
> >In following function isDefaultPartitionBound, first statement "return
> false"
> >is not needed.
> It is needed to return false if the node is not DefElem.
>
> Todo:
> Add regression tests
> Documentation
>
> Thank you,
> Rahila Syed
>
>
>

Re: Adding support for Default partition in partitioning

Rahila Syed <rahilasyed90@gmail.com> — 2017-05-12T12:00:17Z

Hello,

>(1) With the new patch, we allow new partitions when there is overlapping
data with default partition. The entries in default are ignored when
running queries satisfying the new partition.
This was introduced in latest version. We are not allowing adding a
partition when entries with same key value exist in default partition. So
this scenario should not
come in picture. Please find attached an updated patch which corrects this.

>(2) I get the following warning:

>partition.c: In function ‘check_new_partition_bound’:
>partition.c:882:15: warning: ‘boundinfo’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
>   && boundinfo->has_default)
               ^
>preproc.y:3250.2-8: warning: type clash on default action: <str> != <>
I failed to notice this warning. I will look into it.

>This is incredibly ugly.  I don't know exactly what should be done
>about it, but I think PARTITION_DEFAULT is a bad idea and has got to
>go.  Maybe add a separate isDefault flag to PartitionBoundSpec
Will look at other ways to do it.

>Doesn't it strike you as a bit strange that get_qual_for_default()
>doesn't return a qual?  Functions should generally have names that
>describe what they do.
Will fix this.

>There's an existing function that you can use to concatenate two lists
>instead of open-coding it.
Will check this.

>you should really add the documentation and
>regression tests which you mentioned as a TODO.  And run the code
>through pgindent
I will also update the next version with documentation and regression tests
and run pgindent

Thank you,
Rahila Syed

On Fri, May 12, 2017 at 4:33 PM, Beena Emerson <memissemerson@gmail.com>
wrote:

>
>
> On Thu, May 11, 2017 at 7:37 PM, Rahila Syed <rahilasyed90@gmail.com>
> wrote:
>
>> Hello,
>>
>> Please find attached an updated patch with review comments and bugs
>> reported till date implemented.
>>
>
> Hello Rahila,
>
> Tested on "efa2c18 Doc fix: scale(numeric) returns integer, not numeric."
>
> (1) With the new patch, we allow new partitions when there is overlapping
> data with default partition. The entries in default are ignored when
> running queries satisfying the new partition.
>
> DROP TABLE list1;
> CREATE TABLE list1 (
>     a int,
>     b int
> ) PARTITION BY LIST (a);
> CREATE TABLE list1_1 (LIKE list1);
> ALTER TABLE  list1 ATTACH PARTITION list1_1 FOR VALUES IN (1);
> CREATE TABLE list1_def PARTITION OF list1 DEFAULT;
> INSERT INTO list1 SELECT generate_series(1,2),1;
> -- Partition overlapping with DEF
> CREATE TABLE list1_2 PARTITION OF list1 FOR VALUES IN (2);
> INSERT INTO list1 SELECT generate_series(2,3),2;
>
> postgres=# SELECT * FROM list1 ORDER BY a,b;
>  a | b
> ---+---
>  1 | 1
>  2 | 1
>  2 | 2
>  3 | 2
> (4 rows)
>
> postgres=# SELECT * FROM list1 WHERE a=2;
>  a | b
> ---+---
>  2 | 2
> (1 row)
>
> This ignores the a=2 entries in the DEFAULT.
>
> postgres=# SELECT * FROM list1_def;
>  a | b
> ---+---
>  2 | 1
>  3 | 2
> (2 rows)
>
>
> (2) I get the following warning:
>
> partition.c: In function ‘check_new_partition_bound’:
> partition.c:882:15: warning: ‘boundinfo’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]
>    && boundinfo->has_default)
>                ^
> preproc.y:3250.2-8: warning: type clash on default action: <str> != <>
>
>
>> >1.
>> >In following block, we can just do with def_index, and we do not need
>> found_def
>> >flag. We can check if def_index is -1 or not to decide if default
>> partition is
>> >present.
>> found_def is used to set boundinfo->has_default which is used at couple
>> of other places to check if default partition exists. The implementation
>> is similar
>> to has_null.
>>
>> >3.
>> >In following function isDefaultPartitionBound, first statement "return
>> false"
>> >is not needed.
>> It is needed to return false if the node is not DefElem.
>>
>> Todo:
>> Add regression tests
>> Documentation
>>
>> Thank you,
>> Rahila Syed
>>
>>
>>

Re: Adding support for Default partition in partitioning

Beena Emerson <memissemerson@gmail.com> — 2017-05-12T14:04:57Z

Hello,


On Fri, May 12, 2017 at 5:30 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:

> Hello,
>
> >(1) With the new patch, we allow new partitions when there is overlapping
> data with default partition. The entries in default are ignored when
> running queries satisfying the new partition.
> This was introduced in latest version. We are not allowing adding a
> partition when entries with same key value exist in default partition. So
> this scenario should not
> come in picture. Please find attached an updated patch which corrects this.
>

Thank you for the updated patch. However, now I cannot create a partition
after default.

CREATE TABLE list1 (
    a int,
    b int
) PARTITION BY LIST (a);

CREATE TABLE list1_1 (LIKE list1);
ALTER TABLE  list1 ATTACH PARTITION list1_1 FOR VALUES IN (1);
CREATE TABLE list1_def PARTITION OF list1 DEFAULT;
CREATE TABLE list1_5 PARTITION OF list1 FOR VALUES IN (3);

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.
!>


>
>
> >(2) I get the following warning:
>
> >partition.c: In function ‘check_new_partition_bound’:
> >partition.c:882:15: warning: ‘boundinfo’ may be used uninitialized in
> this function [-Wmaybe-uninitialized]
> >   && boundinfo->has_default)
>                ^
> >preproc.y:3250.2-8: warning: type clash on default action: <str> != <>
> I failed to notice this warning. I will look into it.
>
> >This is incredibly ugly.  I don't know exactly what should be done
> >about it, but I think PARTITION_DEFAULT is a bad idea and has got to
> >go.  Maybe add a separate isDefault flag to PartitionBoundSpec
> Will look at other ways to do it.
>
> >Doesn't it strike you as a bit strange that get_qual_for_default()
> >doesn't return a qual?  Functions should generally have names that
> >describe what they do.
> Will fix this.
>
> >There's an existing function that you can use to concatenate two lists
> >instead of open-coding it.
> Will check this.
>
> >you should really add the documentation and
> >regression tests which you mentioned as a TODO.  And run the code
> >through pgindent
> I will also update the next version with documentation and regression tests
> and run pgindent
>
> Thank you,
> Rahila Syed
>
> On Fri, May 12, 2017 at 4:33 PM, Beena Emerson <memissemerson@gmail.com>
> wrote:
>
>>
>>
>> On Thu, May 11, 2017 at 7:37 PM, Rahila Syed <rahilasyed90@gmail.com>
>> wrote:
>>
>>> Hello,
>>>
>>> Please find attached an updated patch with review comments and bugs
>>> reported till date implemented.
>>>
>>
>> Hello Rahila,
>>
>> Tested on "efa2c18 Doc fix: scale(numeric) returns integer, not numeric."
>>
>> (1) With the new patch, we allow new partitions when there is overlapping
>> data with default partition. The entries in default are ignored when
>> running queries satisfying the new partition.
>>
>> DROP TABLE list1;
>> CREATE TABLE list1 (
>>     a int,
>>     b int
>> ) PARTITION BY LIST (a);
>> CREATE TABLE list1_1 (LIKE list1);
>> ALTER TABLE  list1 ATTACH PARTITION list1_1 FOR VALUES IN (1);
>> CREATE TABLE list1_def PARTITION OF list1 DEFAULT;
>> INSERT INTO list1 SELECT generate_series(1,2),1;
>> -- Partition overlapping with DEF
>> CREATE TABLE list1_2 PARTITION OF list1 FOR VALUES IN (2);
>> INSERT INTO list1 SELECT generate_series(2,3),2;
>>
>> postgres=# SELECT * FROM list1 ORDER BY a,b;
>>  a | b
>> ---+---
>>  1 | 1
>>  2 | 1
>>  2 | 2
>>  3 | 2
>> (4 rows)
>>
>> postgres=# SELECT * FROM list1 WHERE a=2;
>>  a | b
>> ---+---
>>  2 | 2
>> (1 row)
>>
>> This ignores the a=2 entries in the DEFAULT.
>>
>> postgres=# SELECT * FROM list1_def;
>>  a | b
>> ---+---
>>  2 | 1
>>  3 | 2
>> (2 rows)
>>
>>
>> (2) I get the following warning:
>>
>> partition.c: In function ‘check_new_partition_bound’:
>> partition.c:882:15: warning: ‘boundinfo’ may be used uninitialized in
>> this function [-Wmaybe-uninitialized]
>>    && boundinfo->has_default)
>>                ^
>> preproc.y:3250.2-8: warning: type clash on default action: <str> != <>
>>
>>
>>> >1.
>>> >In following block, we can just do with def_index, and we do not need
>>> found_def
>>> >flag. We can check if def_index is -1 or not to decide if default
>>> partition is
>>> >present.
>>> found_def is used to set boundinfo->has_default which is used at couple
>>> of other places to check if default partition exists. The implementation
>>> is similar
>>> to has_null.
>>>
>>> >3.
>>> >In following function isDefaultPartitionBound, first statement "return
>>> false"
>>> >is not needed.
>>> It is needed to return false if the node is not DefElem.
>>>
>>> Todo:
>>> Add regression tests
>>> Documentation
>>>
>>> Thank you,
>>> Rahila Syed
>>>
>>>
>>>
>


-- 

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-16T12:57:50Z

On Fri, May 12, 2017 at 7:34 PM, Beena Emerson <memissemerson@gmail.com>
wrote:

>
> Thank you for the updated patch. However, now I cannot create a partition
> after default.
>
> CREATE TABLE list1 (
>     a int,
>     b int
> ) PARTITION BY LIST (a);
>
> CREATE TABLE list1_1 (LIKE list1);
> ALTER TABLE  list1 ATTACH PARTITION list1_1 FOR VALUES IN (1);
> CREATE TABLE list1_def PARTITION OF list1 DEFAULT;
> CREATE TABLE list1_5 PARTITION OF list1 FOR VALUES IN (3);
>
> 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.
> !>
>

Hi,

I have fixed the crash in attached patch.
Also the patch needed bit of adjustments due to recent commit.
I have re-based the patch on latest commit.

PFA.

Regards,
Jeevan Ladhe

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-05-16T15:31:34Z

On Tue, May 16, 2017 at 8:57 AM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> I have fixed the crash in attached patch.
> Also the patch needed bit of adjustments due to recent commit.
> I have re-based the patch on latest commit.

+    bool        has_default;        /* Is there a default partition?
Currently false
+                                 * for a range partitioned table */
+    int            default_index;        /* Index of the default list
partition. -1 for
+                                 * range partitioned tables */

Why do we need both has_default and default_index?  If default_index
== -1 means that there is no default, we don't also need a separate
bool to record the same thing, do we?

get_qual_for_default() still returns a list of things that are not
quals.  I think that this logic is all pretty poorly organized.  The
logic to create a partitioning constraint for a list partition should
be part of get_qual_for_list(), whether or not it is a default.  And
when we have range partitions, the logic to create a default range
partitioning constraint should be part of get_qual_for_range().  The
code the way it's organized today makes it look like there are three
kinds of partitions: list, range, and default.  But that's not right
at all.  There are two kinds: list and range.  And a list partition
might or might not be a default partition, and similarly for range.

+                    ereport(ERROR, (errcode(ERRCODE_CHECK_VIOLATION),
+                                    errmsg("DEFAULT partition cannot be used"
+                                           " without negator of operator  %s",
+                                           get_opname(operoid))));

I don't think ERRCODE_CHECK_VIOLATION is the right error code here,
and we have a policy against splitting message strings like this.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-05-17T08:58:31Z

On Tue, May 16, 2017 at 9:01 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, May 16, 2017 at 8:57 AM, Jeevan Ladhe
> <jeevan.ladhe@enterprisedb.com> wrote:
>> I have fixed the crash in attached patch.
>> Also the patch needed bit of adjustments due to recent commit.
>> I have re-based the patch on latest commit.
>
> +    bool        has_default;        /* Is there a default partition?
> Currently false
> +                                 * for a range partitioned table */
> +    int            default_index;        /* Index of the default list
> partition. -1 for
> +                                 * range partitioned tables */
>

We have has_null and null_index for list partitioning. There
null_index == -1 = has_null. May be Rahila and/or Jeevan just copied
that style. Probably we should change that as well?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: Adding support for Default partition in partitioning

Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-05-17T09:17:43Z

On 2017/05/17 17:58, Ashutosh Bapat wrote:
> On Tue, May 16, 2017 at 9:01 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Tue, May 16, 2017 at 8:57 AM, Jeevan Ladhe
>> <jeevan.ladhe@enterprisedb.com> wrote:
>>> I have fixed the crash in attached patch.
>>> Also the patch needed bit of adjustments due to recent commit.
>>> I have re-based the patch on latest commit.
>>
>> +    bool        has_default;        /* Is there a default partition?
>> Currently false
>> +                                 * for a range partitioned table */
>> +    int            default_index;        /* Index of the default list
>> partition. -1 for
>> +                                 * range partitioned tables */
>>
> 
> We have has_null and null_index for list partitioning. There
> null_index == -1 = has_null. May be Rahila and/or Jeevan just copied
> that style. Probably we should change that as well?

Probably a good idea.

Thanks,
Amit

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-17T09:29:31Z

On Wed, May 17, 2017 at 2:28 PM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:

> On Tue, May 16, 2017 at 9:01 PM, Robert Haas <robertmhaas@gmail.com>
> wrote:
> > On Tue, May 16, 2017 at 8:57 AM, Jeevan Ladhe
> > <jeevan.ladhe@enterprisedb.com> wrote:
> >> I have fixed the crash in attached patch.
> >> Also the patch needed bit of adjustments due to recent commit.
> >> I have re-based the patch on latest commit.
> >
> > +    bool        has_default;        /* Is there a default partition?
> > Currently false
> > +                                 * for a range partitioned table */
> > +    int            default_index;        /* Index of the default list
> > partition. -1 for
> > +                                 * range partitioned tables */
> >
>
> We have has_null and null_index for list partitioning. There
> null_index == -1 = has_null. May be Rahila and/or Jeevan just copied
> that style. Probably we should change that as well?
>
>
I agree with Ashutosh.
I had given similar comment on earlier version of patch[1]
<https://www.postgresql.org/message-id/CAOgcT0NUUQXWRXmeVKkYTDQvWoKLYRMiPbc89ua6i_gG8FPDmA%40mail.gmail.com>,
and  Rahila reverted
with above reasoning, hence did not change the logic she introduced.

Probably its a good idea to have a separate patch that removes has_null
logic,
in a separate thread.

[1]
https://www.postgresql.org/message-id/CAOgcT0NUUQXWRXmeVKkYTDQvWoKLYRMiPbc89ua6i_gG8FPDmA%40mail.gmail.com

Regards,
Jeevan Ladhe.

Re: Adding support for Default partition in partitioning

Beena Emerson <memissemerson@gmail.com> — 2017-05-22T02:01:47Z

Hello,

Patch for default range partition has been added. PFA the rebased v12 patch
for the same.
I have not removed the has_default variable yet.

Default range partition:
https://www.postgresql.org/message-id/CAOG9ApEYj34fWMcvBMBQ-YtqR9fTdXhdN82QEKG0SVZ6zeL1xg%40mail.gmail.com
-- 

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-25T06:40:22Z

Hi,

I started looking into Rahila's default_partition_v11.patch, and reworked on
few things as below:

- I tried to cover all the review comments posted on the thread. Do let
me know if something is missing.

- Got rid of the functions get_qual_for_default() and
generate_qual_for_defaultpart().
There is no need of collecting boundspecs of all the partitions in case of
list
partition, the list is available in boundinfo->ndatums, an expression for
default can be created from the information that is available in boundinfo.

- Got rid of variable has_default, and added a macro for it.

- Changed the logic of checking the overlapping of existing rows in default
partition. Earlier version of patch used to build new constraints for
default
partition table and then was checking if any of existing rows violate those
constraints. However, current version of patch just checks if any of the
rows in
default partition satisfy the new partition's constraint and fail if there
exists any.
This logic can also be used as it is for default partition in case of RANGE
partitioning.

- Simplified grammar rule.

- Got rid of PARTITION_DEFAULT since DEFAULT is not a different partition
strategy, the applicable logic is also revised:

- There are few other code adjustments like: indentation, commenting, code
simplification etc.

- Added regression tests.

TODO:
Documentation, I am working on it. Will updated the patch soon.

PFA.

Regards,
Jeevan

On Mon, May 22, 2017 at 7:31 AM, Beena Emerson <memissemerson@gmail.com>
wrote:

> Hello,
>
> Patch for default range partition has been added. PFA the rebased v12
> patch for the same.
> I have not removed the has_default variable yet.
>
> Default range partition: https://www.postgresql.org/message-id/
> CAOG9ApEYj34fWMcvBMBQ-YtqR9fTdXhdN82QEKG0SVZ6zeL1xg%40mail.gmail.com
> --
>
> Beena Emerson
>
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: Adding support for Default partition in partitioning

Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> — 2017-05-25T07:09:13Z

On Thu, May 25, 2017 at 12:10 PM, Jeevan Ladhe <
jeevan.ladhe@enterprisedb.com> wrote:

> PFA.
>

Hi

I have applied v13 patch, got a crash when trying to attach default temp
partition.

postgres=# CREATE TEMP TABLE temp_list_part (a int) PARTITION BY LIST (a);
CREATE TABLE
postgres=# CREATE TEMP TABLE temp_def_part (a int);
CREATE TABLE
postgres=# ALTER TABLE temp_list_part ATTACH PARTITION temp_def_part
DEFAULT;
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.
!>

Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-25T09:32:32Z

Hi Rajkumar,

postgres=# CREATE TEMP TABLE temp_list_part (a int) PARTITION BY LIST (a);
> CREATE TABLE
> postgres=# CREATE TEMP TABLE temp_def_part (a int);
> CREATE TABLE
> postgres=# ALTER TABLE temp_list_part ATTACH PARTITION temp_def_part
> DEFAULT;
> 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.
> !>
>

Thanks for reporting.
PFA patch that fixes above issue.

Regards,
Jeevan Ladhe

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-25T09:33:05Z

Forgot to attach the patch.
PFA.

On Thu, May 25, 2017 at 3:02 PM, Jeevan Ladhe <jeevan.ladhe@enterprisedb.com
> wrote:

> Hi Rajkumar,
>
> postgres=# CREATE TEMP TABLE temp_list_part (a int) PARTITION BY LIST (a);
>> CREATE TABLE
>> postgres=# CREATE TEMP TABLE temp_def_part (a int);
>> CREATE TABLE
>> postgres=# ALTER TABLE temp_list_part ATTACH PARTITION temp_def_part
>> DEFAULT;
>> 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.
>> !>
>>
>
> Thanks for reporting.
> PFA patch that fixes above issue.
>
> Regards,
> Jeevan Ladhe
>

Re: Adding support for Default partition in partitioning

Beena Emerson <memissemerson@gmail.com> — 2017-05-29T06:55:15Z

On Thu, May 25, 2017 at 3:03 PM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
>
> Forgot to attach the patch.
> PFA.
>
> On Thu, May 25, 2017 at 3:02 PM, Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> wrote:
>>
>> Hi Rajkumar,
>>
>>> postgres=# CREATE TEMP TABLE temp_list_part (a int) PARTITION BY LIST (a);
>>> CREATE TABLE
>>> postgres=# CREATE TEMP TABLE temp_def_part (a int);
>>> CREATE TABLE
>>> postgres=# ALTER TABLE temp_list_part ATTACH PARTITION temp_def_part DEFAULT;
>>> 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.
>>> !>
>>
>>
>> Thanks for reporting.
>> PFA patch that fixes above issue.
>>


The existing comment is not valid
            /*
             * A null partition key is only acceptable if null-accepting list
             * partition exists.
             */
as we allow NULL to be stored in default. It should be updated.

DROP TABLE list1;
CREATE TABLE list1 (    a int) PARTITION BY LIST (a);
CREATE TABLE list1_1 (LIKE list1);
ALTER TABLE  list1 ATTACH PARTITION list1_1 FOR VALUES IN (2);
CREATE TABLE list1_def PARTITION OF list1 DEFAULT;
INSERT INTO list1 VALUES (NULL);
SELECT * FROM list1_def;
 a
---

(1 row)


-- 

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-29T08:57:04Z

This patch needs a rebase on recent commits, and also a fix[1] that is
posted for get_qual_for_list().

I am working on both of these tasks. Will update the patch once I am done
with this.


Regards,

Jeevan Ladhe

On Mon, May 29, 2017 at 12:25 PM, Beena Emerson <memissemerson@gmail.com>
wrote:

> On Thu, May 25, 2017 at 3:03 PM, Jeevan Ladhe
> <jeevan.ladhe@enterprisedb.com> wrote:
> >
> > Forgot to attach the patch.
> > PFA.
> >
> > On Thu, May 25, 2017 at 3:02 PM, Jeevan Ladhe <
> jeevan.ladhe@enterprisedb.com> wrote:
> >>
> >> Hi Rajkumar,
> >>
> >>> postgres=# CREATE TEMP TABLE temp_list_part (a int) PARTITION BY LIST
> (a);
> >>> CREATE TABLE
> >>> postgres=# CREATE TEMP TABLE temp_def_part (a int);
> >>> CREATE TABLE
> >>> postgres=# ALTER TABLE temp_list_part ATTACH PARTITION temp_def_part
> DEFAULT;
> >>> 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.
> >>> !>
> >>
> >>
> >> Thanks for reporting.
> >> PFA patch that fixes above issue.
> >>
>
>
> The existing comment is not valid
>             /*
>              * A null partition key is only acceptable if null-accepting
> list
>              * partition exists.
>              */
> as we allow NULL to be stored in default. It should be updated.
>
> DROP TABLE list1;
> CREATE TABLE list1 (    a int) PARTITION BY LIST (a);
> CREATE TABLE list1_1 (LIKE list1);
> ALTER TABLE  list1 ATTACH PARTITION list1_1 FOR VALUES IN (2);
> CREATE TABLE list1_def PARTITION OF list1 DEFAULT;
> INSERT INTO list1 VALUES (NULL);
> SELECT * FROM list1_def;
>  a
> ---
>
> (1 row)
>
>
> --
>
> Beena Emerson
>
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-29T08:58:36Z

>
> The existing comment is not valid
>             /*
>              * A null partition key is only acceptable if null-accepting
> list
>              * partition exists.
>              */
> as we allow NULL to be stored in default. It should be updated.
>

Sure Beena, as stated earlier will update this on my next version of patch.


Regards,
Jeevan Ladhe

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-29T16:03:45Z

Hi,

I have rebased the patch on latest commit with few cosmetic changes.

The patch fix_listdatums_get_qual_for_list_v3.patch [1]
<http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg315490.html>
 needs to be applied
before applying this patch.

[1] http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg315490.html

Regards,
Jeevan Ladhe


On Mon, May 29, 2017 at 2:28 PM, Jeevan Ladhe <jeevan.ladhe@enterprisedb.com
> wrote:

>
>
>> The existing comment is not valid
>>             /*
>>              * A null partition key is only acceptable if null-accepting
>> list
>>              * partition exists.
>>              */
>> as we allow NULL to be stored in default. It should be updated.
>>
>
> Sure Beena, as stated earlier will update this on my next version of patch.
>
>
> Regards,
> Jeevan Ladhe
>

Re: Adding support for Default partition in partitioning

Beena Emerson <memissemerson@gmail.com> — 2017-05-30T05:29:45Z

On Mon, May 29, 2017 at 9:33 PM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> Hi,
>
> I have rebased the patch on latest commit with few cosmetic changes.
>
> The patch fix_listdatums_get_qual_for_list_v3.patch [1]  needs to be applied
> before applying this patch.
>
> [1] http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg315490.html
>


This needs a rebase again.

-- 

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-30T07:38:47Z

Hi,

I have rebased the patch on the latest commit.
PFA.

There exists one issue reported by Rajkumar[1] off-line as following, where
describing the default partition after deleting null partition, does not
show
updated constraints. I am working on fixing this issue.

create table t1 (c1 int) partition by list (c1);
create table t11 partition of t1 for values in (1,2);
create table t12 partition of t1 default;
create table t13 partition of t1 for values in (10,11);
create table t14 partition of t1 for values in (null);

postgres=# \d+ t12
                                    Table "public.t12"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
 c1     | integer |           |          |         | plain   |
 |
Partition of: t1 DEFAULT
Partition constraint: ((c1 IS NOT NULL) AND (c1 <> ALL (ARRAY[1, 2, 10,
11])))

postgres=# alter table t1 detach partition t14;
ALTER TABLE
postgres=# \d+ t12
                                    Table "public.t12"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
 c1     | integer |           |          |         | plain   |
 |
Partition of: t1 DEFAULT
Partition constraint: ((c1 IS NOT NULL) AND (c1 <> ALL (ARRAY[1, 2, 10,
11])))

postgres=# insert into t1 values(null);
INSERT 0 1

Note that the parent correctly allows the nulls to be inserted.

[1] rajkumar.raghuwanshi@enterprisedb.com

Regards,
Jeevan Ladhe

On Tue, May 30, 2017 at 10:59 AM, Beena Emerson <memissemerson@gmail.com>
wrote:

> On Mon, May 29, 2017 at 9:33 PM, Jeevan Ladhe
> <jeevan.ladhe@enterprisedb.com> wrote:
> > Hi,
> >
> > I have rebased the patch on latest commit with few cosmetic changes.
> >
> > The patch fix_listdatums_get_qual_for_list_v3.patch [1]  needs to be
> applied
> > before applying this patch.
> >
> > [1] http://www.mail-archive.com/pgsql-hackers@postgresql.org/
> msg315490.html
> >
>
>
> This needs a rebase again.
>
> --
>
> Beena Emerson
>
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-30T11:21:26Z

Hi,

I have fixed the issue related to default partition constraints not getting
updated
after detaching a partition.

PFA.

Regards,
Jeevan Ladhe

On Tue, May 30, 2017 at 1:08 PM, Jeevan Ladhe <jeevan.ladhe@enterprisedb.com
> wrote:

> Hi,
>
> I have rebased the patch on the latest commit.
> PFA.
>
> There exists one issue reported by Rajkumar[1] off-line as following, where
> describing the default partition after deleting null partition, does not
> show
> updated constraints. I am working on fixing this issue.
>
> create table t1 (c1 int) partition by list (c1);
> create table t11 partition of t1 for values in (1,2);
> create table t12 partition of t1 default;
> create table t13 partition of t1 for values in (10,11);
> create table t14 partition of t1 for values in (null);
>
> postgres=# \d+ t12
>                                     Table "public.t12"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
> --------+---------+-----------+----------+---------+--------
> -+--------------+-------------
>  c1     | integer |           |          |         | plain   |
>  |
> Partition of: t1 DEFAULT
> Partition constraint: ((c1 IS NOT NULL) AND (c1 <> ALL (ARRAY[1, 2, 10,
> 11])))
>
> postgres=# alter table t1 detach partition t14;
> ALTER TABLE
> postgres=# \d+ t12
>                                     Table "public.t12"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
> --------+---------+-----------+----------+---------+--------
> -+--------------+-------------
>  c1     | integer |           |          |         | plain   |
>  |
> Partition of: t1 DEFAULT
> Partition constraint: ((c1 IS NOT NULL) AND (c1 <> ALL (ARRAY[1, 2, 10,
> 11])))
>
> postgres=# insert into t1 values(null);
> INSERT 0 1
>
> Note that the parent correctly allows the nulls to be inserted.
>
> [1] rajkumar.raghuwanshi@enterprisedb.com
>
> Regards,
> Jeevan Ladhe
>
> On Tue, May 30, 2017 at 10:59 AM, Beena Emerson <memissemerson@gmail.com>
> wrote:
>
>> On Mon, May 29, 2017 at 9:33 PM, Jeevan Ladhe
>> <jeevan.ladhe@enterprisedb.com> wrote:
>> > Hi,
>> >
>> > I have rebased the patch on latest commit with few cosmetic changes.
>> >
>> > The patch fix_listdatums_get_qual_for_list_v3.patch [1]  needs to be
>> applied
>> > before applying this patch.
>> >
>> > [1] http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg
>> 315490.html
>> >
>>
>>
>> This needs a rebase again.
>>
>> --
>>
>> Beena Emerson
>>
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>

Re: Adding support for Default partition in partitioning

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-05-30T12:41:35Z

On Tue, May 30, 2017 at 1:08 PM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> Hi,
>
> I have rebased the patch on the latest commit.
> PFA.
>

Thanks for rebasing the patch. Here are some review comments.
+                /*
+                 * In case of default partition, just note the index, we do not
+                 * add this to non_null_values list.
+                 */
We may want to rephrase it like
"Note the index of the partition bound spec for the default partition. There's
no datum to add to the list of non-null datums for this partition."

                    /* Assign mapping index for default partition. */
"mapping index" should be "mapped index". May be we want to use "the" before
default partition everywhere, there's only one specific default partition.

                        Assert(default_index >= 0 &&
                               mapping[default_index] == -1);
Needs some explanation for asserting mapping[default_index] == -1. Since
default partition accepts any non-specified value, it should not get a mapped
index while assigning those for non-null datums.

+                     * Currently range partition do not have default partition
May be rephrased as "As of now, we do not support default range partition."

+     * ArrayExpr, which would return an negated expression for default
a negated instead of an negated.

+        cur_index = -1;
         /*
-         * A null partition key is only acceptable if null-accepting list
-         * partition exists.
+         * A null partition key is acceptable if null-accepting list partition
+         * or a default partition exists. Check if there exists a null
+         * accepting partition, else this will be handled later by default
+         * partition if it exists.
          */
-        cur_index = -1;
Why do we need to move assignment to cur_index before the comment.
The comment should probably change to "Handle NULL partition key here
if there's a
null-accepting list partition. Else it will routed to a default partition if
one exists."

+-- attaching default partition overlaps if a default partition already exists
+ERROR:  partition "part_def2" would overlap partition "part_def1"
Saying a default partition overlaps is misleading here. A default partition is
not exepected to overlap with anything. It's expected to "adjust" with the rest
of the partitions. It can "conflict" with another default partition. So the
right error message here is "a default partition "part_def1" already exists."

+CREATE TABLE part_def1 PARTITION OF list_parted DEFAULT;
+CREATE TABLE part_def2 (LIKE part_1 INCLUDING CONSTRAINTS);
+ALTER TABLE list_parted ATTACH PARTITION part_def2 DEFAULT;
May be you want to name part_def1 as def_part and part_def2 as fail_def_part to
be consistent with other names in the file. May be you want to test to
consecutive CREATE TABLE ... DEFAULT.

+ALTER TABLE list_parted2 ATTACH PARTITION part_3 FOR VALUES IN (11);
+ERROR:  new default partition constraint is violated by some row
+DETAIL:  Violating row contains (11, z).
The error message seems to be misleading. The default partition is not new. May
be we should say, "default partition contains rows that conflict with the
partition bounds of "part_3"". I think we should use a better word instead of
"conflict", but I am not able to find one right now.

+-- check that leaf partitons of default partition are scanned when
s/partitons/partitions/

-ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IN (5)), ALTER a
SET NOT NULL;
-ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5);
+ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IN (5, 55)), ALTER
a SET NOT NULL;
+ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5, 55);
Why do we want to change partition bounds of this one? The test is for children
of part_5 right?

+drop table part_default;
I think this is premature drop. Down the file there's a SELECT from
list_parted, which won't list the rows inserted to the default partition and we
will miss to check whether the tuples were routed to the right partition or
not.

+update list_part1 set a = 'c' where a = 'a';
+ERROR:  new row for relation "list_part1" violates partition constraint
+DETAIL:  Failing row contains (c, 1).
Why do we need this test here? It's not dealing with the default partition and
partition row movement is not in there. So the updated row may not move to the
default partition, even if it's there.

This isn't a complete review. I will continue to review this patch further.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: Adding support for Default partition in partitioning

Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-05-31T00:33:45Z

Hi Jeevan,

On 2017/05/30 16:38, Jeevan Ladhe wrote:
> I have rebased the patch on the latest commit.
> PFA.

Was looking at the patch and felt that the parse node representation of
default partition bound could be slightly different.  Can you explain the
motivation behind implementing it without adding a new member to the
PartitionBoundSpec struct?

I would suggest instead adding a bool named is_default and be done with
it.  It will help get rid of the public isDefaultPartitionBound() in the
proposed patch whose interface isn't quite clear and instead simply check
if (spec->is_default) in places where it's called by passing it (Node *)
linitial(spec->listdatums).

Further looking into the patch, I found a tiny problem in
check_default_allows_bound().  If the default partition that will be
scanned by it is a foreign table or a partitioned table with a foreign
leaf partition, you will get a failure like:

-- default partition is a foreign table
alter table p attach partition fp default;

-- adding a new partition will try to scan fp above
alter table p attach partition p12 for values in (1, 2);
ERROR:  could not open file "base/13158/16456": No such file or directory

I think the foreign tables should be ignored here to avoid the error.  The
fact that foreign default partition may contain data that satisfies the
new partition's constraint is something we cannot do much about.  Also,
see the note in ATTACH PARTITION description regarding foreign tables [1]
and the discussion at [2].

Thanks,
Amit

[1] https://www.postgresql.org/docs/devel/static/sql-altertable.html
[2]
https://www.postgresql.org/message-id/flat/8f89dcb2-bd15-d8dc-5f54-3e11dc6c9463%40lab.ntt.co.jp

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-05-31T01:28:35Z

Thanks Amit for your comments.

On 31-May-2017 6:03 AM, "Amit Langote" <Langote_Amit_f8@lab.ntt.co.jp>
wrote:

Hi Jeevan,

On 2017/05/30 16:38, Jeevan Ladhe wrote:
> I have rebased the patch on the latest commit.
> PFA.

Was looking at the patch and felt that the parse node representation of
default partition bound could be slightly different.  Can you explain the
motivation behind implementing it without adding a new member to the
PartitionBoundSpec struct?

I would suggest instead adding a bool named is_default and be done with
it.  It will help get rid of the public isDefaultPartitionBound() in the
proposed patch whose interface isn't quite clear and instead simply check
if (spec->is_default) in places where it's called by passing it (Node *)
linitial(spec->listdatums).

I thought of reusing the existing members of PartitionBoundSpec, but I
agree that having a bool could simplify the code. Will do the receptive
change.

Further looking into the patch, I found a tiny problem in
check_default_allows_bound().  If the default partition that will be
scanned by it is a foreign table or a partitioned table with a foreign
leaf partition, you will get a failure like:

-- default partition is a foreign table
alter table p attach partition fp default;

-- adding a new partition will try to scan fp above
alter table p attach partition p12 for values in (1, 2);
ERROR:  could not open file "base/13158/16456": No such file or directory

I think the foreign tables should be ignored here to avoid the error.  The
fact that foreign default partition may contain data that satisfies the
new partition's constraint is something we cannot do much about.  Also,
see the note in ATTACH PARTITION description regarding foreign tables [1]
and the discussion at [2].

Will look into this.

Regards,
Jeevan Ladhe

Re: Adding support for Default partition in partitioning

Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-05-31T02:43:20Z

On 2017/05/31 9:33, Amit Langote wrote:
> On 2017/05/30 16:38, Jeevan Ladhe wrote:
>> I have rebased the patch on the latest commit.
>> PFA.
> 
> Was looking at the patch

I tried creating default partition of a range-partitioned table and got
the following error:

ERROR:  invalid bound specification for a range partition

I thought it would give:

ERROR: creating default partition is not supported for range partitioned
tables

Which means transformPartitionBound() should perform this check more
carefully.  As I suggested in my previous email, if there were a
is_default field in the PartitionBoundSpec, then one could add the
following block of code at the beginning of transformPartitionBound:

  if (spec->is_default && spec->strategy != PARTITION_STRATEGY_LIST)
      ereport(ERROR,
              (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
               errmsg("creating default partition is not supported for %s
partitioned tables", get_partition_strategy_name(key->strategy))));

Some more comments on the patch:

+                         errmsg("new default partition constraint is
violated by some row"),

"new default partition constraint" may sound a bit confusing to users.
That we recompute the default partition's constraint and check the "new
constraint" against the rows it contains seems to me to be the description
of internal details.  How about:

ERROR: default partition contains rows that belong to partition being created

+char *ExecBuildSlotValueDescription(Oid reloid,
+                              TupleTableSlot *slot,
+                              TupleDesc tupdesc,
+                              Bitmapset *modifiedCols,
+                              int maxfieldlen);

It seems that you made the above public to use it in
check_default_allows_bound(), which while harmless, I'm not sure if
needed.  ATRewriteTable() in tablecmds.c, for example, emits the following
error messages:

errmsg("check constraint \"%s\" is violated by some row",

errmsg("partition constraint is violated by some row")));

but neither outputs the DETAIL part showing exactly what row.  I think
it's fine for check_default_allows_bound() not to show the row itself and
hence no need to make ExecBuildSlotValueDescription public.

In get_rule_expr():

                     case PARTITION_STRATEGY_LIST:
                         Assert(spec->listdatums != NIL);

+                        /*
+                         * If the boundspec is of Default partition, it does
+                         * not have list of datums, but has only one node to
+                         * indicate its a default partition.
+                         */
+                        if (isDefaultPartitionBound(
+                                        (Node *) linitial(spec->listdatums)))
+                        {
+                            appendStringInfoString(buf, "DEFAULT");
+                            break;
+                        }
+

How about adding this part before the switch (key->strategy)?  That way,
we won't have to come back and add this again when we add range default
partitions.

Gotta go; will provide more comments later.

Thanks,
Amit

Re: Adding support for Default partition in partitioning

Beena Emerson <memissemerson@gmail.com> — 2017-05-31T04:20:44Z

On Wed, May 31, 2017 at 8:13 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/05/31 9:33, Amit Langote wrote:
>
>
> In get_rule_expr():
>
>                      case PARTITION_STRATEGY_LIST:
>                          Assert(spec->listdatums != NIL);
>
> +                        /*
> +                         * If the boundspec is of Default partition, it does
> +                         * not have list of datums, but has only one node to
> +                         * indicate its a default partition.
> +                         */
> +                        if (isDefaultPartitionBound(
> +                                        (Node *) linitial(spec->listdatums)))
> +                        {
> +                            appendStringInfoString(buf, "DEFAULT");
> +                            break;
> +                        }
> +
>
> How about adding this part before the switch (key->strategy)?  That way,
> we won't have to come back and add this again when we add range default
> partitions.

I think it is best that we add a bool is_default to PartitionBoundSpec
and then have a general check for both list and range. Though
listdatums, upperdatums and lowerdatums are set to default for a
DEFAULt partition, it does not seem proper that we check listdatums
for range as well.




-- 

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-06-01T19:35:03Z

Hi,

I have addressed Ashutosh's and Amit's comments in the attached patch.

Please let me know if I have missed anything and any further comments.

PFA.

Regards,
Jeevan Ladhe

On Wed, May 31, 2017 at 9:50 AM, Beena Emerson <memissemerson@gmail.com>
wrote:

> On Wed, May 31, 2017 at 8:13 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> > On 2017/05/31 9:33, Amit Langote wrote:
> >
> >
> > In get_rule_expr():
> >
> >                      case PARTITION_STRATEGY_LIST:
> >                          Assert(spec->listdatums != NIL);
> >
> > +                        /*
> > +                         * If the boundspec is of Default partition, it
> does
> > +                         * not have list of datums, but has only one
> node to
> > +                         * indicate its a default partition.
> > +                         */
> > +                        if (isDefaultPartitionBound(
> > +                                        (Node *)
> linitial(spec->listdatums)))
> > +                        {
> > +                            appendStringInfoString(buf, "DEFAULT");
> > +                            break;
> > +                        }
> > +
> >
> > How about adding this part before the switch (key->strategy)?  That way,
> > we won't have to come back and add this again when we add range default
> > partitions.
>
> I think it is best that we add a bool is_default to PartitionBoundSpec
> and then have a general check for both list and range. Though
> listdatums, upperdatums and lowerdatums are set to default for a
> DEFAULt partition, it does not seem proper that we check listdatums
> for range as well.
>
>
>
>
> --
>
> Beena Emerson
>
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-06-02T20:41:35Z

On Thu, Jun 1, 2017 at 3:35 PM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> Please let me know if I have missed anything and any further comments.

+                     errmsg("a default partition \"%s\" already exists",

I suggest: partition \"%s\" conflicts with existing default partition \"%s\"

The point is that's more similar to the message you get when overlap
&& !spec->is_default.

+     * If the default partition exists, it's partition constraint will change

it's -> its

+                         errmsg("default partition contains row(s)
that would overlap with partition being created")));

It doesn't really sound right to talk about rows overlapping with a
partition.  Partitions can overlap with each other, but not rows.
Also, it's not really project style to use ambiguously plural forms
like "row(s)" in error messages.  Maybe something like:

new partition constraint for default partition \"%s\" would be
violated by some row

+/*
+ * InvalidateDefaultPartitionRelcache
+ *
+ * Given a parent oid, this function checks if there exists a default partition
+ * and invalidates it's relcache if it exists.
+ */
+void
+InvalidateDefaultPartitionRelcache(Oid parentOid)
+{
+    Relation parent = heap_open(parentOid, AccessShareLock);
+    Oid default_relid =
parent->rd_partdesc->oids[DEFAULT_PARTITION_INDEX(parent)];
+
+    if (partition_bound_has_default(parent->rd_partdesc->boundinfo))
+        CacheInvalidateRelcacheByRelid(default_relid);
+
+    heap_close(parent, AccessShareLock);
+}

It does not seem like a good idea to put the heap_open() call inside
this function.  One of the two callers already *has* the Relation, and
we definitely want to avoid pulling the Oid out of the Relation only
to reopen it to get the Relation back.  And I think
heap_drop_with_catalog could open the parent relation instead of
calling LockRelationOid().

If DETACH PARTITION and DROP PARTITION require this, why not ATTACH
PARTITION and CREATE TABLE .. PARTITION OF?

The indentation of the changes in gram.y doesn't appear to match the
nearby code.  I'd remove this comment:

+             * Currently this is supported only for LIST partition.

Since nothing here is dependent on this working only for LIST
partitions, and since this will probably change, I think it would be
more future-proof to leave this out, lest somebody forget to update it
later.

-                switch (spec->strategy)
+                if (spec->is_default && (strategy == PARTITION_STRATEGY_LIST ||
+                                         strategy == PARTITION_STRATEGY_RANGE))

Checking strategy here appears pointless.

This is not a full review, but I'm out of time for today.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-06-03T11:43:18Z

Here's some detailed review of the code.

@@ -1883,6 +1883,15 @@ heap_drop_with_catalog(Oid relid)
     if (OidIsValid(parentOid))
     {
         /*
+         * Default partition constraints are constructed run-time from the
+         * constraints of its siblings(basically by negating them), so any
+         * change in the siblings needs to rebuild the constraints of the
+         * default partition. So, invalidate the sibling default partition's
+         * relcache.
+         */
+        InvalidateDefaultPartitionRelcache(parentOid);
+
Do we need a lock on the default partition for doing this? A query might be
scanning the default partition directly and we will invalidate the relcache
underneath it. What if two partitions are being dropped simultaneously and
change default constraints simultaneously. Probably the lock on the parent
helps there, but need to check it. What if the default partition cache is
invalidated because partition gets added/dropped to the default partition
itself. If we need a lock on the default partition, we will need to
check the order in which we should be obtaining the locks so as to avoid
deadlocks. This also means that we have to test PREPARED statements involving
default partition. Any addition/deletion/attach/detach of other partition
should invalidate those cached statements.

+                        if (partition_bound_has_default(boundinfo))
+                        {
+                            overlap = true;
+                            with = boundinfo->default_index;
+                        }
You could possibly rewrite this as
overlap = partition_bound_has_default(boundinfo);
with = boundinfo->default_index;
that would save one indentation and a conditional jump.

+    if (partdesc->nparts > 0 && partition_bound_has_default(boundinfo))
+        check_default_allows_bound(parent, spec);
If the table has a default partition, nparts > 0, nparts > 0 check looks
redundant. The comments above should also explain that this check doesn't
trigger when a default partition is added since we don't expect an existing
default partition in such a case.

+ * Checks if there exists any row in the default partition that passes the
+ * check for constraints of new partition, if any reports an error.
grammar two conflicting ifs in the same statement. You may want to rephrase
this as "This function checks if there exists a row in the default
partition that fits in the new
partition and throws an error if it finds one."

+    if (new_spec->strategy != PARTITION_STRATEGY_LIST)
+        return;
This should probably be an Assert. When default range partition is supported
this function would silently return, meaning there is no row in the default
partition which fits the new partition. We don't want that behavior.

The code in check_default_allows_bound() to check whether the default partition
has any rows that would fit new partition looks quite similar to the code in
ATExecAttachPartition() checking whether all rows in the table being attached
as a partition fit the partition bounds. One thing that
check_default_allows_bound() misses is, if there's already a constraint on the
default partition refutes the partition constraint on the new partition, we can
skip the scan of the default partition since it can not have rows that would
fit the new partition. ATExecAttachPartition() has code to deal with a similar
case i.e. the table being attached has a constraint which implies the partition
constraint. There may be more cases which check_default_allows_bound() does not
handle but ATExecAttachPartition() handles. So, I am wondering whether it's
better to somehow take out the common code into a function and use it. We will
have to deal with a difference through. The first one would throw an error when
finding a row that satisfies partition constraints whereas the second one would
throw an error when it doesn't find such a row. But this difference can be
handled through a flag or by negating the constraint. This would also take care
of Amit Langote's complaint about foreign partitions. There's also another
difference that the ATExecAttachPartition() queues the table for scan and the
actual scan takes place in ATRewriteTable(), but there is not such queue while
creating a table as a partition. But we should check if we can reuse the code to
scan the heap for checking a constraint.

In case of ATTACH PARTITION, probably we should schedule scan of default
partition in the alter table's work queue like what ATExecAttachPartition() is
doing for the table being attached. That would fit in the way alter table
works.

 make_partition_op_expr(PartitionKey key, int keynum,
-                       uint16 strategy, Expr *arg1, Expr *arg2)
+                    uint16 strategy, Expr *arg1, Expr *arg2, bool is_default)
Indentation

+                if (is_default &&
+                    ((operoid = get_negator(operoid)) == InvalidOid))
+                    ereport(ERROR, (errcode(ERRCODE_RESTRICT_VIOLATION),
+                                    errmsg("DEFAULT partition cannot
be used without negator of operator  %s",
+                                           get_opname(operoid))));
+
If the existence of default partition depends upon the negator, shouldn't there
be a dependency between the default partition and the negator. At the time of
creating the default partition, we will try to constuct the partition
constraint for the default partition and if the negator doesn't exist that
time, it will throw an error. But in an unlikely event when the user drops the
negator, the partitioned table will not be usable at all, as every time it will
try to create the relcache, it will try to create default partition constraint
and will throw error because of missing negator. That's not a very good
scenario. Have you tried this case? Apart from that, while restoring a dump, if
the default partition gets restored before the negator is created, restore will
fail with this error.

     /* Generate the main expression, i.e., keyCol = ANY (arr) */
     opexpr = make_partition_op_expr(key, 0, BTEqualStrategyNumber,
-                                    keyCol, (Expr *) arr);
+                                    keyCol, (Expr *) arr, spec->is_default);
                 /* Build leftop = ANY (rightop) */
                 saopexpr = makeNode(ScalarArrayOpExpr);
The comments in both the places need correction, as for default partition the
expression will be keyCol <> ALL(arr).

+    /*
+     * In case of the default partition for list, the partition constraint
+     * is basically any value that is not equal to any of the values in
+     * boundinfo->datums array. So, construct a list of constants from
+     * boundinfo->datums to pass to function make_partition_op_expr via
+     * ArrayExpr, which would return a negated expression for the default
+     * partition.
+     */
This is misleading, since the actual constraint would also have NOT NULL or IS
NULL in there depending upon the existence of a NULL partition.
I would simply rephrase this as "For default list partition, collect lists for
all the partitions. The default partition constraint should check that the
partition key is equal to none of those."

+        ndatums = (pdesc->nparts > 0) ? boundinfo->ndatums : 0;
wouldn't ndatums be simply boundinfo->ndatums? When nparts = 0, ndatums will be
0.
+        int         ndatums = 0;
This assignment looks redundant then.

+        if (boundinfo && partition_bound_accepts_nulls(boundinfo))
You have not checked existence of boundinfo when extracting ndatums out of it
and just few lines below you check that. If the later check is required then we
will get a segfault while extracting ndatums.

+    if ((!list_has_null && !spec->is_default) ||
+        (list_has_null && spec->is_default))
Need a comment explaining what's going on here. The condition is no more a
simple condition.

-            result = -1;
-            *failed_at = parent;
-            *failed_slot = slot;
-            break;
+            if (partition_bound_has_default(partdesc->boundinfo))
+            {
+                result = parent->indexes[partdesc->boundinfo->default_index];
+
+                if (result >= 0)
+                    break;
+                else
+                    parent = pd[-result];
+            }
+            else
+            {
+                result = -1;
+                *failed_at = parent;
+                *failed_slot = slot;
+                break;
+            }
The code to handle result is duplicated here and few lines below. I think it
would be better to not duplicate it by having separate condition blocks to deal
with setting result and setting parent. Basically if (cur_index < 0) ... else
would set the result breaking when setting result = -1 explicitly. A follow-on
block would adjust the parent if result < 0 or break otherwise.

Both the places where DEFAULT_PARTITION_INDEX is used, its result is used to
fetch OID of the default partition. So, instead of having this macro, may be we
should have macro to fetch OID of default partition. But even there I don't see
much value in that. Further, the macro and code using that macro fetches
rd_partdesc directly from Relation. We have RelationGetPartitionDesc() for
that. Probably we should also add Asserts to check that every pointer in the
long pointer chain is Non-null.

InvalidateDefaultPartitionRelcache() is called in case of drop and detach.
Shouldn't the constraint change when we add or attach a new partition.
Shouldn't we invalidate the cache then as well? I am not able to find that
code in your patch.

     /*
+     * Default partition constraints are constructed run-time from the
+     * constraints of its siblings(basically by negating them), so any
+     * change in the siblings needs to rebuild the constraints of the
+     * default partition. So, invalidate the sibling default partition's
+     * relcache.
+     */
May be rephrase this as "The default partition constraints depend upon the
partition bounds of other partitions. Detaching a partition invalidates the
default partition constraints. Invalidate the default partition's relcache so
that the constraints are built anew and any plans dependent on those
constraints are invalidated as well."

+                     errmsg("default partition is supported only for
list partitioned table")));
for "a" list partitioned table.

+            /*
+             * A default partition, that can be partition of either LIST or
+             * RANGE partitioned table.
+             * Currently this is supported only for LIST partition.
+             */
Keep everything in single paragraph without line break.

                 }
+
         ;
unnecessary extra line.

+        /*
+         * The default partition bound does not have any datums to be
+         * transformed, return the new bound.
+         */
Probably not needed.

+                if (spec->is_default && (strategy == PARTITION_STRATEGY_LIST ||
+                                         strategy == PARTITION_STRATEGY_RANGE))
+                {
+                    appendStringInfoString(buf, "DEFAULT");
+                    break;
+                }
+
What happens if strategy is something other than RANGE or LIST. For that matter
why not just LIST? Possibly you could write this as
+                if (spec->is_default)
+                {
+                    Assert(strategy == PARTITION_STRATEGY_LIST);
+                    appendStringInfoString(buf, "DEFAULT");
+                    break;
+                }

@@ -2044,7 +2044,7 @@ psql_completion(const char *text, int start, int end)
         COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, "");
     /* Limited completion support for partition bound specification */
     else if (TailMatches3("ATTACH", "PARTITION", MatchAny))
-        COMPLETE_WITH_CONST("FOR VALUES");
+        COMPLETE_WITH_LIST2("FOR VALUES", "DEFAULT");
     else if (TailMatches2("FOR", "VALUES"))
         COMPLETE_WITH_LIST2("FROM (", "IN (");

@@ -2483,7 +2483,7 @@ psql_completion(const char *text, int start, int end)
         COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_partitioned_tables, "");
     /* Limited completion support for partition bound specification */
     else if (TailMatches3("PARTITION", "OF", MatchAny))
-        COMPLETE_WITH_CONST("FOR VALUES");
+        COMPLETE_WITH_LIST2("FOR VALUES", "DEFAULT");
Do we include psql tab completion in the main feature patch? I have not seen
this earlier. But appreciate taking care of these defails.

+char *ExecBuildSlotValueDescription(Oid reloid,
needs an "extern" declaration.

On Fri, Jun 2, 2017 at 1:05 AM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> Hi,
>
> I have addressed Ashutosh's and Amit's comments in the attached patch.
>
> Please let me know if I have missed anything and any further comments.
>
> PFA.
>
> Regards,
> Jeevan Ladhe
>
> On Wed, May 31, 2017 at 9:50 AM, Beena Emerson <memissemerson@gmail.com>
> wrote:
>>
>> On Wed, May 31, 2017 at 8:13 AM, Amit Langote
>> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> > On 2017/05/31 9:33, Amit Langote wrote:
>> >
>> >
>> > In get_rule_expr():
>> >
>> >                      case PARTITION_STRATEGY_LIST:
>> >                          Assert(spec->listdatums != NIL);
>> >
>> > +                        /*
>> > +                         * If the boundspec is of Default partition, it
>> > does
>> > +                         * not have list of datums, but has only one
>> > node to
>> > +                         * indicate its a default partition.
>> > +                         */
>> > +                        if (isDefaultPartitionBound(
>> > +                                        (Node *)
>> > linitial(spec->listdatums)))
>> > +                        {
>> > +                            appendStringInfoString(buf, "DEFAULT");
>> > +                            break;
>> > +                        }
>> > +
>> >
>> > How about adding this part before the switch (key->strategy)?  That way,
>> > we won't have to come back and add this again when we add range default
>> > partitions.
>>
>> I think it is best that we add a bool is_default to PartitionBoundSpec
>> and then have a general check for both list and range. Though
>> listdatums, upperdatums and lowerdatums are set to default for a
>> DEFAULt partition, it does not seem proper that we check listdatums
>> for range as well.
>>
>>
>>
>>
>> --
>>
>> Beena Emerson
>>
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>
>



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-06-04T13:00:28Z

Hi Robert,

Thanks for your comments:


> If DETACH PARTITION and DROP PARTITION require this, why not ATTACH
> PARTITION and CREATE TABLE .. PARTITION OF?
>
>
For CREATE and ATTACH parition the invalidation of default relation is taken
care by the following clean-up part in check_default_allows_bound():

+ ResetExprContext(econtext);
+ CHECK_FOR_INTERRUPTS();
+ }
+
+ CacheInvalidateRelcache(part_rel);
+ MemoryContextSwitchTo(oldCxt);

However, post your comment I carefully looked in the code I wrote here, and
I
see that this still explicitly needs cache invalidation in ATTACH and CREATE
command, because the above invalidation call will not happen in case the
default partition is further partitioned. Plus, I think the call to
CacheInvalidateRelcache() in check_default_allows_bound() can be completely
removed.

This code however will be rearranged, as I plan to address Ashutosh's one
of the
comment to write a function for common code of ATExecAttachPartition() and
check_default_allows_bound().

Regards,
Jeevan Ladhe

Re: Adding support for Default partition in partitioning

Beena Emerson <memissemerson@gmail.com> — 2017-06-04T18:36:30Z

Hello,

On Fri, Jun 2, 2017 at 1:05 AM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> Hi,
>
> I have addressed Ashutosh's and Amit's comments in the attached patch.
>
> Please let me know if I have missed anything and any further comments.
>
> PFA.
>
> Regards,
> Jeevan Ladhe
>

What is the reason the new patch does not mention of violating rows
when a new partition overlaps with default?
Is it because more than one row could be violating the condition?

-- 

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-06-04T18:44:20Z

> What is the reason the new patch does not mention of violating rows
> when a new partition overlaps with default?
> Is it because more than one row could be violating the condition?
>

This is because, for reporting the violating error, I had to function
ExecBuildSlotValueDescription() public. Per Amit's comment I have
removed this change and let the overlapping error without row contains.
I think this is analogus to other functions that are throwing violation
error
but are not local to execMain.c.

Regards,
Jeevan Ladhe

Re: Adding support for Default partition in partitioning

Beena Emerson <memissemerson@gmail.com> — 2017-06-04T18:46:31Z

On Mon, Jun 5, 2017 at 12:14 AM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
>
>
>>
>> What is the reason the new patch does not mention of violating rows
>> when a new partition overlaps with default?
>> Is it because more than one row could be violating the condition?
>
>
> This is because, for reporting the violating error, I had to function
> ExecBuildSlotValueDescription() public. Per Amit's comment I have
> removed this change and let the overlapping error without row contains.
> I think this is analogus to other functions that are throwing violation
> error
> but are not local to execMain.c.
>

ok thanks.


-- 

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Adding support for Default partition in partitioning

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-06-07T09:47:28Z

On Sat, Jun 3, 2017 at 2:11 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> +                         errmsg("default partition contains row(s)
> that would overlap with partition being created")));
>
> It doesn't really sound right to talk about rows overlapping with a
> partition.  Partitions can overlap with each other, but not rows.
> Also, it's not really project style to use ambiguously plural forms
> like "row(s)" in error messages.  Maybe something like:
>
> new partition constraint for default partition \"%s\" would be
> violated by some row
>

Partition constraint is implementation detail here. We enforce
partition bounds through constraints and we call such constraints as
partition constraints. But a user may not necessarily understand this
term or may interpret it different. Adding "new" adds to the confusion
as the default partition is not new. My suggestion in an earlier mail
was ""default partition contains rows that conflict with the partition
bounds of "part_xyz"", with a note that we should use a better word
than "conflict". So, Jeevan seems to have used overlap, which again is
not correct. How about "default partition contains row/s which would
fit the partition "part_xyz" being created or attached." with a hint
to move those rows to the new partition's table in case of attach. I
don't think hint would be so straight forward i.e. to create the table
with SELECT INTO and then ATTACH.

What do you think?

Also, the error code ERRCODE_CHECK_VIOLATION, which is an "integrity
constraint violation" code, seems misleading. We aren't violating any
integrity here. In fact I am not able to understand, how could adding
an object violate integrity constraint. The nearest errorcode seems to
be ERRCODE_INVALID_OBJECT_DEFINITION, which is also used for
partitions with overlapping bounds.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: Adding support for Default partition in partitioning

Robert Haas <robertmhaas@gmail.com> — 2017-06-08T16:56:10Z

On Wed, Jun 7, 2017 at 5:47 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Sat, Jun 3, 2017 at 2:11 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> +                         errmsg("default partition contains row(s)
>> that would overlap with partition being created")));
>>
>> It doesn't really sound right to talk about rows overlapping with a
>> partition.  Partitions can overlap with each other, but not rows.
>> Also, it's not really project style to use ambiguously plural forms
>> like "row(s)" in error messages.  Maybe something like:
>>
>> new partition constraint for default partition \"%s\" would be
>> violated by some row
>
> Partition constraint is implementation detail here. We enforce
> partition bounds through constraints and we call such constraints as
> partition constraints. But a user may not necessarily understand this
> term or may interpret it different. Adding "new" adds to the confusion
> as the default partition is not new.

I see your point.  We could say "updated partition constraint" instead
of "new partition constraint" to address that to some degree.

> My suggestion in an earlier mail
> was ""default partition contains rows that conflict with the partition
> bounds of "part_xyz"", with a note that we should use a better word
> than "conflict". So, Jeevan seems to have used overlap, which again is
> not correct. How about "default partition contains row/s which would
> fit the partition "part_xyz" being created or attached." with a hint
> to move those rows to the new partition's table in case of attach. I
> don't think hint would be so straight forward i.e. to create the table
> with SELECT INTO and then ATTACH.

The problem is that none of these actually sound very good.  Neither
conflict nor overlap nor fit actually express the underlying idea very
clearly, at least IMHO.  I'm not opposed to using some wording along
these lines if we can think of a clear way to word it, but I think my
wording is better than using some unclear word for this concept.  I
can't immediately think of a way to adjust your wording so that it
seems completely clear.

> Also, the error code ERRCODE_CHECK_VIOLATION, which is an "integrity
> constraint violation" code, seems misleading. We aren't violating any
> integrity here. In fact I am not able to understand, how could adding
> an object violate integrity constraint. The nearest errorcode seems to
> be ERRCODE_INVALID_OBJECT_DEFINITION, which is also used for
> partitions with overlapping bounds.

I think that calling a constraint failure a check violation is not too
much of a stretch, even if it's technically a partition constraint
rather than a CHECK constraint.  However, your proposal also seems
reasonable.  I'm happy to go with whatever most people like best.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company