Should new partitions inherit their tablespace from their parent?

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Should new partitions inherit their tablespace from their parent?

David Rowley-3
Dear Hackers,

I've just read the thread in [1] where there was a discussion on a bug
fix regarding index partitions not being created in the same
tablespace as the partitioned index was created.

One paragraph that stood out when reading the thread was:

On 8 November 2018 at 09:00, Robert Haas <[hidden email]> wrote:
> With regard to this patch, I think the new behavior is fine in and of
> itself, but I *do not* think it should have been back-patched and I
> *do not* think it should work one way for tables and another for
> indexes.

While I don't agree with that 100% as there's no option to specify
were an index partition gets automatically created when a new
partitioned table is ATTACHed, whereas with new partitioned tables you
can at least specify the TABLESPACE option.


Anyway, Robert mentioned that he does not think it should work one way
for partitioned tables and another for partitioned indexes.

Here's the current situation with partitioned tables:

create table listp (a int) partition by list(a) tablespace foo;
create table listp1 partition of listp for values in(1);
select relname,reltablespace from pg_class where relname like 'listp%';
 relname | reltablespace
---------+---------------
 listp   |             0
 listp1  |             0
(2 rows)

It does not seem very good that when I created the partitioned table
and asked for its tablespace to be "foo" that postgres ignored that.
However, a partitioned table has no storage, so you could entertain
claims that this is not a bug.

I do have experience with using partitioned tables in a production
environment. During my experience a series of events took place that I
don't think is unique to this one case.  I think we could make life
easier for this case. Here's the scenario:

1. Start a business
2. Record some time series data.
3. Your business grows faster than you thought.
4. You painfully partition your time-series table so you can easily
get rid of older data.
5. Your business grows even more and the single disk partition you use
for data on the database server is filling up quickly.
6. Panic.
7. Add more disks and be thankful you partitioned your table back in #4.

Now, we can, of course, manage all this today, but new partitions
defaulting to the default tablespace might seem a little surprising.
Imagine the situation of the DBA removing the old partitions, it's
pretty convenient if, once they've done that they can then also glance
at free diskspace and then perhaps decide to change the default
partition tablespace to the disk partition with the most free space so
that the nightly batch job which creates new partitions puts them in
the best place.  The DBA could set the default_tablespace GUC, but the
tablespaces for the partitioned table might be on slower storage due
to how large they become and they might not want all new tables to go
in there.

I think we can do better:

How about we record the tablespace option for the partitioned table in
reltablespace instead of saving it as 0.  Newly created partitions
which don't have a TABLESPACE mentioned in the CREATE TABLE command
should be created in their direct parent partitioned tables
tablespace.

The above idea was mentioned in [2] and many people seemed to like it.
It was never implemented which likely lead to [1], which appears to
not be a great situation.

I've no patch, but I'm willing to go and write one if the general
consensus is that we want it to work this way.

[1] https://www.postgresql.org/message-id/flat/20181102003138.uxpaca6qfxzskepi%40alvherre.pgsql
[2] https://www.postgresql.org/message-id/flat/CAKJS1f9PXYcT%2Bj%3DoyL-Lquz%3DScNwpRtmD7u9svLASUygBdbN8w%40mail.gmail.com#089ce41fe9a33c340f7433e5f0018912

--
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Should new partitions inherit their tablespace from their parent?

Michael Paquier-2
On Thu, Nov 08, 2018 at 12:50:40PM +1300, David Rowley wrote:
> How about we record the tablespace option for the partitioned table in
> reltablespace instead of saving it as 0.  Newly created partitions
> which don't have a TABLESPACE mentioned in the CREATE TABLE command
> should be created in their direct parent partitioned tables
> tablespace.

I have seen enough complains on the mailing lists regarding the way
tablespaces are handled for partitioned tables and their partitions that
it looks like a very good idea to make the tablespace being inherited
automatically, by setting up reltablespace to a non-zero value even if
a partitioned table has no physical presence.  Of course not on v11 or
older releases, just on HEAD.  It is no good to have partitioned indexes
and partitioned tables being handling inconsistently for such things.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Should new partitions inherit their tablespace from their parent?

Robert Haas
On Wed, Nov 7, 2018 at 9:43 PM Michael Paquier <[hidden email]> wrote:

> On Thu, Nov 08, 2018 at 12:50:40PM +1300, David Rowley wrote:
> > How about we record the tablespace option for the partitioned table in
> > reltablespace instead of saving it as 0.  Newly created partitions
> > which don't have a TABLESPACE mentioned in the CREATE TABLE command
> > should be created in their direct parent partitioned tables
> > tablespace.
>
> I have seen enough complains on the mailing lists regarding the way
> tablespaces are handled for partitioned tables and their partitions that
> it looks like a very good idea to make the tablespace being inherited
> automatically, by setting up reltablespace to a non-zero value even if
> a partitioned table has no physical presence.  Of course not on v11 or
> older releases, just on HEAD.  It is no good to have partitioned indexes
> and partitioned tables being handling inconsistently for such things.

+1.

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

Reply | Threaded
Open this post in threaded view
|

Re: Should new partitions inherit their tablespace from their parent?

David Rowley-3
On Fri, 9 Nov 2018 at 06:58, Robert Haas <[hidden email]> wrote:

