Fix pg_dump/pg_restore to restore event triggers later.

tglsfdc · tglsfdc · commit 4c40b27b5064 · 2020-03-09T14:58:11.000-04:00
Previously, event triggers were restored just after regular triggers (and FK constraints, which are basically triggers). This is risky since an event trigger, once installed, could interfere with subsequent restore commands. Worse, because event triggers don't have any particular dependencies on any post-data objects, a parallel restore would consider them eligible to be restored the moment the post-data phase starts, allowing them to also interfere with restoration of a whole bunch of objects that would have been restored before them in a serial restore. There's no way to completely remove the risk of a misguided event trigger breaking the restore, since if nothing else it could break other event triggers. But we can certainly push them to later in the process to minimize the hazard. To fix, tweak the RestorePass mechanism introduced by commit 3eb9a5e so that event triggers are handled as part of the post-ACL processing pass (renaming the "REFRESH" pass to "POST_ACL" to reflect its more general use). This will cause them to restore after everything except matview refreshes, which seems OK since matview refreshes really ought to run in the post-restore state of the database. In a parallel restore, event triggers and matview refreshes might be intermixed, but that seems all right as well. Also update the code and comments in pg_dump_sort.c so that its idea of how things are sorted agrees with what actually happens due to the RestorePass mechanism. This is mostly cosmetic: it'll affect the order of objects in a dump's TOC, but not the actual restore order. But not changing that would be quite confusing to somebody reading the code. Back-patch to all supported branches. Fabrízio de Royes Mello, tweaked a bit by me Discussion: https://postgr.es/m/CAFcNs+ow1hmFox8P--3GSdtwz-S3Binb6ZmoP6Vk+Xg=K6eZNA@mail.gmail.com
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
@@ -671,11 +671,11 @@ RestoreArchive(Archive *AHX)
 	{
 		/*
 		 * In serial mode, process everything in three phases: normal items,
-		 * then ACLs, then matview refresh items.  We might be able to skip
-		 * one or both extra phases in some cases, eg data-only restores.
+		 * then ACLs, then post-ACL items.  We might be able to skip one or
+		 * both extra phases in some cases, eg data-only restores.
 		 */
 		bool		haveACL = false;
-		bool		haveRefresh = false;
+		bool		havePostACL = false;
 
 		for (te = AH->toc->next; te != AH->toc; te = te->next)
 		{
@@ -690,8 +690,8 @@ RestoreArchive(Archive *AHX)
 				case RESTORE_PASS_ACL:
 					haveACL = true;
 					break;
-				case RESTORE_PASS_REFRESH:
-					haveRefresh = true;
+				case RESTORE_PASS_POST_ACL:
+					havePostACL = true;
 					break;
 			}
 		}
@@ -706,12 +706,12 @@ RestoreArchive(Archive *AHX)
 			}
 		}
 
-		if (haveRefresh)
+		if (havePostACL)
 		{
 			for (te = AH->toc->next; te != AH->toc; te = te->next)
 			{
 				if ((te->reqs & (REQ_SCHEMA | REQ_DATA)) != 0 &&
-					_tocEntryRestorePass(te) == RESTORE_PASS_REFRESH)
+					_tocEntryRestorePass(te) == RESTORE_PASS_POST_ACL)
 					(void) restore_toc_entry(AH, te, false);
 			}
 		}
@@ -3088,8 +3088,9 @@ _tocEntryRestorePass(TocEntry *te)
 		strcmp(te->desc, "ACL LANGUAGE") == 0 ||
 		strcmp(te->desc, "DEFAULT ACL") == 0)
 		return RESTORE_PASS_ACL;
-	if (strcmp(te->desc, "MATERIALIZED VIEW DATA") == 0)
-		return RESTORE_PASS_REFRESH;
+	if (strcmp(te->desc, "EVENT TRIGGER") == 0 ||
+		strcmp(te->desc, "MATERIALIZED VIEW DATA") == 0)
+		return RESTORE_PASS_POST_ACL;
 	return RESTORE_PASS_MAIN;
 }
 
diff --git a/src/bin/pg_dump/pg_backup_archiver.h b/src/bin/pg_dump/pg_backup_archiver.h
@@ -211,20 +211,24 @@ typedef enum
  * data restore failures.  On the other hand, matview REFRESH commands should
  * come out after ACLs, as otherwise non-superuser-owned matviews might not
  * be able to execute.  (If the permissions at the time of dumping would not
- * allow a REFRESH, too bad; we won't fix that for you.)  These considerations
- * force us to make three passes over the TOC, restoring the appropriate
- * subset of items in each pass.  We assume that the dependency sort resulted
- * in an appropriate ordering of items within each subset.
+ * allow a REFRESH, too bad; we won't fix that for you.)  We also want event
+ * triggers to be restored after ACLs, so that they can't mess those up.
+ *
+ * These considerations force us to make three passes over the TOC,
+ * restoring the appropriate subset of items in each pass.  We assume that
+ * the dependency sort resulted in an appropriate ordering of items within
+ * each subset.
+ *
  * XXX This mechanism should be superseded by tracking dependencies on ACLs
  * properly; but we'll still need it for old dump files even after that.
  */
 typedef enum
 {
 	RESTORE_PASS_MAIN = 0,		/* Main pass (most TOC item types) */
 	RESTORE_PASS_ACL,			/* ACL item types */
-	RESTORE_PASS_REFRESH		/* Matview REFRESH items */
+	RESTORE_PASS_POST_ACL		/* Event trigger and matview refresh items */
 
-#define RESTORE_PASS_LAST RESTORE_PASS_REFRESH
+#define RESTORE_PASS_LAST RESTORE_PASS_POST_ACL
 } RestorePass;
 
 typedef enum
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
@@ -25,8 +25,15 @@
  * Sort priority for database object types.
  * Objects are sorted by type, and within a type by name.
  *
- * Because materialized views can potentially reference system views,
- * DO_REFRESH_MATVIEW should always be the last thing on the list.
+ * Triggers, event triggers, and materialized views are intentionally sorted
+ * late.  Triggers must be restored after all data modifications, so that
+ * they don't interfere with loading data.  Event triggers are restored
+ * next-to-last so that they don't interfere with object creations of any
+ * kind.  Matview refreshes are last because they should execute in the
+ * database's normal state (e.g., they must come after all ACLs are restored;
+ * also, if they choose to look at system catalogs, they should see the final
+ * restore state).  If you think to change this, see also the RestorePass
+ * mechanism in pg_backup_archiver.c.
  *
  * NOTE: object-type priorities must match the section assignments made in
  * pg_dump.c; that is, PRE_DATA objects must sort before DO_PRE_DATA_BOUNDARY,
