On 2025-03-19 We 2:41 AM, Mahendra Singh Thalor wrote:
On Wed, 12 Mar 2025 at 21:18, Andrew Dunstan <and...@dunslane.net> wrote:
On 2025-03-12 We 3:03 AM, jian he wrote:
On Wed, Mar 12, 2025 at 1:06 AM Álvaro Herrera <alvhe...@alvh.no-ip.org> 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
*
