Обсуждение: Should new partitions inherit their tablespace from their parent?

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 <robertmhaas@gmail.com> 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

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

On Wed, Nov 7, 2018 at 9:43 PM Michael Paquier <michael@paquier.xyz> 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

On Fri, 9 Nov 2018 at 06:58, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Nov 7, 2018 at 9:43 PM Michael Paquier <michael@paquier.xyz> 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

On Fri, 23 Nov 2018 at 17:03, David Rowley <david.rowley@2ndquadrant.com> 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

On Sat, Nov 24, 2018 at 12:32:36PM +1300, David Rowley wrote:
> On Fri, 23 Nov 2018 at 17:03, David Rowley <david.rowley@2ndquadrant.com> 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

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 <david.rowley@2ndquadrant.com> 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

Thank you for looking at this.

On Fri, 7 Dec 2018 at 20:15, Michael Paquier <michael@paquier.xyz> 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

On Mon, Dec 10, 2018 at 02:05:29AM +1300, David Rowley wrote:
> On Fri, 7 Dec 2018 at 20:15, Michael Paquier <michael@paquier.xyz> 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.

-       Assert(rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX);
+       Assert(rel->rd_rel->relkind == RELKIND_VIEW ||
+                  rel->rd_rel->relkind == RELKIND_COMPOSITE_TYPE ||
+                  rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE ||
+                  rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
+                  rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX);

Alvaro has begun a thread about that recently, so it seems to me that
this assertion could become invalid (per se my comments down-thread):
https://www.postgresql.org/message-id/20181206215552.fm2ypuxq6nhpwjuc@alvherre.pgsql

Of course, it depends on the conclusion of this other thread.  It will
perhaps make sense at some point to review on which relkinds this
function can work on, but I would recommend to review that behavior when
it actually makes sense and keep the current interface simple.  So your
first version looked fine to me on those grounds.  Another important
thing which would help with a restricted assertion is that if this code
path begins to be taken for other relkinds then the developer who
implements a new feature depending on it will need to look at this code
and consider the use-case behind it, instead of assuming that it will
hopefully just work.

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

Thanks for the new version.  The new test case looks fine to me.
--
Michael

On Mon, 10 Dec 2018 at 15:22, Michael Paquier <michael@paquier.xyz> wrote:
> -       Assert(rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX);
> +       Assert(rel->rd_rel->relkind == RELKIND_VIEW ||
> +                  rel->rd_rel->relkind == RELKIND_COMPOSITE_TYPE ||
> +                  rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE ||
> +                  rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
> +                  rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX);
>
> Alvaro has begun a thread about that recently, so it seems to me that
> this assertion could become invalid (per se my comments down-thread):
> https://www.postgresql.org/message-id/20181206215552.fm2ypuxq6nhpwjuc@alvherre.pgsql
>
> Of course, it depends on the conclusion of this other thread.  It will
> perhaps make sense at some point to review on which relkinds this
> function can work on, but I would recommend to review that behavior when
> it actually makes sense and keep the current interface simple.  So your
> first version looked fine to me on those grounds.  Another important
> thing which would help with a restricted assertion is that if this code
> path begins to be taken for other relkinds then the developer who
> implements a new feature depending on it will need to look at this code
> and consider the use-case behind it, instead of assuming that it will
> hopefully just work.

Likely it would be nice if we had a macro to determine if the relkind
should have storage or not, and then just Assert() we get one of those
into the function.

For now, I've just changed the Assert to only pass when we get a
partitioned table or partitioned index. I've left a comment to mention
that other types may need to be added later. Assert failures should
remind us to check that any newly added callers work correctly.

Updated patch attached.

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

On Mon, Dec 10, 2018 at 11:37:16PM +1300, David Rowley wrote:
> Likely it would be nice if we had a macro to determine if the relkind
> should have storage or not, and then just Assert() we get one of those
> into the function.

The other thread can deal about that matter I guess.

> For now, I've just changed the Assert to only pass when we get a
> partitioned table or partitioned index. I've left a comment to mention
> that other types may need to be added later. Assert failures should
> remind us to check that any newly added callers work correctly.
>
> Updated patch attached.

Cool, thanks for updating the assertion.  At quick glance the patch
seems fine to me.  Let's keep ATExecSetTableSpaceNoStorage as routine
name as you suggest then.
--
Michael

