Thread: [HACKERS] Adding support for Default partition in partitioning

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

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

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

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

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

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

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

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
thatresponsibility 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
constraintyet to say that it  no longer contains key=3 rows (constraint that the planner consumes),  so it will
continueto 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
thenew 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
longerbe 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

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

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

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

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

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
twopartitions can contain
 
the same partition key.

Thanks,
Amit

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.

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.

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

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;

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

[...] CREATE TABLE p1 PARTITION OF test FOR VALUES IN  (DEFAULT) PARTITION BY LIST(b); [...]

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

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

On Tue, May 9, 2017 at 9:26 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>>Hi Rahila,
>
>>I am not able add a new partition if default partition is further
>> partitioned
>>with default partition.
>
>>Consider example below:
>
>>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=# CREATE TABLE test_pd_pd PARTITION OF test_pd DEFAULT;
>>CREATE TABLE
>>postgres=# INSERT INTO test VALUES (20, 24, 12);
>>INSERT 0 1
>>postgres=# CREATE TABLE test_p2 PARTITION OF test FOR VALUES IN(15);
> ERROR:  could not open file "base/12335/16420": No such file or directory
>
> Regarding fix for this I think we need to prohibit this case. That is
> prohibit creation
> of new partition after a default partition which is further partitioned.
> Currently before adding a new partition after default partition all the rows
> of default
> partition are scanned and if a row which matches the new partitions
> constraint exists
> the new partition is not added.
>
> If we allow this for default partition which is partitioned further, we will
> have to scan
> all the partitions of default partition for matching rows which can slow
> down execution.

I think this case should be allowed and I don't think it should
require scanning all the partitions of the default partition.  This is
no different than any other case where multiple levels of partitioning
are used.  First, you route the tuple at the root level; then, you
route it at the next level; and so on.  It shouldn't matter whether
the routing at the top level is to that level's default partition or
not.

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

On 2017/05/10 2:09, Robert Haas wrote:
> On Tue, May 9, 2017 at 9:26 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>>> Hi Rahila,
>>
>>> I am not able add a new partition if default partition is further
>>> partitioned
>>> with default partition.
>>
>>> Consider example below:
>>
>>> 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=# CREATE TABLE test_pd_pd PARTITION OF test_pd DEFAULT;
>>> CREATE TABLE
>>> postgres=# INSERT INTO test VALUES (20, 24, 12);
>>> INSERT 0 1
>>> postgres=# CREATE TABLE test_p2 PARTITION OF test FOR VALUES IN(15);
>> ERROR:  could not open file "base/12335/16420": No such file or directory
>>
>> Regarding fix for this I think we need to prohibit this case. That is
>> prohibit creation
>> of new partition after a default partition which is further partitioned.
>> Currently before adding a new partition after default partition all the rows
>> of default
>> partition are scanned and if a row which matches the new partitions
>> constraint exists
>> the new partition is not added.
>>
>> If we allow this for default partition which is partitioned further, we will
>> have to scan
>> all the partitions of default partition for matching rows which can slow
>> down execution.
> 
> I think this case should be allowed

+1

> and I don't think it should
> require scanning all the partitions of the default partition.  This is
> no different than any other case where multiple levels of partitioning
> are used.  First, you route the tuple at the root level; then, you
> route it at the next level; and so on.  It shouldn't matter whether
> the routing at the top level is to that level's default partition or
> not.

It seems that adding a new partition at the same level as the default
partition will require scanning it or its (leaf) partitions if
partitioned.  Consider that p1, pd are partitions of a list-partitioned
table p accepting 1 and everything else, respectively, and pd is further
partitioned.  When adding p2 of p for 2, we need to scan the partitions of
pd to check if there are any (2, ...) rows.

As for fixing the reported issue whereby the partitioned default
partition's non-existent file is being accessed, it would help to take a
look at the code in ATExecAttachPartition() starting at the following:
   /*    * Set up to have the table be scanned to validate the partition    * constraint (see partConstraint above).
Ifit's a partitioned table, we    * instead schedule its leaf partitions to be scanned.    */   if (!skip_validate)
{

Thanks,
Amit

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

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.

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

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

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

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.

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.

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