Feedback-ID: ie3de48e3:Fastmail
Date: Mon, 14 Apr 2025 14:20:10 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: Andrew Dunstan <andrew@dunslane.net>, 
	Mahendra Singh Thalor <mahi6run@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: pgsql: Non text modes for pg_dumpall, correspondingly change
 pg_restore
Message-ID: <202504141220.343fmoxfsbj4@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2h4sxwwjii2o4jzh"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <E1u0nMP-002gkh-0E@gemulon.postgresql.org>
Archived-At: <https://www.postgresql.org/message-id/202504141220.343fmoxfsbj4%40alvherre.pgsql>
Precedence: bulk


--2h4sxwwjii2o4jzh
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2025-Apr-04, Andrew Dunstan wrote:

> Non text modes for pg_dumpall, correspondingly change pg_restore

I think the new oid_string_list stuff in this commit is unnecessary, and
we can remove a bunch of lines by simplifying that to using
SimplePtrList, as the attached illustrates.  (Perhaps the names of
members of the proposed struct can be improved.)

I also propose to remove the open-coded implementation of pg_get_line.

WDYT?

I'm also not sure about the double sscanf() business there ... There
must be a better way to do this.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

--2h4sxwwjii2o4jzh
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0001-remove-unnecessary-oid_string-list-stuff.patch"

From 91d1fcee443e96d09fa6c23f30bdee7da279ada5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@alvh.no-ip.org>
Date: Mon, 14 Apr 2025 13:57:48 +0200
Subject: [PATCH] remove unnecessary oid_string list stuff

Also use pg_get_line_buf() instead of open-coding it
---
 src/bin/pg_dump/pg_restore.c       | 93 ++++++++++++++++++------------
 src/fe_utils/simple_list.c         | 41 -------------
 src/include/fe_utils/simple_list.h | 16 -----
 src/tools/pgindent/typedefs.list   |  1 +
 4 files changed, 57 insertions(+), 94 deletions(-)

diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index ff4bb320fc9..4ace56891e7 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -67,10 +67,20 @@ static int	process_global_sql_commands(PGconn *conn, const char *dumpdirpath,
 										const char *outfile);
 static void copy_or_print_global_file(const char *outfile, FILE *pfile);
 static int	get_dbnames_list_to_restore(PGconn *conn,
-										SimpleOidStringList *dbname_oid_list,
+										SimplePtrList *dbname_oid_list,
 										SimpleStringList db_exclude_patterns);
 static int	get_dbname_oid_list_from_mfile(const char *dumpdirpath,
-										   SimpleOidStringList *dbname_oid_list);
+										   SimplePtrList *dbname_oid_list);
+
+/*
+ * Stores a database OID and the corresponding name.
+ */
+typedef struct DbOidName
+{
+	Oid			oid;
+	char		str[FLEXIBLE_ARRAY_MEMBER]; /* null-terminated string here */
+} DbOidName;
+
 
 int
 main(int argc, char **argv)
