Date: Sat, 5 Oct 2019 19:43:33 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: parallel restore sometimes fails for FKs to partitioned tables
Message-ID: <20191005224333.GA9738@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="envbJBWh7q8WU6mo"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk


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

Hello

While playing around I noticed that depending on the number of parallel
workers in pg_restore compared to the number of partitions a table has,
restoring an FK fails because the FK itself is restored before the index
partitions have completed restoring.  The exact conditions to cause the
failure seem to vary depending on whether the dump is schema-only or not.

This can seemingly be fixed by having pg_dump make the constraint depend
on the attach of each partition, as in the attached patch.  With this
patch I no longer see failures.


This patch is a bit weird because I added a new "simple list" type, to
store pointers.  One alternative would be to store the dumpId values for
the partitions instead, but we don't have a dumpId-typed simple list
either.  We could solve that by casting the dumpId to Oid, but that
seems almost as strange as the current proposal.

The other thing that makes this patch a little weird is that we have to
scan the list of indexes in the referenced partitioned table in order to
find the correct one.  This should be okay, as the number of indexes in
any one table is not expected to grow very large.  This isn't easy to
fix because we don't have a bsearchable array of indexes like we do of
other object types, and this already requires some contortions nearby.
Still, I'm not sure that this absolutely needs fixing now.


-- 
Álvaro Herrera                         Developer, https://www.PostgreSQL.org/

--envbJBWh7q8WU6mo
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="dump-fks-parallel.patch"

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 02a865f456..3549f7bc08 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -412,6 +412,9 @@ flagInhIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
 			addObjectDependency(&attachinfo[k].dobj,
 								parentidx->indextable->dobj.dumpId);
 
+			/* keep track of the list of partitions in the parent index */
+			simple_ptr_list_append(&parentidx->partattaches, &attachinfo[k].dobj);
+
 			k++;
 		}
 	}
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index a9c868b9af..06b07a5c96 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7109,6 +7109,7 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
 			indxinfo[j].indisclustered = (PQgetvalue(res, j, i_indisclustered)[0] == 't');
 			indxinfo[j].indisreplident = (PQgetvalue(res, j, i_indisreplident)[0] == 't');
 			indxinfo[j].parentidx = atooid(PQgetvalue(res, j, i_parentidx));
+			indxinfo[j].partattaches = (SimplePtrList) { NULL, NULL };
 			contype = *(PQgetvalue(res, j, i_contype));
 
 			if (contype == 'p' || contype == 'u' || contype == 'x')
@@ -7241,6 +7242,7 @@ getConstraints(Archive *fout, TableInfo tblinfo[], int numTables)
 				i_conoid,
 				i_conname,
 				i_confrelid,
+				i_conindid,
 				i_condef;
 	int			ntups;
 
