Subject: Re: pgsql: Non text modes for pg_dumpall, correspondingly change pg_restore

From: Andrew Dunstan <[email-masked]>
Date: Mon, 14 Apr 2025 12:28:15 -0400

On 2025-04-14 Mo 8:20 AM, Álvaro Herrera wrote:
> 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.)

Sure, we can do it that way.

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

seems reasonable

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

Yes, probably. I'll look into that if you like.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

---

From: Andrew Dunstan <[email-masked]>
Date: Mon, 14 Apr 2025 16:14:37 -0400

> Yes, probably. I'll look into that if you like.

something like this?

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index ff4bb320fc9..a600f7d9a3b 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -1053,26 +1053,32 @@ get_dbname_oid_list_from_mfile(const char *dumpdirpath, SimpleOidStringList *dbn
 	while ((fgets(line, MAXPGPATH, pfile)) != NULL)
 	{
 		Oid			db_oid = InvalidOid;
-		char		db_oid_str[MAXPGPATH + 1] = "";
 		char	   *dbname;
+		char       *p = line;
 
 		/* Extract dboid. */
-		sscanf(line, "%u", &db_oid);
-		sscanf(line, "%20s", db_oid_str);
+		while(isdigit(*p))
+			p++;
+		if (p > line && *p == ' ')
+		{
+			sscanf(line, "%u", &db_oid);
+			p++;
+		}
 
 		/* dbname is the rest of the line */
-		dbname = line + strlen(db_oid_str) + 1;
+		dbname = p;
 
 		/* Remove \n from dbname. */
 		dbname[strlen(dbname) - 1] = '\0';
 
-		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);
+		else
+			pg_log_info("found database \"%s\" (OID: %u) in \"%s\"",
+						dbname, db_oid, map_file_path);
+
 
 		simple_oid_string_list_append(dbname_oid_list, db_oid, dbname);
 		count++;

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Tue, 15 Apr 2025 04:25:42 +0530

> members of the proposed struct can be improved.)

Thanks Álvaro for the patch.

I took this patch and did some testing. Patch looks good to me. I was not
able to use "git am" in my setup due to CR's in patch but no warning in the
patch when I used "patch -p".

*One review comment:*
We are using FLEXIBLE_ARRAY_MEMBER for dbname. Can we use NAMEDATALEN? In
code, many places we are using this hard coded value of 64 size for names.

> > Yes, probably. I'll look into that if you like.

Thanks Andrew.

Your patch looks good to me. I took your patch and did 1-2 line adjustments
as per below. Please have a look.

> @@ -1053,15 +1053,19 @@ get_dbname_oid_list_from_mfile(const char
> *dumpdirpath, SimpleOidStringList *dbn
>                 sscanf(line, "%u", &db_oid);
> +               Assert(*p == ' ');
> +               dbname = ++p;

-- 
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com