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