Date: Thu, 18 Apr 2024 16:19:24 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: improve performance of pg_dump --binary-upgrade
Message-ID: <20240418211924.GA3828609@nathanxps13>
References: <20240418041712.GA3441570@nathanxps13>
 <F579A6BD-C431-4043-B58C-FE5B07FC2977@yesql.se>
 <20240418152301.GB3501884@nathanxps13>
 <20240418202813.GA3669511@nathanxps13>
 <93129B02-33E0-4591-96B4-0B9C2183C7B4@yesql.se>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="yrj/dFKFPuw6o+aM"
Content-Disposition: inline
In-Reply-To: <93129B02-33E0-4591-96B4-0B9C2183C7B4@yesql.se>
Archived-At: 
 <https://www.postgresql.org/message-id/20240418211924.GA3828609%40nathanxps13>
Precedence: bulk


--yrj/dFKFPuw6o+aM
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Apr 18, 2024 at 10:33:08PM +0200, Daniel Gustafsson wrote:
> From a read-through they look good, a very nice performance improvement in an
> important path.  I think it would be nice with some comments on the
> BinaryUpgradeClassOids struct (since the code using it is thousands of lines
> away), and a comment on the if (oids == NULL) block explaining the caching.

Added.  Thanks for reviewing!  Unfortunately, this one will have to sit for
a couple months...

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

--yrj/dFKFPuw6o+aM
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0001-Remove-is_index-parameter-from-binary_upgrade_set.patch"

From 6797e4e1fc1a63f710ed0a409b1337880eb91ad4 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 18 Apr 2024 15:15:19 -0500
Subject: [PATCH v3 1/2] Remove is_index parameter from
 binary_upgrade_set_pg_class_oids().