On Mon, Dec 10, 2018 at 10:56:47PM +0900, Michael Paquier wrote:
> Cool, thanks for updating the assertion.  At quick glance the patch
> seems fine to me.  Let's keep ATExecSetTableSpaceNoStorage as routine
> name as you suggest then.

+       /*
+        * For partitions, when no other tablespace is specified, we default
+        * the tablespace to the parent partitioned table's.
+        */
+       Assert(list_length(stmt->inhRelations) == 1);
+
+       parent = linitial(stmt->inhRelations);
+
+       parentrel = heap_openrv(parent, AccessExclusiveLock);
So, in order to determine which tablespace should be used here, an
exclusive lock is taken on the parent because its partition descriptor
gets updated by the addition of the new partition.  This process is
actually done already in MergeAttributes() as well, but it won't get
triggered if a tablespace is defined directly in the CREATE TABLE
statement.  I think that we should add a comment to explain the
dependency between both, as well as why an exclusive lock is needed, so
something among those lines perhaps?  Here is an idea:
+       /*
+        * Grab an exclusive lock on the parent because its partition
+        * descriptor will be changed by the addition of the new partition.
+        * The same lock level is taken as when merging attributes below
+        * in MergeAttributes() to protect from lock upgrade deadlocks.
+        */

The position where the tablespace is chosen is definitely the good one.

What do you think?
--
Michael

On Tue, 11 Dec 2018 at 15:43, Michael Paquier <michael@paquier.xyz> wrote:
> +       parentrel = heap_openrv(parent, AccessExclusiveLock);
> So, in order to determine which tablespace should be used here, an
> exclusive lock is taken on the parent because its partition descriptor
> gets updated by the addition of the new partition.  This process is
> actually done already in MergeAttributes() as well, but it won't get
> triggered if a tablespace is defined directly in the CREATE TABLE
> statement.  I think that we should add a comment to explain the
> dependency between both, as well as why an exclusive lock is needed, so
> something among those lines perhaps?  Here is an idea:
> +       /*
> +        * Grab an exclusive lock on the parent because its partition
> +        * descriptor will be changed by the addition of the new partition.
> +        * The same lock level is taken as when merging attributes below
> +        * in MergeAttributes() to protect from lock upgrade deadlocks.
> +        */
>
> The position where the tablespace is chosen is definitely the good one.
>
> What do you think?

I think a comment in that location is a good idea. There's work being
done to reduce the lock level required for attaching a partition so a
comment here will help show that it's okay to reduce the lock level
for fetching the tablespace too.

I've attached an updated patch that includes the new comment. I didn't
use your proposed words though.

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

I didn't like this, so I rewrote it a little bit.

First, I changed the Assert() to use the macro for relations with
storage that I just posted in the other thread that Michael mentioned.

I then noticed that we're doing a heap_openrv() in the parent relation
and closing it before MergeAttribute() does the same thing all over
again; also MergeAttribute has the silly array-of-OIDs return value for
the parents so that DefineRelation can handle it again later.  Rube
Goldberg says hi.  I changed this so that *before* doing anything with
the parent list, we transform it to a list of OIDs, and lock them; so
MergeAttributes now receives the list of OIDs of parents rather than
RangeVars.  We can also move the important comment (about lock level of
parent rels) buried in the bowels of MergeAttribute to the place where
it belongs in DefineRelation; and we no longer have to mess with
transforming names to OIDs multiple times.

Proposed patch attached.

I'll self-review this again tomorrow, 'cause I now have to run.

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

On Sun, Dec 16, 2018 at 07:07:35PM -0300, Alvaro Herrera wrote:
> I'll self-review this again tomorrow, 'cause I now have to run.

> -        if (!is_partition)
> -            relation = heap_openrv(parent, ShareUpdateExclusiveLock);
> -        else
> -            relation = heap_openrv(parent, AccessExclusiveLock);
> +        /* caller already got lock */
> +        relation = heap_open(parent, NoLock);

Okay, I think that you should add an assertion on
CheckRelationLockedByMe() as MergeAttributes()'s only caller is
DefineRelation().  Better safe than sorry later.
--
Michael

On Mon, 17 Dec 2018 at 12:59, Michael Paquier <michael@paquier.xyz> wrote:
> Okay, I think that you should add an assertion on
> CheckRelationLockedByMe() as MergeAttributes()'s only caller is
> DefineRelation().  Better safe than sorry later.