@@ -7266,7 +7268,7 @@ getConstraints(Archive *fout, TableInfo tblinfo[], int numTables)
 		resetPQExpBuffer(query);
 		if (fout->remoteVersion >= 110000)
 			appendPQExpBuffer(query,
-							  "SELECT tableoid, oid, conname, confrelid, "
+							  "SELECT tableoid, oid, conname, confrelid, conindid, "
 							  "pg_catalog.pg_get_constraintdef(oid) AS condef "
 							  "FROM pg_catalog.pg_constraint "
 							  "WHERE conrelid = '%u'::pg_catalog.oid "
@@ -7275,7 +7277,7 @@ getConstraints(Archive *fout, TableInfo tblinfo[], int numTables)
 							  tbinfo->dobj.catId.oid);
 		else
 			appendPQExpBuffer(query,
-							  "SELECT tableoid, oid, conname, confrelid, "
+							  "SELECT tableoid, oid, conname, confrelid, 0 as conindid, "
 							  "pg_catalog.pg_get_constraintdef(oid) AS condef "
 							  "FROM pg_catalog.pg_constraint "
 							  "WHERE conrelid = '%u'::pg_catalog.oid "
@@ -7289,12 +7291,15 @@ getConstraints(Archive *fout, TableInfo tblinfo[], int numTables)
 		i_conoid = PQfnumber(res, "oid");
 		i_conname = PQfnumber(res, "conname");
 		i_confrelid = PQfnumber(res, "confrelid");
+		i_conindid = PQfnumber(res, "conindid");
 		i_condef = PQfnumber(res, "condef");
 
 		constrinfo = (ConstraintInfo *) pg_malloc(ntups * sizeof(ConstraintInfo));
 
 		for (j = 0; j < ntups; j++)
 		{
+			TableInfo *reftable;
+
 			constrinfo[j].dobj.objType = DO_FK_CONSTRAINT;
 			constrinfo[j].dobj.catId.tableoid = atooid(PQgetvalue(res, j, i_contableoid));
 			constrinfo[j].dobj.catId.oid = atooid(PQgetvalue(res, j, i_conoid));
@@ -7311,6 +7316,34 @@ getConstraints(Archive *fout, TableInfo tblinfo[], int numTables)
 			constrinfo[j].condeferred = false;
 			constrinfo[j].conislocal = true;
 			constrinfo[j].separate = true;
+
+			/*
+			 * Restoring an FK that points to a partitioned table requires
+			 * that all partition indexes have been attached beforehand.
+			 * Ensure that happens by making the constraint depend on each
+			 * index partition attach object.
+			 */
+			reftable = findTableByOid(constrinfo[j].confrelid);
+
+			if (reftable->relkind == RELKIND_PARTITIONED_TABLE)
+			{
+				IndxInfo   *refidx;
+				OId			indexOid = atooid(PQgetvalue(res, j, i_conindid));
+
+				for (int k = 0; k < reftable->numIndexes; k++)
+				{
+					if (reftable->indexes[k].dobj.catId.oid == indexOid)
+					{
+						SimplePtrListCell *cell;
+
+						refidx = &reftable->indexes[k];
+						for (cell = refidx->partattaches.head; cell; cell = cell->next)
+							addObjectDependency(&constrinfo[j].dobj,
+												((DumpableObject *) cell->ptr)->dumpId);
+						break;
+					}
+				}
+			}
 		}
 
 		PQclear(res);
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index c3c2ea1473..ce04f914e1 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -371,6 +371,8 @@ typedef struct _indxInfo
 	bool		indisclustered;
 	bool		indisreplident;
 	Oid			parentidx;		/* if partitioned, parent index OID */
+	SimplePtrList partattaches;	/* if partitioned, partition attach objects */
+
 	/* if there is an associated constraint object, its dumpId: */
 	DumpId		indexconstraint;
 } IndxInfo;
diff --git a/src/fe_utils/simple_list.c b/src/fe_utils/simple_list.c
index 8d605140a7..2232e8db73 100644
--- a/src/fe_utils/simple_list.c
+++ b/src/fe_utils/simple_list.c
@@ -114,3 +114,24 @@ simple_string_list_not_touched(SimpleStringList *list)
 	}
 	return NULL;
 }
+
+/*
+ * Append a pointer to the list.
+ *
+ * Caller must ensure that the pointer remains valid.
+ */
+void
+simple_ptr_list_append(SimplePtrList *list, void *ptr)
+{
+	SimplePtrListCell *cell;
+
+	cell = (SimplePtrListCell *) pg_malloc(sizeof(SimplePtrListCell));
+	cell->next = NULL;
+	cell->ptr = ptr;
+
+	if (list->tail)
+		list->tail->next = cell;
+	else
+		list->head = cell;
+	list->tail = cell;
+}
diff --git a/src/include/fe_utils/simple_list.h b/src/include/fe_utils/simple_list.h
index 8a95cbb3a8..1443b68888 100644
--- a/src/include/fe_utils/simple_list.h
+++ b/src/include/fe_utils/simple_list.h
@@ -43,6 +43,17 @@ typedef struct SimpleStringList
 	SimpleStringListCell *tail;
 } SimpleStringList;
 
+typedef struct SimplePtrListCell
+{
+	struct SimplePtrListCell *next;
+	void	   *ptr;
+} SimplePtrListCell;
+
+typedef struct SimplePtrList
+{
+	SimplePtrListCell *head;
+	SimplePtrListCell *tail;
+} SimplePtrList;
 
 extern void simple_oid_list_append(SimpleOidList *list, Oid val);
 extern bool simple_oid_list_member(SimpleOidList *list, Oid val);
@@ -52,4 +63,6 @@ extern bool simple_string_list_member(SimpleStringList *list, const char *val);
 
 extern const char *simple_string_list_not_touched(SimpleStringList *list);
 
+extern void simple_ptr_list_append(SimplePtrList *list, void *val);
+
 #endif							/* SIMPLE_LIST_H */

--envbJBWh7q8WU6mo--