pgsql: Non text modes for pg_dumpall, correspondingly change pg_restore
Non text modes for pg_dumpall, correspondingly change pg_restore
pg_dumpall acquires a new -F/--format option, with the same meanings as
pg_dump. The default is p, meaning plain text. For any other value, a
directory is created containing two files, globals.data and map.dat. The
first contains SQL for restoring the global data, and the second
contains a map from oids to database names. It will also contain a
subdirectory called databases, inside which it will create archives in
the specified format, named using the database oids.
In these casess the -f argument is required.
If pg_restore encounters a directory containing globals.dat, and no
toc.dat, it restores the global settings and then restores each
database.
pg_restore acquires two new options: -g/--globals-only which suppresses
restoration of any databases, and --exclude-database which inhibits
restoration of particualr database(s) in the same way the same option
works in pg_dumpall.
Author: Mahendra Singh Thalor <mahi6run@gmail.com>
Co-authored-by: Andrew Dunstan <andrew@dunslane.net>
Reviewed-by: jian he <jian.universality@gmail.com>
Reviewed-by: Srinath Reddy <srinath2133@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: /messages/by-id/cb103623-8ee6-4ba5-a2c9-f32e3a4933fa@dunslane.net
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/1495eff7bdb0779cc54ca04f3bd768f647240df2
Modified Files
--------------
doc/src/sgml/ref/pg_dumpall.sgml | 86 +++-
doc/src/sgml/ref/pg_restore.sgml | 66 ++-
src/bin/pg_dump/parallel.c | 10 +
src/bin/pg_dump/pg_backup.h | 2 +-
src/bin/pg_dump/pg_backup_archiver.c | 20 +-
src/bin/pg_dump/pg_backup_archiver.h | 1 +
src/bin/pg_dump/pg_backup_tar.c | 2 +-
src/bin/pg_dump/pg_dump.c | 2 +-
src/bin/pg_dump/pg_dumpall.c | 294 +++++++++++--
src/bin/pg_dump/pg_restore.c | 794 ++++++++++++++++++++++++++++++++++-
src/bin/pg_dump/t/001_basic.pl | 9 +
11 files changed, 1201 insertions(+), 85 deletions(-)
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/
Attachments:
0001-remove-unnecessary-oid_string-list-stuff.patchtext/x-diff; charset=utf-8Download
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
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
On 2025-04-14 Mo 12:28 PM, Andrew Dunstan wrote:
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.
something like this?
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachments:
pg_restore-cleanup.patchtext/x-patch; charset=UTF-8; name=pg_restore-cleanup.patchDownload
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++;
On Mon, 14 Apr 2025 at 21:39, Álvaro Herrera <alvherre@kurilemu.de> 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.)
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.
On Tue, 15 Apr 2025 at 01:44, Andrew Dunstan <andrew@dunslane.net> wrote:
On 2025-04-14 Mo 12:28 PM, Andrew Dunstan wrote:
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.
something like this?
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
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.
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c @@ -1053,15 +1053,19 @@ 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++; + + Assert(*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';
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
On 2025-04-14 Mo 6:55 PM, Mahendra Singh Thalor wrote:
--- a/src/bin/pg_dump/pg_restore.c +++ b/src/bin/pg_dump/pg_restore.c @@ -1053,15 +1053,19 @@ 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++; + + Assert(*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';
I don't think an Assert is the right thing here. It's for things that
should not be possible, and won't trigger anything in a non-assertion
build. This condition should cause a pg_fatal().
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
On 2025-Apr-15, Mahendra Singh Thalor wrote:
I took this patch and did some testing. Patch looks good to me.
Hello, thank you very much for looking.
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".
Hmm, don't blame me; rather I think your email rig is broken and
produces CRs for no reason (pretty normal on Windows I think). Here's
what hexdump shows for the attached file direct from the email:
000002a0 2d 2d 2d 20 61 2f 73 72 63 2f 62 69 6e 2f 70 67 |--- a/src/bin/pg|
000002b0 5f 64 75 6d 70 2f 70 67 5f 72 65 73 74 6f 72 65 |_dump/pg_restore|
000002c0 2e 63 0a 2b 2b 2b 20 62 2f 73 72 63 2f 62 69 6e |.c.+++ b/src/bin|
000002d0 2f 70 67 5f 64 75 6d 70 2f 70 67 5f 72 65 73 74 |/pg_dump/pg_rest|
000002e0 6f 72 65 2e 63 0a 40 40 20 2d 36 37 2c 31 30 20 |ore.c.@@ -67,10 |
000002f0 2b 36 37 2c 32 30 20 40 40 20 73 74 61 74 69 63 |+67,20 @@ static|
00000300 20 69 6e 74 09 70 72 6f 63 65 73 73 5f 67 6c 6f | int.process_glo|
00000310 62 61 6c 5f 73 71 6c 5f 63 6f 6d 6d 61 6e 64 73 |bal_sql_commands|
00000320 28 50 47 63 6f 6e 6e 20 2a 63 6f 6e 6e 2c 20 63 |(PGconn *conn, c|
00000330 6f 6e 73 74 20 63 68 61 72 20 2a 64 75 6d 70 64 |onst char *dumpd|
00000340 69 72 70 61 74 68 2c 0a 20 09 09 09 09 09 09 09 |irpath,. .......|
00000350 09 09 09 63 6f 6e 73 74 20 63 68 61 72 20 2a 6f |...const char *o|
00000360 75 74 66 69 6c 65 29 3b 0a 20 73 74 61 74 69 63 |utfile);. static|
In bytes 340ff you can see that the comma (2c) is followed by 0a ("line
feed" or newline) and then a bunch of tabs (09). There's no carriage
return (0d) anywhere.
Maybe you would be able to convince git not to complain by downloading
the file from the email[1]/messages/by-id/202504141220.343fmoxfsbj4@alvherre.pgsql directly onto it somehow, without letting
Windows mess things up. Maybe something like this on a command line
curl "/messages/by-id/attachment/175873/0001-remove-unnecessary-oid_string-list-stuff.patch" | git am -
assuming you can get curl to run on a command line at all.
[1]: /messages/by-id/202504141220.343fmoxfsbj4@alvherre.pgsql
*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.
Well, yes we can, but then we'd be allocating a bunch of null bytes for
no reason. The current allocation strategy, which comes from the
original commit not my code, is to allocate a struct with the OID and
just enough bytes to store the database name, which is known. I don't
see any reason to modify this tbh. The reason to use NAMEDATALEN in
various places is to have enough room to store whatever name the user
specifies, without having to know the length before the allocation
occurs.
FLEXIBLE_ARRAY_MEMBER is there to document exactly what's happening
here, and used thoroughly across the codebase.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"E pur si muove" (Galileo Galilei)
On Wed, 16 Apr 2025 at 01:52, Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2025-Apr-15, Mahendra Singh Thalor wrote:
I took this patch and did some testing. Patch looks good to me.
Hello, thank you very much for looking.
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".Hmm, don't blame me; rather I think your email rig is broken and
produces CRs for no reason (pretty normal on Windows I think). Here's
what hexdump shows for the attached file direct from the email:
Sorry for the noise. My mistake, I will fix my git setup.
Actually "git am" was working but "git apply" was failing. I think my
git setup is missing something.
I am downloading on a Mac and I am doing testing in a VM with Centos.
Below are the outputs of git commands.
[mst@localhost postgres]$ git apply
0001-remove-unnecessary-oid_string-list-stuff.patch
error: patch failed: src/bin/pg_dump/pg_restore.c:67
error: src/bin/pg_dump/pg_restore.c: patch does not apply
error: patch failed: src/fe_utils/simple_list.c:192
error: src/fe_utils/simple_list.c: patch does not apply
error: patch failed: src/include/fe_utils/simple_list.h:55
error: src/include/fe_utils/simple_list.h: patch does not apply
error: patch failed: src/tools/pgindent/typedefs.list:615
error: src/tools/pgindent/typedefs.list: patch does not apply
[mst@localhost postgres]$
[mst@localhost postgres]$ git am
0001-remove-unnecessary-oid_string-list-stuff.patch
Applying: remove unnecessary oid_string list stuff
[mst@localhost postgres]$
[mst@localhost postgres]$ git reset --hard HEAD~1
HEAD is now at 3b35f9a4c5e Fix an incorrect check in get_memoize_path
[mst@localhost postgres]$
[mst@localhost postgres]$ patch -p 1 <
0001-remove-unnecessary-oid_string-list-stuff.patch
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/bin/pg_dump/pg_restore.c
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/fe_utils/simple_list.c
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/include/fe_utils/simple_list.h
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/tools/pgindent/typedefs.list
000002a0 2d 2d 2d 20 61 2f 73 72 63 2f 62 69 6e 2f 70 67 |--- a/src/bin/pg|
000002b0 5f 64 75 6d 70 2f 70 67 5f 72 65 73 74 6f 72 65 |_dump/pg_restore|
000002c0 2e 63 0a 2b 2b 2b 20 62 2f 73 72 63 2f 62 69 6e |.c.+++ b/src/bin|
000002d0 2f 70 67 5f 64 75 6d 70 2f 70 67 5f 72 65 73 74 |/pg_dump/pg_rest|
000002e0 6f 72 65 2e 63 0a 40 40 20 2d 36 37 2c 31 30 20 |ore.c.@@ -67,10 |
000002f0 2b 36 37 2c 32 30 20 40 40 20 73 74 61 74 69 63 |+67,20 @@ static|
00000300 20 69 6e 74 09 70 72 6f 63 65 73 73 5f 67 6c 6f | int.process_glo|
00000310 62 61 6c 5f 73 71 6c 5f 63 6f 6d 6d 61 6e 64 73 |bal_sql_commands|
00000320 28 50 47 63 6f 6e 6e 20 2a 63 6f 6e 6e 2c 20 63 |(PGconn *conn, c|
00000330 6f 6e 73 74 20 63 68 61 72 20 2a 64 75 6d 70 64 |onst char *dumpd|
00000340 69 72 70 61 74 68 2c 0a 20 09 09 09 09 09 09 09 |irpath,. .......|
00000350 09 09 09 63 6f 6e 73 74 20 63 68 61 72 20 2a 6f |...const char *o|
00000360 75 74 66 69 6c 65 29 3b 0a 20 73 74 61 74 69 63 |utfile);. static|In bytes 340ff you can see that the comma (2c) is followed by 0a ("line
feed" or newline) and then a bunch of tabs (09). There's no carriage
return (0d) anywhere.Maybe you would be able to convince git not to complain by downloading
the file from the email[1] directly onto it somehow, without letting
Windows mess things up. Maybe something like this on a command line
curl "/messages/by-id/attachment/175873/0001-remove-unnecessary-oid_string-list-stuff.patch" | git am -
assuming you can get curl to run on a command line at all.[1] /messages/by-id/202504141220.343fmoxfsbj4@alvherre.pgsql
*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.Well, yes we can, but then we'd be allocating a bunch of null bytes for
no reason. The current allocation strategy, which comes from the
original commit not my code, is to allocate a struct with the OID and
just enough bytes to store the database name, which is known. I don't
see any reason to modify this tbh. The reason to use NAMEDATALEN in
various places is to have enough room to store whatever name the user
specifies, without having to know the length before the allocation
occurs.
Thanks for feedback. Yes, FLEXIBLE_ARRAY_MEMBER is added in the original commit.
In other code parts also, we can use FLEXIBLE_ARRAY_MEMBER instead
NAMEDATALEN. I will try if this is possible to do in other code parts.
FLEXIBLE_ARRAY_MEMBER is there to document exactly what's happening
here, and used thoroughly across the codebase.--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"E pur si muove" (Galileo Galilei)
-
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
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.)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.
Pushed your patch plus cleanup of the sscanf stuff.
Thanks!
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On 2025-Apr-16, Andrew Dunstan wrote:
Pushed your patch plus cleanup of the sscanf stuff.
Thank you!
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"El sabio habla porque tiene algo que decir;
el tonto, porque tiene que decir algo" (Platon).