Would that not just double up the work that's already done in relation_open()?

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

On Mon, 17 Dec 2018 at 11:07, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> First, I changed the Assert() to use the macro for relations with
> storage that I just posted in the other thread that Michael mentioned.
>
> I then noticed that we're doing a heap_openrv() in the parent relation
> and closing it before MergeAttribute() does the same thing all over
> again; also MergeAttribute has the silly array-of-OIDs return value for
> the parents so that DefineRelation can handle it again later.  Rube
> Goldberg says hi.  I changed this so that *before* doing anything with
> the parent list, we transform it to a list of OIDs, and lock them; so
> MergeAttributes now receives the list of OIDs of parents rather than
> RangeVars.  We can also move the important comment (about lock level of
> parent rels) buried in the bowels of MergeAttribute to the place where
> it belongs in DefineRelation; and we no longer have to mess with
> transforming names to OIDs multiple times.
>
> Proposed patch attached.

I like this idea. Seems much better than resolving the relation name twice.

I've read over the patch and the only two things that I noted were:

1. Shouldn't you be using the RangeVarGetRelid() macro instead of
calling RangeVarGetRelidExtended()?
2. In MergeAttributes(), the parentOids list still exists and is
populated.  This is now only used to determine if the "supers" list
contains any duplicate Oids. Maybe it's better to rename that list to
something like "seenOids" to avoid any confusion with the "supers"
list. Or maybe it's worth thinking of a better way to detect duplicate
items in the "supers" list.

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

On 2018-Dec-18, David Rowley wrote:

> 1. Shouldn't you be using the RangeVarGetRelid() macro instead of
> calling RangeVarGetRelidExtended()?

This should have been obvious but I didn't notice.

> 2. In MergeAttributes(), the parentOids list still exists and is
> populated.  This is now only used to determine if the "supers" list
> contains any duplicate Oids. Maybe it's better to rename that list to
> something like "seenOids" to avoid any confusion with the "supers"
> list. Or maybe it's worth thinking of a better way to detect duplicate
> items in the "supers" list.

Good catch.

What I did was move the duplicate detection to the loop doing
RangeVarGetRelid, and remove parentOids from MergeAttributes.

Pushed with those changes.  Thanks for the patch!

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

On Tue, 18 Dec 2018 at 08:21, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Pushed with those changes.  Thanks for the patch!

Thanks for committing it and thanks for reviewing it, Michael.

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

On Tue, Dec 18, 2018 at 09:36:17AM +1300, David Rowley wrote:
> On Tue, 18 Dec 2018 at 08:21, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> Pushed with those changes.  Thanks for the patch!
>
> Thanks for committing it and thanks for reviewing it, Michael.

Perhaps too early to speak, tests of sepgsql are broken after this
commit as per rhinoceros report:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2018-12-18%2000%3A45%3A01
--
Michael

On 2018-Dec-18, Michael Paquier wrote:

> On Tue, Dec 18, 2018 at 09:36:17AM +1300, David Rowley wrote:
> > On Tue, 18 Dec 2018 at 08:21, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> >> Pushed with those changes.  Thanks for the patch!
> > 
> > Thanks for committing it and thanks for reviewing it, Michael.
> 
> Perhaps too early to speak, tests of sepgsql are broken after this
> commit as per rhinoceros report:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2018-12-18%2000%3A45%3A01

Ah, thanks.

I think we need to accept the new output.  It is saying that previously
we would not invoke the hook on SET TABLESPACE for a partitioned table,
which makes sense, and now we do invoke it, which also makes sense.

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

On Mon, Dec 17, 2018 at 10:13:42PM -0300, Alvaro Herrera wrote:
> I think we need to accept the new output.  It is saying that previously
> we would not invoke the hook on SET TABLESPACE for a partitioned table,
> which makes sense, and now we do invoke it, which also makes sense.

Indeed, that looks right.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Dec 17, 2018 at 10:13:42PM -0300, Alvaro Herrera wrote:
>> I think we need to accept the new output.  It is saying that previously
>> we would not invoke the hook on SET TABLESPACE for a partitioned table,
>> which makes sense, and now we do invoke it, which also makes sense.

> Indeed, that looks right.

Agreed, done.

            regards, tom lane

On Tue, Dec 18, 2018 at 11:40:49AM -0500, Tom Lane wrote:
> Agreed, done.

Thanks Tom for stepping in!
--
Michael