@@ -67,18 +74,18 @@ static const int dbObjectTypePriority[] =
 	15,							/* DO_TSCONFIG */
 	16,							/* DO_FDW */
 	17,							/* DO_FOREIGN_SERVER */
-	33,							/* DO_DEFAULT_ACL */
+	38,							/* DO_DEFAULT_ACL --- done in ACL pass */
 	3,							/* DO_TRANSFORM */
 	21,							/* DO_BLOB */
 	25,							/* DO_BLOB_DATA */
 	22,							/* DO_PRE_DATA_BOUNDARY */
 	26,							/* DO_POST_DATA_BOUNDARY */
-	34,							/* DO_EVENT_TRIGGER */
-	39,							/* DO_REFRESH_MATVIEW */
-	35,							/* DO_POLICY */
-	36,							/* DO_PUBLICATION */
-	37,							/* DO_PUBLICATION_REL */
-	38							/* DO_SUBSCRIPTION */
+	39,							/* DO_EVENT_TRIGGER --- next to last! */
+	40,							/* DO_REFRESH_MATVIEW --- last! */
+	34,							/* DO_POLICY */
+	35,							/* DO_PUBLICATION */
+	36,							/* DO_PUBLICATION_REL */
+	37							/* DO_SUBSCRIPTION */
 };
 
 static DumpId preDataBoundId;

Original file line number	Diff line number	Diff line change
`@@ -671,11 +671,11 @@ RestoreArchive(Archive *AHX)`
`671`	`671`	`{`
`672`	`672`	`/*`
`673`	`673`	`* In serial mode, process everything in three phases: normal items,`
`674`		`- * then ACLs, then matview refresh items. We might be able to skip`
`675`		`- * one or both extra phases in some cases, eg data-only restores.`
	`674`	`+ * then ACLs, then post-ACL items. We might be able to skip one or`
	`675`	`+ * both extra phases in some cases, eg data-only restores.`
`676`	`676`	`*/`
`677`	`677`	`bool haveACL = false;`
`678`		`- bool haveRefresh = false;`
	`678`	`+ bool havePostACL = false;`
`679`	`679`
`680`	`680`	`for (te = AH->toc->next; te != AH->toc; te = te->next)`
`681`	`681`	`{`
`@@ -690,8 +690,8 @@ RestoreArchive(Archive *AHX)`
`690`	`690`	`case RESTORE_PASS_ACL:`
`691`	`691`	`haveACL = true;`
`692`	`692`	`break;`
`693`		`- case RESTORE_PASS_REFRESH:`
`694`		`- haveRefresh = true;`
	`693`	`+ case RESTORE_PASS_POST_ACL:`
	`694`	`+ havePostACL = true;`
`695`	`695`	`break;`
`696`	`696`	`}`
`697`	`697`	`}`
`@@ -706,12 +706,12 @@ RestoreArchive(Archive *AHX)`
`706`	`706`	`}`
`707`	`707`	`}`
`708`	`708`
`709`		`- if (haveRefresh)`
	`709`	`+ if (havePostACL)`
`710`	`710`	`{`
`711`	`711`	`for (te = AH->toc->next; te != AH->toc; te = te->next)`
`712`	`712`	`{`
`713`	`713`	`if ((te->reqs & (REQ_SCHEMA \| REQ_DATA)) != 0 &&`
`714`		`- _tocEntryRestorePass(te) == RESTORE_PASS_REFRESH)`
	`714`	`+ _tocEntryRestorePass(te) == RESTORE_PASS_POST_ACL)`
`715`	`715`	`(void) restore_toc_entry(AH, te, false);`
`716`	`716`	`}`
`717`	`717`	`}`
`@@ -3088,8 +3088,9 @@ _tocEntryRestorePass(TocEntry *te)`
`3088`	`3088`	`strcmp(te->desc, "ACL LANGUAGE") == 0 \|\|`
`3089`	`3089`	`strcmp(te->desc, "DEFAULT ACL") == 0)`
`3090`	`3090`	`return RESTORE_PASS_ACL;`
`3091`		`- if (strcmp(te->desc, "MATERIALIZED VIEW DATA") == 0)`
`3092`		`- return RESTORE_PASS_REFRESH;`
	`3091`	`+ if (strcmp(te->desc, "EVENT TRIGGER") == 0 \|\|`
	`3092`	`+ strcmp(te->desc, "MATERIALIZED VIEW DATA") == 0)`
	`3093`	`+ return RESTORE_PASS_POST_ACL;`
`3093`	`3094`	`return RESTORE_PASS_MAIN;`
`3094`	`3095`	`}`
`3095`	`3096`