Date: Tue, 9 Apr 2019 18:34:35 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: David Rowley <david.rowley@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pg_dump is broken for partition tablespaces
Message-ID: <20190409223435.GA14335@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="YiEDa0DAkWCtVeE4"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190409203507.GA25150@alvherre.pgsql>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


--YiEDa0DAkWCtVeE4
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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

--YiEDa0DAkWCtVeE4
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="tbspc-partitioned.patch"

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,

--YiEDa0DAkWCtVeE4--