On 2019-Apr-09, Alvaro Herrera wrote: > However, in order to fix the pg_dump behavior for the partitioned rel, > we would need to emit the tablespace differently, i.e. not use SET > default_tablespace, but instead attach the tablespace clause to the > CREATE TABLE line. > > I'll go have a look at what problems are there with doing that.
I tried with the attached tbspc-partitioned.patch (on top of the previous patch). It makes several additional cases work correctly, but causes much more pain than it fixes: 1. if a partitioned table is created without a tablespace spec, then pg_dump dumps it with a clause like TABLESPACE "". 2. If you fix that to make pg_dump omit the tablespace clause if the tablespace is default, then there's no SET nor TABLESPACE clauses, so the table is created in the tablespace of the previously dumped table. Useless. 3. You can probably make the above work anyway with enough hacks, but by the time you're finished, the code stinks like months-only fish. 4. Even if you ignore the odor, all the resulting CREATE TABLE commands will fail if you run them in a database that doesn't contain the tablespaces in question. That is, it negates one of the main points of using "SET default_tablespace" in the first place. I therefore decree that this approach is not a solution and never will be. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 6b77eff0af1..3a778ce5c05 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -386,9 +386,19 @@ heap_create(const char *relname, * tablespace, the pg_class entry will still match where CREATE DATABASE * will put the physically copied relation. * - * Yes, this is a bit of a hack. + * We make an exception for partitioned relations, though: for them, + * reltablespace does not really indicate a storage location (there isn't + * any storage in the first place) but instead it only determines where + * storage for partitions is to be created later. We must make this + * specific even with the default tablespace, so that partitions are + * created in the right location even if the database's default tablespace + * is later changed. + * + * Yes, this is all a bit of a hack. */ - if (reltablespace == MyDatabaseTableSpace) + if (reltablespace == MyDatabaseTableSpace && + relkind != RELKIND_PARTITIONED_TABLE && + relkind != RELKIND_PARTITIONED_INDEX) reltablespace = InvalidOid; /* diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index a7987790df5..e15e92c1a2a 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -15785,6 +15785,14 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo) if (ftoptions && ftoptions[0]) appendPQExpBuffer(q, "\nOPTIONS (\n %s\n)", ftoptions); + /* + * For partitioned tables, emit the tablespace spec (if there is one). + */ + if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE && + tbinfo->reltablespace) + appendPQExpBuffer(q, "\nTABLESPACE %s", + fmtId(tbinfo->reltablespace)); + /* * For materialized views, create the AS clause just like a view. At * this point, we always mark the view as not populated. @@ -16166,7 +16174,8 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo) ArchiveEntry(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId, ARCHIVE_OPTS(.tag = tbinfo->dobj.name, .namespace = tbinfo->dobj.namespace->dobj.name, - .tablespace = (tbinfo->relkind == RELKIND_VIEW) ? + .tablespace = (tbinfo->relkind == RELKIND_VIEW || + tbinfo->relkind == RELKIND_PARTITIONED_TABLE) ? NULL : tbinfo->reltablespace, .tableam = tableam, .owner = tbinfo->rolname, @@ -16334,7 +16343,11 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo) indxinfo->dobj.catId.oid, true); /* Plain secondary index */ - appendPQExpBuffer(q, "%s;\n", indxinfo->indexdef); + appendPQExpBuffer(q, "%s\n", indxinfo->indexdef); + if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE && + indxinfo->tablespace) + appendPQExpBuffer(q, "TABLESPACE %s", indxinfo->tablespace); + appendPQExpBufferStr(q, ";"); /* * Append ALTER TABLE commands as needed to set properties that we @@ -16395,7 +16408,8 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo) ArchiveEntry(fout, indxinfo->dobj.catId, indxinfo->dobj.dumpId, ARCHIVE_OPTS(.tag = indxinfo->dobj.name, .namespace = tbinfo->dobj.namespace->dobj.name, - .tablespace = indxinfo->tablespace, + .tablespace = (tbinfo->relkind == RELKIND_PARTITIONED_TABLE ? + NULL : indxinfo->tablespace), .owner = tbinfo->rolname, .description = "INDEX", .section = SECTION_POST_DATA, @@ -16618,6 +16632,11 @@ dumpConstraint(Archive *fout, ConstraintInfo *coninfo) appendPQExpBufferStr(q, " INITIALLY DEFERRED"); } + if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE && + indxinfo->tablespace) + appendPQExpBuffer(q, "USING INDEX TABLESPACE %s", + indxinfo->tablespace); + appendPQExpBufferStr(q, ";\n"); } @@ -16658,7 +16677,8 @@ dumpConstraint(Archive *fout, ConstraintInfo *coninfo) ArchiveEntry(fout, coninfo->dobj.catId, coninfo->dobj.dumpId, ARCHIVE_OPTS(.tag = tag, .namespace = tbinfo->dobj.namespace->dobj.name, - .tablespace = indxinfo->tablespace, + .tablespace = (tbinfo->relkind == RELKIND_PARTITIONED_TABLE ? + NULL : indxinfo->tablespace), .owner = tbinfo->rolname, .description = "CONSTRAINT", .section = SECTION_POST_DATA,
