Adding support for Default partition in partitioning
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.
/messages/by-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
Attachments:
default_list_partition_v1.patchapplication/x-download; name=default_list_partition_v1.patchDownload+92-39
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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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:
Show quoted text
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-hackersI'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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
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:
Show quoted text
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
Attachments:
default_partition_v2.patchapplication/x-download; name=default_partition_v2.patchDownload+98-38
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 SyedOn 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
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:
Show quoted text
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 errorsApart 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 SyedOn 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 bea
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
Attachments:
default_partition_v3.patchapplication/x-download; name=default_partition_v3.patchDownload+189-50
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 SyedOn 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 errorsApart 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 SyedOn 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 tobe 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
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:
Show quoted text
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 TABLEpostgres=# 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 SyedOn 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 errorsApart 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 SyedOn 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_dumpand
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
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:
Show quoted text
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 TABLEpostgres=# 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 SyedOn 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 errorsApart 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 SyedOn 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_dumpand
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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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:
Show quoted text
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
Attachments:
default_partition_v4.patchapplication/x-download; name=default_partition_v4.patchDownload+345-50
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
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 issueHowever, 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 issueHowever, 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_partitionedFOR
VALUES IN (DEFAULT);
CREATE TABLE
Time: 3.492 ms
keith@keith=# CREATE TABLE part_1 PARTITION OF list_partitioned FORVALUES
IN (4,5);
ERROR: unrecognized node type: 216It 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 rows2. 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=
7526e10224f0792201e99631567bbe44492bbde4However, thinking through this, I'm not sure I can see a solution without
the global index support. If this restriction is removed, there's stillan
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