@@ -927,7 +937,7 @@ read_one_statement(StringInfo inBuf, FILE *pfile)
  */
 static int
 get_dbnames_list_to_restore(PGconn *conn,
-							SimpleOidStringList *dbname_oid_list,
+							SimplePtrList *dbname_oid_list,
 							SimpleStringList db_exclude_patterns)
 {
 	int			count_db = 0;
@@ -943,13 +953,14 @@ get_dbnames_list_to_restore(PGconn *conn,
 	 * Process one by one all dbnames and if specified to skip restoring, then
 	 * remove dbname from list.
 	 */
-	for (SimpleOidStringListCell * db_cell = dbname_oid_list->head;
+	for (SimplePtrListCell *db_cell = dbname_oid_list->head;
 		 db_cell; db_cell = db_cell->next)
 	{
+		DbOidName  *dbidname = (DbOidName *) db_cell->ptr;
 		bool		skip_db_restore = false;
 		PQExpBuffer db_lit = createPQExpBuffer();
 
-		appendStringLiteralConn(db_lit, db_cell->str, conn);
+		appendStringLiteralConn(db_lit, dbidname->str, conn);
 
 		for (SimpleStringListCell *pat_cell = db_exclude_patterns.head; pat_cell; pat_cell = pat_cell->next)
 		{
@@ -957,7 +968,7 @@ get_dbnames_list_to_restore(PGconn *conn,
 			 * If there is an exact match then we don't need to try a pattern
 			 * match
 			 */
-			if (pg_strcasecmp(db_cell->str, pat_cell->val) == 0)
+			if (pg_strcasecmp(dbidname->str, pat_cell->val) == 0)
 				skip_db_restore = true;
 			/* Otherwise, try a pattern match if there is a connection */
 			else if (conn)
@@ -972,7 +983,7 @@ get_dbnames_list_to_restore(PGconn *conn,
 				if (dotcnt > 0)
 				{
 					pg_log_error("improper qualified name (too many dotted names): %s",
-								 db_cell->str);
+								 dbidname->str);
 					PQfinish(conn);
 					exit_nicely(1);
 				}
@@ -982,7 +993,7 @@ get_dbnames_list_to_restore(PGconn *conn,
 				if ((PQresultStatus(res) == PGRES_TUPLES_OK) && PQntuples(res))
 				{
 					skip_db_restore = true;
-					pg_log_info("database \"%s\" matches exclude pattern: \"%s\"", db_cell->str, pat_cell->val);
+					pg_log_info("database \"%s\" matches exclude pattern: \"%s\"", dbidname->str, pat_cell->val);
 				}
 
 				PQclear(res);
@@ -1001,8 +1012,8 @@ get_dbnames_list_to_restore(PGconn *conn,
 		 */
 		if (skip_db_restore)
 		{
-			pg_log_info("excluding database \"%s\"", db_cell->str);
-			db_cell->oid = InvalidOid;
+			pg_log_info("excluding database \"%s\"", dbidname->str);
+			dbidname->oid = InvalidOid;
 		}
 		else
 		{
@@ -1024,13 +1035,14 @@ get_dbnames_list_to_restore(PGconn *conn,
  * Returns, total number of database names in map.dat file.
  */
 static int
-get_dbname_oid_list_from_mfile(const char *dumpdirpath, SimpleOidStringList *dbname_oid_list)
+get_dbname_oid_list_from_mfile(const char *dumpdirpath, SimplePtrList *dbname_oid_list)
 {
+	StringInfoData linebuf;
 	FILE	   *pfile;
 	char		map_file_path[MAXPGPATH];
-	char		line[MAXPGPATH];
 	int			count = 0;
 
+
 	/*
 	 * If there is only global.dat file in dump, then return from here as
 	 * there is no database to restore.
@@ -1049,32 +1061,38 @@ get_dbname_oid_list_from_mfile(const char *dumpdirpath, SimpleOidStringList *dbn
 	if (pfile == NULL)
 		pg_fatal("could not open \"%s\": %m", map_file_path);
 
+	initStringInfo(&linebuf);
+
 	/* Append all the dbname/db_oid combinations to the list. */
-	while ((fgets(line, MAXPGPATH, pfile)) != NULL)
+	while (pg_get_line_buf(pfile, &linebuf))
 	{
 		Oid			db_oid = InvalidOid;
 		char		db_oid_str[MAXPGPATH + 1] = "";
 		char	   *dbname;
+		DbOidName  *dbidname;
+		int			namelen;
 
 		/* Extract dboid. */
-		sscanf(line, "%u", &db_oid);
-		sscanf(line, "%20s", db_oid_str);
+		sscanf(linebuf.data, "%u", &db_oid);
+		sscanf(linebuf.data, "%20s", db_oid_str);
 
 		/* dbname is the rest of the line */
-		dbname = line + strlen(db_oid_str) + 1;
+		dbname = linebuf.data + strlen(db_oid_str) + 1;
+		namelen = strlen(dbname);
 
-		/* Remove \n from dbname. */
-		dbname[strlen(dbname) - 1] = '\0';
+		/* Report error and exit if the file has any corrupted data. */
+		if (!OidIsValid(db_oid) || namelen <= 1)
+			pg_fatal("invalid entry in \"%s\" at line: %d", map_file_path,
+					 count + 1);
 
 		pg_log_info("found database \"%s\" (OID: %u) in \"%s\"",
 					dbname, db_oid, map_file_path);
 
-		/* Report error and exit if the file has any corrupted data. */
-		if (!OidIsValid(db_oid) || strlen(dbname) == 0)
-			pg_fatal("invalid entry in \"%s\" at line : %d", map_file_path,
-					 count + 1);
+		dbidname = pg_malloc(offsetof(DbOidName, str) + namelen + 1);
+		dbidname->oid = db_oid;
+		strlcpy(dbidname->str, dbname, namelen);
 
-		simple_oid_string_list_append(dbname_oid_list, db_oid, dbname);
+		simple_ptr_list_append(dbname_oid_list, dbidname);
 		count++;
 	}
 
@@ -1100,7 +1118,7 @@ restore_all_databases(PGconn *conn, const char *dumpdirpath,
 					  SimpleStringList db_exclude_patterns, RestoreOptions *opts,
 					  int numWorkers)
 {
-	SimpleOidStringList dbname_oid_list = {NULL, NULL};
+	SimplePtrList dbname_oid_list = {NULL, NULL};
 	int			num_db_restore = 0;
 	int			num_total_db;
 	int			n_errors_total;
@@ -1116,7 +1134,7 @@ restore_all_databases(PGconn *conn, const char *dumpdirpath,
 
 	num_total_db = get_dbname_oid_list_from_mfile(dumpdirpath, &dbname_oid_list);
 
-	/* If map.dat has no entry, return after processing global.dat */
+	/* If map.dat has no entries, return after processing global.dat */
 	if (dbname_oid_list.head == NULL)
 		return process_global_sql_commands(conn, dumpdirpath, opts->filename);
 
@@ -1168,16 +1186,17 @@ restore_all_databases(PGconn *conn, const char *dumpdirpath,
 	 * skipping names of exclude-database.  Now we can launch parallel workers
 	 * to restore these databases.
 	 */
-	for (SimpleOidStringListCell * db_cell = dbname_oid_list.head;
+	for (SimplePtrListCell *db_cell = dbname_oid_list.head;
 		 db_cell; db_cell = db_cell->next)
 	{
+		DbOidName  *dbidname = (DbOidName *) db_cell->ptr;
 		char		subdirpath[MAXPGPATH];
 		char		subdirdbpath[MAXPGPATH];
 		char		dbfilename[MAXPGPATH];
 		int			n_errors;
 
 		/* ignore dbs marked for skipping */
-		if (db_cell->oid == InvalidOid)
+		if (dbidname->oid == InvalidOid)
 			continue;
 
 		/*
@@ -1197,27 +1216,27 @@ restore_all_databases(PGconn *conn, const char *dumpdirpath,
 		 * {oid}.dmp file, use it. Otherwise try to use a directory called
 		 * {oid}
 		 */
-		snprintf(dbfilename, MAXPGPATH, "%u.tar", db_cell->oid);
+		snprintf(dbfilename, MAXPGPATH, "%u.tar", dbidname->oid);
 		if (file_exists_in_directory(subdirdbpath, dbfilename))
-			snprintf(subdirpath, MAXPGPATH, "%s/databases/%u.tar", dumpdirpath, db_cell->oid);
+			snprintf(subdirpath, MAXPGPATH, "%s/databases/%u.tar", dumpdirpath, dbidname->oid);
 		else
 		{
-			snprintf(dbfilename, MAXPGPATH, "%u.dmp", db_cell->oid);
+			snprintf(dbfilename, MAXPGPATH, "%u.dmp", dbidname->oid);
 
 			if (file_exists_in_directory(subdirdbpath, dbfilename))
-				snprintf(subdirpath, MAXPGPATH, "%s/databases/%u.dmp", dumpdirpath, db_cell->oid);
+				snprintf(subdirpath, MAXPGPATH, "%s/databases/%u.dmp", dumpdirpath, dbidname->oid);
 			else
-				snprintf(subdirpath, MAXPGPATH, "%s/databases/%u", dumpdirpath, db_cell->oid);
+				snprintf(subdirpath, MAXPGPATH, "%s/databases/%u", dumpdirpath, dbidname->oid);
 		}
 
-		pg_log_info("restoring database \"%s\"", db_cell->str);
+		pg_log_info("restoring database \"%s\"", dbidname->str);
 
 		/* If database is already created, then don't set createDB flag. */
 		if (opts->cparams.dbname)
 		{
 			PGconn	   *test_conn;
 
-			test_conn = ConnectDatabase(db_cell->str, NULL, opts->cparams.pghost,
+			test_conn = ConnectDatabase(dbidname->str, NULL, opts->cparams.pghost,
 										opts->cparams.pgport, opts->cparams.username, TRI_DEFAULT,
 										false, progname, NULL, NULL, NULL, NULL);
 			if (test_conn)
@@ -1226,7 +1245,7 @@ restore_all_databases(PGconn *conn, const char *dumpdirpath,
 
 				/* Use already created database for connection. */
 				opts->createDB = 0;
-				opts->cparams.dbname = db_cell->str;
+				opts->cparams.dbname = dbidname->str;
 			}
 			else
 			{
@@ -1251,7 +1270,7 @@ restore_all_databases(PGconn *conn, const char *dumpdirpath,
 		if (n_errors)
 		{
 			n_errors_total += n_errors;
-			pg_log_warning("errors ignored on database \"%s\" restore: %d", db_cell->str, n_errors);
+			pg_log_warning("errors ignored on database \"%s\" restore: %d", dbidname->str, n_errors);
 		}
 
 		count++;
@@ -1261,7 +1280,7 @@ restore_all_databases(PGconn *conn, const char *dumpdirpath,
 	pg_log_info("number of restored databases is %d", num_db_restore);
 
 	/* Free dbname and dboid list. */
-	simple_oid_string_list_destroy(&dbname_oid_list);
+	simple_ptr_list_destroy(&dbname_oid_list);
 
 	return n_errors_total;
 }
diff --git a/src/fe_utils/simple_list.c b/src/fe_utils/simple_list.c
index b0686e57c4a..483d5455594 100644
--- a/src/fe_utils/simple_list.c
+++ b/src/fe_utils/simple_list.c
@@ -192,44 +192,3 @@ simple_ptr_list_destroy(SimplePtrList *list)
 		cell = next;
 	}
 }
-
-/*
- * Add to an oid_string list
- */
-void
-simple_oid_string_list_append(SimpleOidStringList *list, Oid oid, const char *str)
-{
-	SimpleOidStringListCell *cell;
-
-	cell = (SimpleOidStringListCell *)
-		pg_malloc(offsetof(SimpleOidStringListCell, str) + strlen(str) + 1);
-
-	cell->next = NULL;
-	cell->oid = oid;
-	strcpy(cell->str, str);
-
-	if (list->tail)
-		list->tail->next = cell;
-	else
-		list->head = cell;
-	list->tail = cell;
-}
-
-/*
- * Destroy an oid_string list
- */
-void
-simple_oid_string_list_destroy(SimpleOidStringList *list)
-{
-	SimpleOidStringListCell *cell;
-
-	cell = list->head;
-	while (cell != NULL)
-	{
-		SimpleOidStringListCell *next;
-
-		next = cell->next;
-		pg_free(cell);
-		cell = next;
-	}
-}
diff --git a/src/include/fe_utils/simple_list.h b/src/include/fe_utils/simple_list.h
index a5373932555..3b8e38414ec 100644
--- a/src/include/fe_utils/simple_list.h
+++ b/src/include/fe_utils/simple_list.h
@@ -55,19 +55,6 @@ typedef struct SimplePtrList
 	SimplePtrListCell *tail;
 } SimplePtrList;
 
-typedef struct SimpleOidStringListCell
-{
-	struct SimpleOidStringListCell *next;
-	Oid			oid;
-	char		str[FLEXIBLE_ARRAY_MEMBER]; /* null-terminated string here */
-}			SimpleOidStringListCell;
-
-typedef struct SimpleOidStringList
-{
-	SimpleOidStringListCell *head;
-	SimpleOidStringListCell *tail;
-} SimpleOidStringList;
-
 extern void simple_oid_list_append(SimpleOidList *list, Oid val);
 extern bool simple_oid_list_member(SimpleOidList *list, Oid val);
 extern void simple_oid_list_destroy(SimpleOidList *list);
@@ -81,7 +68,4 @@ extern const char *simple_string_list_not_touched(SimpleStringList *list);
 extern void simple_ptr_list_append(SimplePtrList *list, void *ptr);
 extern void simple_ptr_list_destroy(SimplePtrList *list);
 
-extern void simple_oid_string_list_append(SimpleOidStringList *list, Oid oid, const char *str);
-extern void simple_oid_string_list_destroy(SimpleOidStringList *list);
-
 #endif							/* SIMPLE_LIST_H */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index d16bc208654..beb4a113170 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -615,6 +615,7 @@ DatumTupleFields
 DbInfo
 DbInfoArr
 DbLocaleInfo
+DbOidName
 DeClonePtrType
 DeadLockState
 DeallocateStmt
-- 
2.39.5


--2h4sxwwjii2o4jzh--