---
 src/bin/pg_dump/pg_dump.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 6d2f3fdef3..58c77a5e2b 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -324,7 +324,7 @@ static void binary_upgrade_set_type_oids_by_rel(Archive *fout,
 												const TableInfo *tbinfo);
 static void binary_upgrade_set_pg_class_oids(Archive *fout,
 											 PQExpBuffer upgrade_buffer,
-											 Oid pg_class_oid, bool is_index);
+											 Oid pg_class_oid);
 static void binary_upgrade_extension_member(PQExpBuffer upgrade_buffer,
 											const DumpableObject *dobj,
 											const char *objtype,
@@ -5394,8 +5394,7 @@ binary_upgrade_set_type_oids_by_rel(Archive *fout,
 
 static void
 binary_upgrade_set_pg_class_oids(Archive *fout,
-								 PQExpBuffer upgrade_buffer, Oid pg_class_oid,
-								 bool is_index)
+								 PQExpBuffer upgrade_buffer, Oid pg_class_oid)
 {
 	PQExpBuffer upgrade_query = createPQExpBuffer();
 	PGresult   *upgrade_res;
@@ -5444,7 +5443,8 @@ binary_upgrade_set_pg_class_oids(Archive *fout,
 	appendPQExpBufferStr(upgrade_buffer,
 						 "\n-- For binary upgrade, must preserve pg_class oids and relfilenodes\n");
 
-	if (!is_index)
+	if (relkind != RELKIND_INDEX &&
+		relkind != RELKIND_PARTITIONED_INDEX)
 	{
 		appendPQExpBuffer(upgrade_buffer,
 						  "SELECT pg_catalog.binary_upgrade_set_next_heap_pg_class_oid('%u'::pg_catalog.oid);\n",
@@ -11864,7 +11864,7 @@ dumpCompositeType(Archive *fout, const TypeInfo *tyinfo)
 		binary_upgrade_set_type_oids_by_type_oid(fout, q,
 												 tyinfo->dobj.catId.oid,
 												 false, false);
-		binary_upgrade_set_pg_class_oids(fout, q, tyinfo->typrelid, false);
+		binary_upgrade_set_pg_class_oids(fout, q, tyinfo->typrelid);
 	}
 
 	qtypname = pg_strdup(fmtId(tyinfo->dobj.name));
@@ -15998,7 +15998,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
 
 		if (dopt->binary_upgrade)
 			binary_upgrade_set_pg_class_oids(fout, q,
-											 tbinfo->dobj.catId.oid, false);
+											 tbinfo->dobj.catId.oid);
 
 		appendPQExpBuffer(q, "CREATE VIEW %s", qualrelname);
 
@@ -16100,7 +16100,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
 
 		if (dopt->binary_upgrade)
 			binary_upgrade_set_pg_class_oids(fout, q,
-											 tbinfo->dobj.catId.oid, false);
+											 tbinfo->dobj.catId.oid);
 
 		appendPQExpBuffer(q, "CREATE %s%s %s",
 						  tbinfo->relpersistence == RELPERSISTENCE_UNLOGGED ?
@@ -16990,7 +16990,7 @@ dumpIndex(Archive *fout, const IndxInfo *indxinfo)
 
 		if (dopt->binary_upgrade)
 			binary_upgrade_set_pg_class_oids(fout, q,
-											 indxinfo->dobj.catId.oid, true);
+											 indxinfo->dobj.catId.oid);
 
 		/* Plain secondary index */
 		appendPQExpBuffer(q, "%s;\n", indxinfo->indexdef);
@@ -17244,7 +17244,7 @@ dumpConstraint(Archive *fout, const ConstraintInfo *coninfo)
 
 		if (dopt->binary_upgrade)
 			binary_upgrade_set_pg_class_oids(fout, q,
-											 indxinfo->dobj.catId.oid, true);
+											 indxinfo->dobj.catId.oid);
 
 		appendPQExpBuffer(q, "ALTER %sTABLE ONLY %s\n", foreign,
 						  fmtQualifiedDumpable(tbinfo));
@@ -17653,7 +17653,7 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo)
 	if (dopt->binary_upgrade)
 	{
 		binary_upgrade_set_pg_class_oids(fout, query,
-										 tbinfo->dobj.catId.oid, false);
+										 tbinfo->dobj.catId.oid);
 
 		/*
 		 * In older PG versions a sequence will have a pg_type entry, but v14
-- 
2.25.1


--yrj/dFKFPuw6o+aM
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0002-Improve-performance-of-pg_dump-binary-upgrade.patch"

From b3ae9df69fdbf386d09250f1c225ecd8665141a7 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 17 Apr 2024 22:55:27 -0500
Subject: [PATCH v3 2/2] Improve performance of pg_dump --binary-upgrade.

---
 src/bin/pg_dump/pg_dump.c        | 126 ++++++++++++++++++++-----------
 src/tools/pgindent/typedefs.list |   1 +
 2 files changed, 81 insertions(+), 46 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 58c77a5e2b..b1f06a199b 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -55,6 +55,7 @@
 #include "catalog/pg_trigger_d.h"
 #include "catalog/pg_type_d.h"
 #include "common/connect.h"
+#include "common/int.h"
 #include "common/relpath.h"
 #include "compress_io.h"
 #include "dumputils.h"
@@ -99,6 +100,21 @@ typedef enum OidOptions
 	zeroAsNone = 4,
 } OidOptions;
 
+/*
+ * This struct is used for the sorted array populated by
+ * binary_upgrade_set_pg_class_oids().
+ */
+typedef struct
+{
+	Oid			oid;
+	char		relkind;
+	RelFileNumber relfilenode;
+	Oid			reltoastrelid;
+	RelFileNumber toast_relfilenode;
+	Oid			indexrelid;
+	RelFileNumber toast_index_relfilenode;
+} BinaryUpgradeClassOids;
+
 /* global decls */
 static bool dosync = true;		/* Issue fsync() to make dump durable on disk. */
 
@@ -5392,18 +5408,60 @@ binary_upgrade_set_type_oids_by_rel(Archive *fout,
 												 pg_type_oid, false, false);
 }
 
