pg_dump: Lock all relations, not just plain tables

alvherre · alvherre · commit d9ed510ef64b · 2020-10-27T14:31:37.000-03:00
Now that LOCK TABLE can take any relation type, acquire lock on all relations that are to be dumped. This prevents schema changes or deadlock errors that could cause a dump to fail after expending much effort. The server is tested to have the capability and the feature disabled if it doesn't, so that a patched pg_dump doesn't fail when connecting to an unpatched server. Backpatch to 9.5. Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Reported-by: Wells Oliver <wells.oliver@gmail.com> Discussion: https://postgr.es/m/20201021200659.GA32358@alvherre.pgsql
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
@@ -187,6 +187,8 @@ typedef struct Archive
 	int			minRemoteVersion;		/* allowable range */
 	int			maxRemoteVersion;
 
+	bool		hasGenericLockTable;	/* can LOCK TABLE do non-table rels */
+
 	int			numWorkers;		/* number of parallel processes */
 	char	   *sync_snapshot_id;		/* sync snapshot id for parallel
 										 * operation */
diff --git a/src/bin/pg_dump/pg_backup_db.c b/src/bin/pg_dump/pg_backup_db.c
@@ -551,6 +551,71 @@ EndDBCopyMode(Archive *AHX, const char *tocEntryTag)
 	}
 }
 
+/*
+ * Does LOCK TABLE work on non-table relations on this server?
+ *
+ * Note: assumes it is called out of any transaction
+ */
+bool
+IsLockTableGeneric(Archive *AHX)
+{
+	ArchiveHandle *AH = (ArchiveHandle *) AHX;
+	PGresult *res;
+	char	 *sqlstate;
+	bool	retval;
+
+	if (AHX->remoteVersion >= 140000)
+		return true;
+	else if (AHX->remoteVersion < 90500)
+		return false;
+
+	StartTransaction(AHX);
+
+	/*
+	 * Try a LOCK TABLE on a well-known non-table catalog; WRONG_OBJECT_TYPE
+	 * tells us that this server doesn't support locking non-table rels, while
+	 * LOCK_NOT_AVAILABLE and INSUFFICIENT_PRIVILEGE tell us that it does.
+	 * Report anything else as a fatal problem.
+	 */
+#define ERRCODE_INSUFFICIENT_PRIVILEGE	"42501"
+#define ERRCODE_WRONG_OBJECT_TYPE	"42809"
+#define ERRCODE_LOCK_NOT_AVAILABLE	"55P03"
+	res = PQexec(AH->connection,
+				 "LOCK TABLE pg_catalog.pg_class_tblspc_relfilenode_index IN ACCESS SHARE MODE NOWAIT");
+	switch (PQresultStatus(res))
+	{
+		case PGRES_COMMAND_OK:
+			retval = true;
+			break;
+		case PGRES_FATAL_ERROR:
+			sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE);
+			if (strcmp(sqlstate, ERRCODE_WRONG_OBJECT_TYPE) == 0)
+			{
+				retval = false;
+				break;
+			}
+			else if (strcmp(sqlstate, ERRCODE_LOCK_NOT_AVAILABLE) == 0 ||
+					 strcmp(sqlstate, ERRCODE_INSUFFICIENT_PRIVILEGE) == 0)
+			{
+				retval = true;
+				break;
+			}
+			/* else, falls through */
+		default:
+			warn_or_exit_horribly(AH, modulename,
+								  "LOCK TABLE failed for \"%s\": %s",
+								  "pg_catalog.pg_class_tblspc_relfilenode_index",
+								  PQerrorMessage(AH->connection));
+			retval = false;		/* not reached */
+			break;
+	}
+	PQclear(res);
+
+	CommitTransaction(AHX);
+
+	return retval;
+}
+
 void
 StartTransaction(Archive *AHX)
 {
diff --git a/src/bin/pg_dump/pg_backup_db.h b/src/bin/pg_dump/pg_backup_db.h
@@ -20,6 +20,8 @@ extern PGresult *ExecuteSqlQueryForSingleRow(Archive *fout, char *query);
 
 extern void EndDBCopyMode(Archive *AHX, const char *tocEntryTag);
 
+extern bool IsLockTableGeneric(Archive *AHX);
+
 extern void StartTransaction(Archive *AHX);
 extern void CommitTransaction(Archive *AHX);
 
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
@@ -1062,6 +1062,9 @@ setup_connection(Archive *AH, const char *dumpencoding,
 			ExecuteSqlStatement(AH, "SET row_security = off");
 	}
 
+	/* Detect whether LOCK TABLE can handle non-table relations */
+	AH->hasGenericLockTable = IsLockTableGeneric(AH);
+
 	/*
 	 * Start transaction-snapshot mode transaction to dump consistent data.
 	 */
@@ -5398,10 +5401,12 @@ getTables(Archive *fout, int *numTables)
 		 * assume our lock on the child is enough to prevent schema
 		 * alterations to parent tables.
 		 *
-		 * NOTE: it'd be kinda nice to lock other relations too, not only
-		 * plain tables, but the backend doesn't presently allow that.
+		 * On server versions that support it, we lock all relations not just
+		 * plain tables.
 		 */
-		if (tblinfo[i].dobj.dump && tblinfo[i].relkind == RELKIND_RELATION)
+		if (tblinfo[i].dobj.dump &&
+			(fout->hasGenericLockTable ||
+			 tblinfo[i].relkind == RELKIND_RELATION))
 		{
 			resetPQExpBuffer(query);
 			appendPQExpBuffer(query,