> On Wed, Nov 7, 2018 at 9:43 PM Michael Paquier <[hidden email]> wrote:
> > On Thu, Nov 08, 2018 at 12:50:40PM +1300, David Rowley wrote:
> > > How about we record the tablespace option for the partitioned table in
> > > reltablespace instead of saving it as 0.  Newly created partitions
> > > which don't have a TABLESPACE mentioned in the CREATE TABLE command
> > > should be created in their direct parent partitioned tables
> > > tablespace.
> >
> > I have seen enough complains on the mailing lists regarding the way
> > tablespaces are handled for partitioned tables and their partitions that
> > it looks like a very good idea to make the tablespace being inherited
> > automatically, by setting up reltablespace to a non-zero value even if
> > a partitioned table has no physical presence.  Of course not on v11 or
> > older releases, just on HEAD.  It is no good to have partitioned indexes
> > and partitioned tables being handling inconsistently for such things.
>
> +1.
Here's a patch for that.  Parking here until January's commitfest.

--
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

v1-0001-Allow-newly-created-partitions-to-inherit-their-p.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Should new partitions inherit their tablespace from their parent?

David Rowley-3
On Fri, 23 Nov 2018 at 17:03, David Rowley <[hidden email]> wrote:
> Here's a patch for that.  Parking here until January's commitfest.

And another, now rebased atop of de38ce1b8 (which I probably should
have waited for).

--
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

v2-0001-Allow-newly-created-partitions-to-inherit-their-p.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Should new partitions inherit their tablespace from their parent?

Michael Paquier-2
On Sat, Nov 24, 2018 at 12:32:36PM +1300, David Rowley wrote:
> On Fri, 23 Nov 2018 at 17:03, David Rowley <[hidden email]> wrote:
> > Here's a patch for that.  Parking here until January's commitfest.
>
> And another, now rebased atop of de38ce1b8 (which I probably should
> have waited for).

You have just gained a reviewer.  The current inconsistent state is
rather bad I think, so I'll try to look at that before January.  Now
there are other things waiting in the queue...
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Should new partitions inherit their tablespace from their parent?

Michael Paquier-2
On Sat, Nov 24, 2018 at 10:18:17AM +0900, Michael Paquier wrote:
> On Sat, Nov 24, 2018 at 12:32:36PM +1300, David Rowley wrote:
>> On Fri, 23 Nov 2018 at 17:03, David Rowley <[hidden email]> wrote:
>> > Here's a patch for that.  Parking here until January's commitfest.
>>
>> And another, now rebased atop of de38ce1b8 (which I probably should
>> have waited for).

Thanks for the patch, David.  I can see that this patch makes the code
more consistent for partitioned tables and partitioned indexes when it
comes to tablespace handling, which is a very good thing.

-ATExecPartedIdxSetTableSpace(Relation rel, Oid newTableSpace)
+ATExecSetTableSpaceNoStorage(Relation rel, Oid newTableSpace)
NoStorage looks strange as routine name for this case.  Would something
like ATExecPartedRelSetTableSpace be more adapted perhaps?

+   else if (stmt->partbound)
+   {
+       RangeVar   *parent;
+       Relation    parentrel;
+
+       /*
+        * For partitions, when no other tablespace is specified, we default
+        * the tablespace to the parent partitioned table's.
+        */
Okay, so the direct parent is what counts, and not the top-most parent.
Could you add a test with multiple level of partitioned tables, like:
- parent is in tablespace 1.
- child is created in tablespace 2.
- grandchild is created, which should inherit tablespace 2 from the
child, but not tablespace 1 from the parent.  In the existing example,
as one partition is used to test the top-most parent and another for the
new default, it looks cleaner to create a third partition which would be
itself a partitioned table.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Should new partitions inherit their tablespace from their parent?

David Rowley-3
Thank you for looking at this.

On Fri, 7 Dec 2018 at 20:15, Michael Paquier <[hidden email]> wrote:
> -ATExecPartedIdxSetTableSpace(Relation rel, Oid newTableSpace)
> +ATExecSetTableSpaceNoStorage(Relation rel, Oid newTableSpace)
> NoStorage looks strange as routine name for this case.  Would something
> like ATExecPartedRelSetTableSpace be more adapted perhaps?

I'd say it's better to name the function in the more general purpose
way.  Perhaps we'll get some relkind in the future that's not a
partitioned table or index that might need the reltablespace set. When
that happens, if we name the function the way you're proposing, then
we might end up with some other function which does the same thing, or
the patch adding the new relkind would have to rename the function to
something more like how I have it.  To save from either of those
having to happen, would it not be better to give it the generic name
now?

Instead of renaming the function, I've altered the Assert() to allow
all relkinds which don't have storage and also updated the comment to
remove the mention of partitioned tables and indexes.

> +   else if (stmt->partbound)
> +   {
> +       RangeVar   *parent;
> +       Relation    parentrel;
> +
> +       /*
> +        * For partitions, when no other tablespace is specified, we default
> +        * the tablespace to the parent partitioned table's.
> +        */
> Okay, so the direct parent is what counts, and not the top-most parent.
> Could you add a test with multiple level of partitioned tables, like:
> - parent is in tablespace 1.
> - child is created in tablespace 2.
> - grandchild is created, which should inherit tablespace 2 from the
> child, but not tablespace 1 from the parent.  In the existing example,
> as one partition is used to test the top-most parent and another for the
> new default, it looks cleaner to create a third partition which would be
> itself a partitioned table.
Sounds good. I've modified the existing test just to do it that way. I
don't think there's a need to have multiple tests for that.

I've attached an updated patch.

--
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

v2-0001-Allow-newly-created-partitions-to-inherit-their-p.patch (12K) Download Attachment