+static int
+BinaryUpgradeClassOidsCmp(const void *p1, const void *p2)
+{
+	BinaryUpgradeClassOids v1 = *((const BinaryUpgradeClassOids *) p1);
+	BinaryUpgradeClassOids v2 = *((const BinaryUpgradeClassOids *) p2);
+
+	return pg_cmp_u32(v1.oid, v2.oid);
+}
+
 static void
 binary_upgrade_set_pg_class_oids(Archive *fout,
 								 PQExpBuffer upgrade_buffer, Oid pg_class_oid)
 {
-	PQExpBuffer upgrade_query = createPQExpBuffer();
-	PGresult   *upgrade_res;
-	RelFileNumber relfilenumber;
-	Oid			toast_oid;
-	RelFileNumber toast_relfilenumber;
-	char		relkind;
-	Oid			toast_index_oid;
-	RelFileNumber toast_index_relfilenumber;
+	static BinaryUpgradeClassOids *oids = NULL;
+	static int	oids_len = 0;
+	BinaryUpgradeClassOids key = {0, '?', 0, 0, 0, 0, 0};
+	BinaryUpgradeClassOids *entry;
+
+	/*
+	 * If this is the first call to this function, gather all the information
+	 * we'll need for this call and future calls into a sorted array.  This
+	 * allows us to avoid executing this expensive query many times.
+	 */
+	if (oids == NULL)
+	{
+		PGresult   *res;
+
+		res = ExecuteSqlQuery(fout,
+							  "SELECT c.oid, c.relkind, c.relfilenode, c.reltoastrelid, ct.relfilenode AS toast_relfilenode, "
+							  "i.indexrelid, cti.relfilenode AS toast_index_relfilenode "
+							  "FROM pg_catalog.pg_class c LEFT JOIN "
+							  "pg_catalog.pg_index i ON (c.reltoastrelid = i.indrelid AND i.indisvalid) "
+							  "LEFT JOIN pg_catalog.pg_class ct ON (c.reltoastrelid = ct.oid) "
+							  "LEFT JOIN pg_catalog.pg_class AS cti ON (i.indexrelid = cti.oid) "
+							  "ORDER BY c.oid;",
+							  PGRES_TUPLES_OK);
+
+		oids_len = PQntuples(res);
+		oids = (BinaryUpgradeClassOids *)
+			pg_malloc(oids_len * sizeof(BinaryUpgradeClassOids));
+
+		for (int i = 0; i < oids_len; i++)
+		{
+			oids[i].oid = atooid(PQgetvalue(res, i, 0));
+			oids[i].relkind = *PQgetvalue(res, i, 1);
+			oids[i].relfilenode = atooid(PQgetvalue(res, i, 2));
+			oids[i].reltoastrelid = atooid(PQgetvalue(res, i, 3));
+			oids[i].toast_relfilenode = atooid(PQgetvalue(res, i, 4));
+			oids[i].indexrelid = atooid(PQgetvalue(res, i, 5));
+			oids[i].toast_index_relfilenode = atooid(PQgetvalue(res, i, 6));
+		}
+
+		PQclear(res);
+	}
 
 	/*
 	 * Preserve the OID and relfilenumber of the table, table's index, table's
@@ -5416,35 +5474,15 @@ binary_upgrade_set_pg_class_oids(Archive *fout,
 	 * by the new backend, so we can copy the files during binary upgrade
 	 * without worrying about this case.
 	 */
-	appendPQExpBuffer(upgrade_query,
-					  "SELECT c.relkind, c.relfilenode, c.reltoastrelid, ct.relfilenode AS toast_relfilenode, i.indexrelid, cti.relfilenode AS toast_index_relfilenode "
-					  "FROM pg_catalog.pg_class c LEFT JOIN "
-					  "pg_catalog.pg_index i ON (c.reltoastrelid = i.indrelid AND i.indisvalid) "
-					  "LEFT JOIN pg_catalog.pg_class ct ON (c.reltoastrelid = ct.oid) "
-					  "LEFT JOIN pg_catalog.pg_class AS cti ON (i.indexrelid = cti.oid) "
-					  "WHERE c.oid = '%u'::pg_catalog.oid;",
-					  pg_class_oid);
-
-	upgrade_res = ExecuteSqlQueryForSingleRow(fout, upgrade_query->data);
-
-	relkind = *PQgetvalue(upgrade_res, 0, PQfnumber(upgrade_res, "relkind"));
-
-	relfilenumber = atooid(PQgetvalue(upgrade_res, 0,
-									  PQfnumber(upgrade_res, "relfilenode")));
-	toast_oid = atooid(PQgetvalue(upgrade_res, 0,
-								  PQfnumber(upgrade_res, "reltoastrelid")));
-	toast_relfilenumber = atooid(PQgetvalue(upgrade_res, 0,
-											PQfnumber(upgrade_res, "toast_relfilenode")));
-	toast_index_oid = atooid(PQgetvalue(upgrade_res, 0,
-										PQfnumber(upgrade_res, "indexrelid")));
-	toast_index_relfilenumber = atooid(PQgetvalue(upgrade_res, 0,
-												  PQfnumber(upgrade_res, "toast_index_relfilenode")));
+	key.oid = pg_class_oid;
+	entry = bsearch(&key, oids, oids_len, sizeof(BinaryUpgradeClassOids),
+					BinaryUpgradeClassOidsCmp);
 
 	appendPQExpBufferStr(upgrade_buffer,
 						 "\n-- For binary upgrade, must preserve pg_class oids and relfilenodes\n");
 
-	if (relkind != RELKIND_INDEX &&
-		relkind != RELKIND_PARTITIONED_INDEX)
+	if (entry->relkind != RELKIND_INDEX &&
+		entry->relkind != RELKIND_PARTITIONED_INDEX)
 	{
 		appendPQExpBuffer(upgrade_buffer,
 						  "SELECT pg_catalog.binary_upgrade_set_next_heap_pg_class_oid('%u'::pg_catalog.oid);\n",
@@ -5455,35 +5493,33 @@ binary_upgrade_set_pg_class_oids(Archive *fout,
 		 * partitioned tables have a relfilenumber, which should not be
 		 * preserved when upgrading.
 		 */
-		if (RelFileNumberIsValid(relfilenumber) && relkind != RELKIND_PARTITIONED_TABLE)
+		if (RelFileNumberIsValid(entry->relfilenode) && entry->relkind != RELKIND_PARTITIONED_TABLE)
 			appendPQExpBuffer(upgrade_buffer,
 							  "SELECT pg_catalog.binary_upgrade_set_next_heap_relfilenode('%u'::pg_catalog.oid);\n",
-							  relfilenumber);
+							  entry->relfilenode);
 
 		/*
 		 * In a pre-v12 database, partitioned tables might be marked as having
 		 * toast tables, but we should ignore them if so.
 		 */
-		if (OidIsValid(toast_oid) &&
-			relkind != RELKIND_PARTITIONED_TABLE)
+		if (OidIsValid(entry->reltoastrelid) &&
+			entry->relkind != RELKIND_PARTITIONED_TABLE)
 		{
 			appendPQExpBuffer(upgrade_buffer,
 							  "SELECT pg_catalog.binary_upgrade_set_next_toast_pg_class_oid('%u'::pg_catalog.oid);\n",
-							  toast_oid);
+							  entry->reltoastrelid);
 			appendPQExpBuffer(upgrade_buffer,
 							  "SELECT pg_catalog.binary_upgrade_set_next_toast_relfilenode('%u'::pg_catalog.oid);\n",
-							  toast_relfilenumber);
+							  entry->toast_relfilenode);
 
 			/* every toast table has an index */
 			appendPQExpBuffer(upgrade_buffer,
 							  "SELECT pg_catalog.binary_upgrade_set_next_index_pg_class_oid('%u'::pg_catalog.oid);\n",
-							  toast_index_oid);
+							  entry->indexrelid);
 			appendPQExpBuffer(upgrade_buffer,
 							  "SELECT pg_catalog.binary_upgrade_set_next_index_relfilenode('%u'::pg_catalog.oid);\n",
-							  toast_index_relfilenumber);
+							  entry->toast_index_relfilenode);
 		}
-
-		PQclear(upgrade_res);
 	}
 	else
 	{
@@ -5493,12 +5529,10 @@ binary_upgrade_set_pg_class_oids(Archive *fout,
 						  pg_class_oid);
 		appendPQExpBuffer(upgrade_buffer,
 						  "SELECT pg_catalog.binary_upgrade_set_next_index_relfilenode('%u'::pg_catalog.oid);\n",
-						  relfilenumber);
+						  entry->relfilenode);
 	}
 
 	appendPQExpBufferChar(upgrade_buffer, '\n');
-
-	destroyPQExpBuffer(upgrade_query);
 }
 
 /*
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index d551ada325..c8062a1a20 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -255,6 +255,7 @@ BernoulliSamplerData
 BgWorkerStartTime
 BgwHandleStatus
 BinaryArithmFunc
+BinaryUpgradeClassOids
 BindParamCbData
 BipartiteMatchState
 BitString
-- 
2.25.1


--yrj/dFKFPuw6o+aM--