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

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