public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andrew Dunstan <[email protected]>
To: Mahendra Singh Thalor <[email protected]>
Cc: jian he <[email protected]>
Cc: Álvaro Herrera <[email protected]>
Cc: Srinath Reddy <[email protected]>
Cc: [email protected]
Subject: Re: Non-text mode for pg_dumpall
Date: Thu, 27 Mar 2025 17:15:57 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAKYtNAoebXn-CC_cgv=-DxL1s7hPvx-1UW7CEfsiJZ8fbD6pjg@mail.gmail.com>
References: <CAKYtNAr7ToCBSLO2Mm6NGFTZvbdTAFNgjv_YoRqJJghC3n1MiQ@mail.gmail.com>
<[email protected]>
<CACJufxG44zd+KV4B3_0d+Lf2fwnDDptX-DQRqn-o9ukmXEWeNw@mail.gmail.com>
<[email protected]>
<CAKYtNAoebXn-CC_cgv=-DxL1s7hPvx-1UW7CEfsiJZ8fbD6pjg@mail.gmail.com>
On 2025-03-19 We 2:41 AM, Mahendra Singh Thalor wrote:
> On Wed, 12 Mar 2025 at 21:18, Andrew Dunstan <[email protected]> wrote:
>>
>> On 2025-03-12 We 3:03 AM, jian he wrote:
>>> On Wed, Mar 12, 2025 at 1:06 AM Álvaro Herrera <[email protected]> wrote:
>>>> Hello,
>>>>
>>>> On 2025-Mar-11, Mahendra Singh Thalor wrote:
>>>>
>>>>> In map.dat file, I tried to fix this issue by adding number of characters
>>>>> in dbname but as per code comments, as of now, we are not supporting \n\r
>>>>> in dbnames so i removed handling.
>>>>> I will do some more study to fix this issue.
>>>> Yeah, I think this is saying that you should not consider the contents
>>>> of map.dat as a shell string. After all, you're not going to _execute_
>>>> that file via the shell.
>>>>
>>>> Maybe for map.dat you need to escape such characters somehow, so that
>>>> they don't appear as literal newlines/carriage returns.
>>>>
>>> I am confused.
>>> currently pg_dumpall plain format will abort when encountering dbname
>>> containing newline.
>>> the left dumped plain file does not contain all the cluster databases data.
>>>
>>>
>>> if pg_dumpall non-text format aborts earlier,
>>> it's aligned with pg_dumpall plain format?
>>> it's also an improvement since aborts earlier, nothing will be dumped?
>>>
>>>
>>> am i missing something?
>>>
>>>
>> I think we should fix that.
>>
>> But for the current proposal, Álvaro and I were talking this morning,
>> and we thought the simplest thing here would be to have the one line
>> format and escape NL/CRs in the database name.
>>
>>
>> cheers
>>
> Okay. As per discussions, we will keep one line entry for each
> database into map.file.
>
> Thanks all for feedback and review.
>
> Here, I am attaching updated patches for review and testing. These
> patches can be applied on commit a6524105d20b.
I'm working through this patch set with a view to committing it.
Attached is some cleanup which is where I got to today, although there
is more to do. One thing I am wondering is why not put the
SimpleDatabaseOidList stuff in fe_utils/simle_list.{c,h} ? That's where
all the similar stuff belongs, and it feels strange to have this inline
in pg_restore.c. (I also don't like the name much - SimpleOidStringList
or maybe SimpleOidPlusStringList might be better).
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index a3dcc585ace..6aab1bfe831 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -434,13 +434,13 @@ main(int argc, char *argv[])
archDumpFormat = parseDumpFormat(formatName);
/*
- * If non-plain format is specified then we must provide the
- * file name to create one main directory.
+ * If a non-plain format is specified, a file name is also required as the
+ * path to the main directory.
*/
if (archDumpFormat != archNull &&
(!filename || strcmp(filename, "") == 0))
{
- pg_log_error("options -F/--format=d|c|t requires option -f/--file with non-empty string");
+ pg_log_error("option -F/--format=d|c|t requires option -f/--file");
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
exit_nicely(1);
}
@@ -513,14 +513,14 @@ main(int argc, char *argv[])
*/
if (archDumpFormat != archNull)
{
- char toc_path[MAXPGPATH];
+ char global_path[MAXPGPATH];
/* Create new directory or accept the empty existing directory. */
create_or_open_dir(filename);
- snprintf(toc_path, MAXPGPATH, "%s/global.dat", filename);
+ snprintf(global_path, MAXPGPATH, "%s/global.dat", filename);
- OPF = fopen(toc_path, PG_BINARY_W);
+ OPF = fopen(global_path, PG_BINARY_W);
if (!OPF)
pg_fatal("could not open global.dat file: %s", strerror(errno));
}
@@ -1680,7 +1680,7 @@ dumpDatabases(PGconn *conn, ArchiveFormat archDumpFormat)
}
/*
- * If this is non-plain dump format, then append dboid and dbname to
+ * If this is not a plain format dump, then append dboid and dbname to
* the map.dat file.
*/
if (archDumpFormat != archNull)
@@ -1688,7 +1688,7 @@ dumpDatabases(PGconn *conn, ArchiveFormat archDumpFormat)
snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\"", db_subdir, oid);
/* Put one line entry for dboid and dbname in map file. */
- fprintf(map_file, "%s %s\n", oid, pg_strdup(dbname));
+ fprintf(map_file, "%s %s\n", oid, dbname);
}
pg_log_info("dumping database \"%s\"", dbname);
@@ -1734,17 +1734,17 @@ dumpDatabases(PGconn *conn, ArchiveFormat archDumpFormat)
if (filename)
{
- char toc_path[MAXPGPATH];
+ char global_path[MAXPGPATH];
if (archDumpFormat != archNull)
- snprintf(toc_path, MAXPGPATH, "%s/global.dat", filename);
+ snprintf(global_path, MAXPGPATH, "%s/global.dat", filename);
else
- snprintf(toc_path, MAXPGPATH, "%s", filename);
+ snprintf(global_path, MAXPGPATH, "%s", filename);
- OPF = fopen(toc_path, PG_BINARY_A);
+ OPF = fopen(global_path, PG_BINARY_A);
if (!OPF)
pg_fatal("could not re-open the output file \"%s\": %m",
- toc_path);
+ global_path);
}
}
@@ -1772,7 +1772,7 @@ runPgDump(const char *dbname, const char *create_opts, char *dbfile,
initPQExpBuffer(&cmd);
/*
- * If this is non-plain format dump, then append file name and dump
+ * If this is not a plain format dump, then append file name and dump
* format to the pg_dump command to get archive dump.
*/
if (archDumpFormat != archNull)
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index e4093427e2f..91602a2e37b 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -46,8 +46,6 @@
#include <termios.h>
#endif
-#include "common/connect.h"
-#include "compress_io.h"
#include "common/string.h"
#include "connectdb.h"
#include "fe_utils/option_utils.h"
@@ -55,7 +53,6 @@
#include "filter.h"
#include "getopt_long.h"
#include "parallel.h"
-#include "pg_backup_archiver.h"
#include "pg_backup_utils.h"
typedef struct SimpleDatabaseOidListCell
@@ -73,10 +70,10 @@ typedef struct SimpleDatabaseOidList
static void usage(const char *progname);
static void read_restore_filters(const char *filename, RestoreOptions *opts);
-static bool IsFileExistsInDirectory(const char *dir, const char *filename);
+static bool file_exists_in_directory(const char *dir, const char *filename);
static int restoreOneDatabase(const char *inputFileSpec, RestoreOptions *opts,
int numWorkers, bool append_data, int num);
-static int ReadOneStatement(StringInfo inBuf, FILE *pfile);
+static int read_one_statement(StringInfo inBuf, FILE *pfile);
static int restoreAllDatabases(PGconn *conn, const char *dumpdirpath,
SimpleStringList db_exclude_patterns, RestoreOptions *opts, int numWorkers);
static int process_global_sql_commands(PGconn *conn, const char *dumpdirpath,
@@ -89,7 +86,6 @@ static int get_dbname_oid_list_from_mfile(const char *dumpdirpath,
SimpleDatabaseOidList *dbname_oid_list);
static void simple_db_oid_list_append(SimpleDatabaseOidList *list, Oid db_oid,
const char *dbname);
-static void simple_string_full_list_delete(SimpleStringList *list);
static void simple_db_oid_full_list_delete(SimpleDatabaseOidList *list);
static void simple_db_oid_list_delete(SimpleDatabaseOidList *list,
SimpleDatabaseOidListCell *cell,
@@ -521,8 +517,8 @@ main(int argc, char **argv)
* databases from map.dat(if exist) file list and skip restoring for
* --exclude-database patterns.
*/
- if (inputFileSpec != NULL && !IsFileExistsInDirectory(inputFileSpec, "toc.dat") &&
- IsFileExistsInDirectory(inputFileSpec, "global.dat"))
+ if (inputFileSpec != NULL && !file_exists_in_directory(inputFileSpec, "toc.dat") &&
+ file_exists_in_directory(inputFileSpec, "global.dat"))
{
PGconn *conn = NULL; /* Connection to restore global sql commands. */
@@ -578,7 +574,7 @@ main(int argc, char **argv)
}
/* Free db pattern list. */
- simple_string_full_list_delete(&db_exclude_patterns);
+ simple_string_list_destroy(&db_exclude_patterns);
}
else /* process if global.dat file does not exist. */
{
@@ -847,12 +843,12 @@ read_restore_filters(const char *filename, RestoreOptions *opts)
}
/*
- * IsFileExistsInDirectory
+ * file_exists_in_directory
*
* Returns true if file exist in current directory.
*/
static bool
-IsFileExistsInDirectory(const char *dir, const char *filename)
+file_exists_in_directory(const char *dir, const char *filename)
{
struct stat st;
char buf[MAXPGPATH];
@@ -864,7 +860,7 @@ IsFileExistsInDirectory(const char *dir, const char *filename)
}
/*
- * ReadOneStatement
+ * read_one_statement
*
* This will start reading from passed file pointer using fgetc and read till
* semicolon(sql statement terminator for global.dat file)
@@ -873,7 +869,7 @@ IsFileExistsInDirectory(const char *dir, const char *filename)
*/
static int
-ReadOneStatement(StringInfo inBuf, FILE *pfile)
+read_one_statement(StringInfo inBuf, FILE *pfile)
{
int c; /* character read from getc() */
int m;
@@ -1064,7 +1060,7 @@ get_dbname_oid_list_from_mfile(const char *dumpdirpath, SimpleDatabaseOidList *d
* If there is only global.dat file in dump, then return from here as there
* is no database to restore.
*/
- if (!IsFileExistsInDirectory(pg_strdup(dumpdirpath), "map.dat"))
+ if (!file_exists_in_directory(dumpdirpath, "map.dat"))
{
pg_log_info("databases restoring is skipped as map.dat file is not present in \"%s\"", dumpdirpath);
return 0;
@@ -1281,7 +1277,7 @@ process_global_sql_commands(PGconn *conn, const char *dumpdirpath, const char *o
initStringInfo(&sqlstatement);
/* Process file till EOF and execute sql statements. */
- while (ReadOneStatement(&sqlstatement, pfile) != EOF)
+ while (read_one_statement(&sqlstatement, pfile) != EOF)
{
pg_log_info("executing query: %s", sqlstatement.data);
result = PQexec(conn, sqlstatement.data);
@@ -1393,28 +1389,6 @@ simple_db_oid_full_list_delete(SimpleDatabaseOidList *list)
list->tail = NULL;
}
-/*
- * simple_string_full_list_delete
- *
- * delete all cell from string list.
- */
-static void
-simple_string_full_list_delete(SimpleStringList *list)
-{
- SimpleStringListCell *cell = list->head;
- SimpleStringListCell *cellnext = NULL;
-
- while (cell)
- {
- cellnext = cell->next;
- pfree(cell);
- cell = cellnext;
- }
-
- list->head = NULL;
- list->tail = NULL;
-}
-
/*
* simple_db_oid_list_delete
*
Attachments:
[text/plain] dumpall_cleanup.patch-noci (8.0K, 2-dumpall_cleanup.patch-noci)
download | inline diff:
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index a3dcc585ace..6aab1bfe831 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -434,13 +434,13 @@ main(int argc, char *argv[])
archDumpFormat = parseDumpFormat(formatName);
/*
- * If non-plain format is specified then we must provide the
- * file name to create one main directory.
+ * If a non-plain format is specified, a file name is also required as the
+ * path to the main directory.
*/
if (archDumpFormat != archNull &&
(!filename || strcmp(filename, "") == 0))
{
- pg_log_error("options -F/--format=d|c|t requires option -f/--file with non-empty string");
+ pg_log_error("option -F/--format=d|c|t requires option -f/--file");
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
exit_nicely(1);
}
@@ -513,14 +513,14 @@ main(int argc, char *argv[])
*/
if (archDumpFormat != archNull)
{
- char toc_path[MAXPGPATH];
+ char global_path[MAXPGPATH];
/* Create new directory or accept the empty existing directory. */
create_or_open_dir(filename);
- snprintf(toc_path, MAXPGPATH, "%s/global.dat", filename);
+ snprintf(global_path, MAXPGPATH, "%s/global.dat", filename);
- OPF = fopen(toc_path, PG_BINARY_W);
+ OPF = fopen(global_path, PG_BINARY_W);
if (!OPF)
pg_fatal("could not open global.dat file: %s", strerror(errno));
}
@@ -1680,7 +1680,7 @@ dumpDatabases(PGconn *conn, ArchiveFormat archDumpFormat)
}
/*
- * If this is non-plain dump format, then append dboid and dbname to
+ * If this is not a plain format dump, then append dboid and dbname to
* the map.dat file.
*/
if (archDumpFormat != archNull)
@@ -1688,7 +1688,7 @@ dumpDatabases(PGconn *conn, ArchiveFormat archDumpFormat)
snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\"", db_subdir, oid);
/* Put one line entry for dboid and dbname in map file. */
- fprintf(map_file, "%s %s\n", oid, pg_strdup(dbname));
+ fprintf(map_file, "%s %s\n", oid, dbname);
}
pg_log_info("dumping database \"%s\"", dbname);
@@ -1734,17 +1734,17 @@ dumpDatabases(PGconn *conn, ArchiveFormat archDumpFormat)
if (filename)
{
- char toc_path[MAXPGPATH];
+ char global_path[MAXPGPATH];
if (archDumpFormat != archNull)
- snprintf(toc_path, MAXPGPATH, "%s/global.dat", filename);
+ snprintf(global_path, MAXPGPATH, "%s/global.dat", filename);
else
- snprintf(toc_path, MAXPGPATH, "%s", filename);
+ snprintf(global_path, MAXPGPATH, "%s", filename);
- OPF = fopen(toc_path, PG_BINARY_A);
+ OPF = fopen(global_path, PG_BINARY_A);
if (!OPF)
pg_fatal("could not re-open the output file \"%s\": %m",
- toc_path);
+ global_path);
}
}
@@ -1772,7 +1772,7 @@ runPgDump(const char *dbname, const char *create_opts, char *dbfile,
initPQExpBuffer(&cmd);
/*
- * If this is non-plain format dump, then append file name and dump
+ * If this is not a plain format dump, then append file name and dump
* format to the pg_dump command to get archive dump.
*/
if (archDumpFormat != archNull)
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index e4093427e2f..91602a2e37b 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -46,8 +46,6 @@
#include <termios.h>
#endif
-#include "common/connect.h"
-#include "compress_io.h"
#include "common/string.h"
#include "connectdb.h"
#include "fe_utils/option_utils.h"
@@ -55,7 +53,6 @@
#include "filter.h"
#include "getopt_long.h"
#include "parallel.h"
-#include "pg_backup_archiver.h"
#include "pg_backup_utils.h"
typedef struct SimpleDatabaseOidListCell
@@ -73,10 +70,10 @@ typedef struct SimpleDatabaseOidList
static void usage(const char *progname);
static void read_restore_filters(const char *filename, RestoreOptions *opts);
-static bool IsFileExistsInDirectory(const char *dir, const char *filename);
+static bool file_exists_in_directory(const char *dir, const char *filename);
static int restoreOneDatabase(const char *inputFileSpec, RestoreOptions *opts,
int numWorkers, bool append_data, int num);
-static int ReadOneStatement(StringInfo inBuf, FILE *pfile);
+static int read_one_statement(StringInfo inBuf, FILE *pfile);
static int restoreAllDatabases(PGconn *conn, const char *dumpdirpath,
SimpleStringList db_exclude_patterns, RestoreOptions *opts, int numWorkers);
static int process_global_sql_commands(PGconn *conn, const char *dumpdirpath,
@@ -89,7 +86,6 @@ static int get_dbname_oid_list_from_mfile(const char *dumpdirpath,
SimpleDatabaseOidList *dbname_oid_list);
static void simple_db_oid_list_append(SimpleDatabaseOidList *list, Oid db_oid,
const char *dbname);
-static void simple_string_full_list_delete(SimpleStringList *list);
static void simple_db_oid_full_list_delete(SimpleDatabaseOidList *list);
static void simple_db_oid_list_delete(SimpleDatabaseOidList *list,
SimpleDatabaseOidListCell *cell,
@@ -521,8 +517,8 @@ main(int argc, char **argv)
* databases from map.dat(if exist) file list and skip restoring for
* --exclude-database patterns.
*/
- if (inputFileSpec != NULL && !IsFileExistsInDirectory(inputFileSpec, "toc.dat") &&
- IsFileExistsInDirectory(inputFileSpec, "global.dat"))
+ if (inputFileSpec != NULL && !file_exists_in_directory(inputFileSpec, "toc.dat") &&
+ file_exists_in_directory(inputFileSpec, "global.dat"))
{
PGconn *conn = NULL; /* Connection to restore global sql commands. */
@@ -578,7 +574,7 @@ main(int argc, char **argv)
}
/* Free db pattern list. */
- simple_string_full_list_delete(&db_exclude_patterns);
+ simple_string_list_destroy(&db_exclude_patterns);
}
else /* process if global.dat file does not exist. */
{
@@ -847,12 +843,12 @@ read_restore_filters(const char *filename, RestoreOptions *opts)
}
/*
- * IsFileExistsInDirectory
+ * file_exists_in_directory
*
* Returns true if file exist in current directory.
*/
static bool
-IsFileExistsInDirectory(const char *dir, const char *filename)
+file_exists_in_directory(const char *dir, const char *filename)
{
struct stat st;
char buf[MAXPGPATH];
@@ -864,7 +860,7 @@ IsFileExistsInDirectory(const char *dir, const char *filename)
}
/*
- * ReadOneStatement
+ * read_one_statement
*
* This will start reading from passed file pointer using fgetc and read till
* semicolon(sql statement terminator for global.dat file)
@@ -873,7 +869,7 @@ IsFileExistsInDirectory(const char *dir, const char *filename)
*/
static int
-ReadOneStatement(StringInfo inBuf, FILE *pfile)
+read_one_statement(StringInfo inBuf, FILE *pfile)
{
int c; /* character read from getc() */
int m;
@@ -1064,7 +1060,7 @@ get_dbname_oid_list_from_mfile(const char *dumpdirpath, SimpleDatabaseOidList *d
* If there is only global.dat file in dump, then return from here as there
* is no database to restore.
*/
- if (!IsFileExistsInDirectory(pg_strdup(dumpdirpath), "map.dat"))
+ if (!file_exists_in_directory(dumpdirpath, "map.dat"))
{
pg_log_info("databases restoring is skipped as map.dat file is not present in \"%s\"", dumpdirpath);
return 0;
@@ -1281,7 +1277,7 @@ process_global_sql_commands(PGconn *conn, const char *dumpdirpath, const char *o
initStringInfo(&sqlstatement);
/* Process file till EOF and execute sql statements. */
- while (ReadOneStatement(&sqlstatement, pfile) != EOF)
+ while (read_one_statement(&sqlstatement, pfile) != EOF)
{
pg_log_info("executing query: %s", sqlstatement.data);
result = PQexec(conn, sqlstatement.data);
@@ -1393,28 +1389,6 @@ simple_db_oid_full_list_delete(SimpleDatabaseOidList *list)
list->tail = NULL;
}
-/*
- * simple_string_full_list_delete
- *
- * delete all cell from string list.
- */
-static void
-simple_string_full_list_delete(SimpleStringList *list)
-{
- SimpleStringListCell *cell = list->head;
- SimpleStringListCell *cellnext = NULL;
-
- while (cell)
- {
- cellnext = cell->next;
- pfree(cell);
- cell = cellnext;
- }
-
- list->head = NULL;
- list->tail = NULL;
-}
-
/*
* simple_db_oid_list_delete
*
view thread (99+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Non-text mode for pg_dumpall
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox