On Tue, Jan 11, 2022 at 10:08:13PM -0600, Justin Pryzby wrote: > I asked about that before. Right now, it'll exit(1) when mkdir fails. > > I had written a patch to allow "." by skipping mkdir (or allowing it to fail > if > errno == EEXIST), but it seems like an awfully bad idea to try to make that > work with rmtree().
So, I have been poking at this patch, and found myself doing a couple of modifications: - Renaming of the option from --logdir to --outputdir, as this does not include only logs. That matches also better with default value assigned in previous patches, aka pg_upgrade_output.d. - Convert the output directory to an absolute path when the various directories are created, and use that for the whole run. pg_upgrade is unlikely going to chdir(), but I don't really see why we should just not use an absolute path all the time, set from the start. - Add some sanity check about the path used, aka no parent reference allowed and the output path should not be a direct parent of the current working directory. - Rather than assuming that "log/" and "dump/" are hardcoded in various places, save more paths into log_opts. I have noticed a couple of incorrect things in the docs, and some other things. It is a bit late here, so I may have missed a couple of things but I'll look at this stuff once again in a couple of days. So, what do you think? -- Michael
From ff66604850b6455ae1b5134199e7bc5f7e61fed1 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 19 Jan 2022 16:56:08 +0900
Subject: [PATCH v2] pg_upgrade: write log files and dumps in subdir..
The subdir must not exist when pg_upgrade is started.
This avoids appending to logfiles which have pre-existing errors in them.
And allows more easily cleaning up after pg_upgrade.
---
src/bin/pg_upgrade/.gitignore | 2 -
src/bin/pg_upgrade/Makefile | 4 +-
src/bin/pg_upgrade/check.c | 12 ++--
src/bin/pg_upgrade/dump.c | 6 +-
src/bin/pg_upgrade/exec.c | 5 +-
src/bin/pg_upgrade/function.c | 3 +-
src/bin/pg_upgrade/option.c | 29 +++------
src/bin/pg_upgrade/pg_upgrade.c | 105 ++++++++++++++++++++++++--------
src/bin/pg_upgrade/pg_upgrade.h | 8 ++-
src/bin/pg_upgrade/server.c | 6 +-
doc/src/sgml/ref/pgupgrade.sgml | 17 +++++-
11 files changed, 130 insertions(+), 67 deletions(-)
diff --git a/src/bin/pg_upgrade/.gitignore b/src/bin/pg_upgrade/.gitignore
index 2d3bfeaa50..939e50db6c 100644
--- a/src/bin/pg_upgrade/.gitignore
+++ b/src/bin/pg_upgrade/.gitignore
@@ -1,9 +1,7 @@
/pg_upgrade
# Generated by test suite
-/pg_upgrade_internal.log
/delete_old_cluster.sh
/delete_old_cluster.bat
/reindex_hash.sql
-/loadable_libraries.txt
/log/
/tmp_check/
diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile
index 44d06be5a6..e833a1c5f0 100644
--- a/src/bin/pg_upgrade/Makefile
+++ b/src/bin/pg_upgrade/Makefile
@@ -45,9 +45,7 @@ uninstall:
clean distclean maintainer-clean:
rm -f pg_upgrade$(X) $(OBJS)
rm -rf delete_old_cluster.sh log/ tmp_check/ \
- loadable_libraries.txt reindex_hash.sql \
- pg_upgrade_dump_globals.sql \
- pg_upgrade_dump_*.custom pg_upgrade_*.log
+ reindex_hash.sql pg_upgrade_output.d/
# When $(MAKE) is present, make automatically infers that this is a
# recursive make. which is not actually what we want here, as that
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 3d218c2ad2..019bcb6c7b 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -783,7 +783,8 @@ check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster)
return;
}
- snprintf(output_path, sizeof(output_path),
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
"contrib_isn_and_int8_pass_by_value.txt");
for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
@@ -860,7 +861,8 @@ check_for_user_defined_postfix_ops(ClusterInfo *cluster)
prep_status("Checking for user-defined postfix operators");
- snprintf(output_path, sizeof(output_path),
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
"postfix_ops.txt");
/* Find any user defined postfix operators */
@@ -959,7 +961,8 @@ check_for_tables_with_oids(ClusterInfo *cluster)
prep_status("Checking for tables WITH OIDS");
- snprintf(output_path, sizeof(output_path),
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
"tables_with_oids.txt");
/* Find any tables declared WITH OIDS */
@@ -1214,7 +1217,8 @@ check_for_user_defined_encoding_conversions(ClusterInfo *cluster)
prep_status("Checking for user-defined encoding conversions");
- snprintf(output_path, sizeof(output_path),
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
"encoding_conversions.txt");
/* Find any user defined encoding conversions */
diff --git a/src/bin/pg_upgrade/dump.c b/src/bin/pg_upgrade/dump.c
index 953873fa01..b69b4f9569 100644
--- a/src/bin/pg_upgrade/dump.c
+++ b/src/bin/pg_upgrade/dump.c
@@ -22,9 +22,10 @@ generate_old_dump(void)
/* run new pg_dumpall binary for globals */
exec_prog(UTILITY_LOG_FILE, NULL, true, true,
"\"%s/pg_dumpall\" %s --globals-only --quote-all-identifiers "
- "--binary-upgrade %s -f %s",
+ "--binary-upgrade %s -f \"%s/%s\"",
new_cluster.bindir, cluster_conn_opts(&old_cluster),
log_opts.verbose ? "--verbose" : "",
+ log_opts.dumpdir,
GLOBALS_DUMP_FILE);
check_ok();
@@ -52,9 +53,10 @@ generate_old_dump(void)
parallel_exec_prog(log_file_name, NULL,
"\"%s/pg_dump\" %s --schema-only --quote-all-identifiers "
- "--binary-upgrade --format=custom %s --file=\"%s\" %s",
+ "--binary-upgrade --format=custom %s --file=\"%s/%s\" %s",
new_cluster.bindir, cluster_conn_opts(&old_cluster),
log_opts.verbose ? "--verbose" : "",
+ log_opts.dumpdir,
sql_file_name, escaped_connstr.data);
termPQExpBuffer(&escaped_connstr);
diff --git a/src/bin/pg_upgrade/exec.c b/src/bin/pg_upgrade/exec.c
index c0cbb1bec7..fadeea12ca 100644
--- a/src/bin/pg_upgrade/exec.c
+++ b/src/bin/pg_upgrade/exec.c
@@ -78,11 +78,12 @@ get_bin_version(ClusterInfo *cluster)
* The code requires it be called first from the primary thread on Windows.
*/
bool
-exec_prog(const char *log_file, const char *opt_log_file,
+exec_prog(const char *log_filename, const char *opt_log_file,
bool report_error, bool exit_on_error, const char *fmt,...)
{
int result = 0;
int written;
+ char log_file[MAXPGPATH];
#define MAXCMDLEN (2 * MAXPGPATH)
char cmd[MAXCMDLEN];
@@ -97,6 +98,8 @@ exec_prog(const char *log_file, const char *opt_log_file,
mainThreadId = GetCurrentThreadId();
#endif
+ snprintf(log_file, MAXPGPATH, "%s/%s", log_opts.logdir, log_filename);
+
written = 0;
va_start(ap, fmt);
written += vsnprintf(cmd + written, MAXCMDLEN - written, fmt, ap);
diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index e73a309070..acc97fafef 100644
--- a/src/bin/pg_upgrade/function.c
+++ b/src/bin/pg_upgrade/function.c
@@ -128,7 +128,8 @@ check_loadable_libraries(void)
prep_status("Checking for presence of required libraries");
- snprintf(output_path, sizeof(output_path), "loadable_libraries.txt");
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir, "loadable_libraries.txt");
/*
* Now we want to sort the library names into order. This avoids multiple
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index c894fdd685..54dff9691e 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -9,7 +9,6 @@
#include "postgres_fe.h"
-#include <time.h>
#ifdef WIN32
#include <io.h>
#endif
@@ -57,16 +56,15 @@ parseCommandLine(int argc, char *argv[])
{"socketdir", required_argument, NULL, 's'},
{"verbose", no_argument, NULL, 'v'},
{"clone", no_argument, NULL, 1},
+ {"outputdir", required_argument, NULL, 2},
{NULL, 0, NULL, 0}
};
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
int os_user_effective_id;
- FILE *fp;
- char **filename;
- time_t run_time = time(NULL);
+ log_opts.basedir = DEFAULT_OUTPUTDIR;
user_opts.do_sync = true;
user_opts.transfer_mode = TRANSFER_MODE_COPY;
@@ -198,6 +196,10 @@ parseCommandLine(int argc, char *argv[])
user_opts.transfer_mode = TRANSFER_MODE_CLONE;
break;
+ case 2:
+ log_opts.basedir = pg_strdup(optarg);
+ break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
os_info.progname);
@@ -208,27 +210,9 @@ parseCommandLine(int argc, char *argv[])
if (optind < argc)
pg_fatal("too many command-line arguments (first is \"%s\")\n", argv[optind]);
- if ((log_opts.internal = fopen_priv(INTERNAL_LOG_FILE, "a")) == NULL)
- pg_fatal("could not open log file \"%s\": %m\n", INTERNAL_LOG_FILE);
-
if (log_opts.verbose)
pg_log(PG_REPORT, "Running in verbose mode\n");
- /* label start of upgrade in logfiles */
- for (filename = output_files; *filename != NULL; filename++)
- {
- if ((fp = fopen_priv(*filename, "a")) == NULL)
- pg_fatal("could not write to log file \"%s\": %m\n", *filename);
-
- /* Start with newline because we might be appending to a file. */
- fprintf(fp, "\n"
- "-----------------------------------------------------------------\n"
- " pg_upgrade run on %s"
- "-----------------------------------------------------------------\n\n",
- ctime(&run_time));
- fclose(fp);
- }
-
/* Turn off read-only mode; add prefix to PGOPTIONS? */
if (getenv("PGOPTIONS"))
{
@@ -303,6 +287,7 @@ usage(void)
printf(_(" -v, --verbose enable verbose internal logging\n"));
printf(_(" -V, --version display version information, then exit\n"));
printf(_(" --clone clone instead of copying files to new cluster\n"));
+ printf(_(" --outputdir=DIR directory to store internal files (logs, etc.)\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\n"
"Before running pg_upgrade you must:\n"
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 07defacd67..52858f9dba 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -38,6 +38,8 @@
#include "postgres_fe.h"
+#include <time.h>
+
#ifdef HAVE_LANGINFO_H
#include <langinfo.h>
#endif
@@ -54,6 +56,7 @@ static void prepare_new_globals(void);
static void create_new_objects(void);
static void copy_xact_xlog_xid(void);
static void set_frozenxids(bool minmxid_only);
+static void make_outputdirs(void);
static void setup(char *argv0, bool *live_check);
static void cleanup(void);
@@ -89,6 +92,8 @@ main(int argc, char **argv)
get_restricted_token();
+ make_outputdirs();
+
adjust_data_dir(&old_cluster);
adjust_data_dir(&new_cluster);
@@ -197,6 +202,72 @@ main(int argc, char **argv)
return 0;
}
+/*
+ * Create and assign proper permissions to the set of output directories
+ * used to store any data generated internally, filling in log_opts in
+ * the process.
+ */
+static void
+make_outputdirs(void)
+{
+ FILE *fp;
+ char **filename;
+ time_t run_time = time(NULL);
+ char filename_path[MAXPGPATH];
+ char *outputpath;
+ char cwd[MAXPGPATH];
+
+ /*
+ * Check that the output directory is a sane location. It should not
+ * include any references to the parent, and if a relative path, this had
+ * better not be a direct parent.
+ */
+ outputpath = make_absolute_path(log_opts.basedir);
+ if (path_contains_parent_reference(outputpath))
+ pg_fatal("reference to parent directory not allowed\n");
+ if (!getcwd(cwd, MAXPGPATH))
+ pg_fatal("could not determine current directory\n");
+ if (path_is_prefix_of_path(outputpath, cwd))
+ pg_fatal("output directory cannot be below current working directory\n");
+
+ /* save the sanitized and absolute path, and just use it */
+ log_opts.basedir = outputpath;
+
+ log_opts.dumpdir = (char *) pg_malloc(MAXPGPATH);
+ snprintf(log_opts.dumpdir, MAXPGPATH, "%s/dump", log_opts.basedir);
+ log_opts.logdir = (char *) pg_malloc(MAXPGPATH);
+ snprintf(log_opts.logdir, MAXPGPATH, "%s/log", log_opts.basedir);
+
+ if (mkdir(log_opts.basedir, S_IRWXU | S_IRWXG | S_IRWXO))
+ pg_fatal("could not create directory \"%s\": %m\n", filename_path);
+ if (mkdir(log_opts.dumpdir, S_IRWXU | S_IRWXG | S_IRWXO))
+ pg_fatal("could not create directory \"%s\": %m\n", filename_path);
+ if (mkdir(log_opts.logdir, S_IRWXU | S_IRWXG | S_IRWXO))
+ pg_fatal("could not create directory \"%s\": %m\n", filename_path);
+
+ snprintf(filename_path, sizeof(filename_path), "%s/%s", log_opts.logdir,
+ INTERNAL_LOG_FILE);
+ if ((log_opts.internal = fopen_priv(filename_path, "a")) == NULL)
+ pg_fatal("could not open log file \"%s\": %m\n", filename_path);
+
+ /* label start of upgrade in logfiles */
+ for (filename = output_files; *filename != NULL; filename++)
+ {
+ snprintf(filename_path, sizeof(filename_path), "%s/%s",
+ log_opts.logdir, *filename);
+ if ((fp = fopen_priv(filename_path, "a")) == NULL)
+ pg_fatal("could not write to log file \"%s\": %m\n", filename_path);
+
+ /* Start with newline because we might be appending to a file. */
+ fprintf(fp, "\n"
+ "-----------------------------------------------------------------\n"
+ " pg_upgrade run on %s"
+ "-----------------------------------------------------------------\n\n",
+ ctime(&run_time));
+ fclose(fp);
+ }
+}
+
static void
setup(char *argv0, bool *live_check)
@@ -306,8 +377,9 @@ prepare_new_globals(void)
prep_status("Restoring global objects in the new cluster");
exec_prog(UTILITY_LOG_FILE, NULL, true, true,
- "\"%s/psql\" " EXEC_PSQL_ARGS " %s -f \"%s\"",
+ "\"%s/psql\" " EXEC_PSQL_ARGS " %s -f \"%s/%s\"",
new_cluster.bindir, cluster_conn_opts(&new_cluster),
+ log_opts.dumpdir,
GLOBALS_DUMP_FILE);
check_ok();
}
@@ -352,10 +424,11 @@ create_new_objects(void)
true,
true,
"\"%s/pg_restore\" %s %s --exit-on-error --verbose "
- "--dbname postgres \"%s\"",
+ "--dbname postgres \"%s/%s\"",
new_cluster.bindir,
cluster_conn_opts(&new_cluster),
create_opts,
+ log_opts.dumpdir,
sql_file_name);
break; /* done once we've processed template1 */
@@ -389,10 +462,11 @@ create_new_objects(void)
parallel_exec_prog(log_file_name,
NULL,
"\"%s/pg_restore\" %s %s --exit-on-error --verbose "
- "--dbname template1 \"%s\"",
+ "--dbname template1 \"%s/%s\"",
new_cluster.bindir,
cluster_conn_opts(&new_cluster),
create_opts,
+ log_opts.dumpdir,
sql_file_name);
}
@@ -689,28 +763,5 @@ cleanup(void)
/* Remove dump and log files? */
if (!log_opts.retain)
- {
- int dbnum;
- char **filename;
-
- for (filename = output_files; *filename != NULL; filename++)
- unlink(*filename);
-
- /* remove dump files */
- unlink(GLOBALS_DUMP_FILE);
-
- if (old_cluster.dbarr.dbs)
- for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
- {
- char sql_file_name[MAXPGPATH],
- log_file_name[MAXPGPATH];
- DbInfo *old_db = &old_cluster.dbarr.dbs[dbnum];
-
- snprintf(sql_file_name, sizeof(sql_file_name), DB_DUMP_FILE_MASK, old_db->db_oid);
- unlink(sql_file_name);
-
- snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid);
- unlink(log_file_name);
- }
- }
+ rmtree(log_opts.basedir, true);
}
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index da6770d0f8..7410ab6c44 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -26,11 +26,13 @@
#define GLOBALS_DUMP_FILE "pg_upgrade_dump_globals.sql"
#define DB_DUMP_FILE_MASK "pg_upgrade_dump_%u.custom"
+/* Default path to include all the files generated internally */
+#define DEFAULT_OUTPUTDIR "pg_upgrade_output.d"
+
#define DB_DUMP_LOG_FILE_MASK "pg_upgrade_dump_%u.log"
#define SERVER_LOG_FILE "pg_upgrade_server.log"
#define UTILITY_LOG_FILE "pg_upgrade_utility.log"
#define INTERNAL_LOG_FILE "pg_upgrade_internal.log"
-
extern char *output_files[];
/*
@@ -263,6 +265,10 @@ typedef struct
FILE *internal; /* internal log FILE */
bool verbose; /* true -> be verbose in messages */
bool retain; /* retain log files on success */
+ /* Set of internal directories for output files */
+ char *basedir; /* Base output directory */
+ char *dumpdir; /* Dumps */
+ char *logdir; /* Log files */
} LogOpts;
diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index f5d4656dec..7878d233de 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -238,8 +238,10 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
* vacuumdb --freeze actually freezes the tuples.
*/
snprintf(cmd, sizeof(cmd),
- "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d -b%s %s%s\" start",
- cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
+ "\"%s/pg_ctl\" -w -l \"%s/%s\" -D \"%s\" -o \"-p %d -b%s %s%s\" start",
+ cluster->bindir,
+ log_opts.logdir,
+ SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
(cluster == &new_cluster) ?
" -c synchronous_commit=off -c fsync=off -c full_page_writes=off -c vacuum_defer_cleanup_age=0" : "",
cluster->pgopts ? cluster->pgopts : "", socket_string);
diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml
index c5ce732ee9..abce224a58 100644
--- a/doc/src/sgml/ref/pgupgrade.sgml
+++ b/doc/src/sgml/ref/pgupgrade.sgml
@@ -231,6 +231,18 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--outputdir=</option><replaceable>path</replaceable></option></term>
+ <listitem><para>Directory where any output file internal to
+ <command>pg_upgrade</command> is generated. This includes SQL and log
+ files. The directory is created when starting
+ <command>pg_upgrade</command>, and is be removed after a successful
+ upgrade unless <option>--retain</option> is specified. The default value
+ is <literal>pg_upgrade_output.d</literal> in the current working
+ directory.
+ </para></listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-?</option></term>
<term><option>--help</option></term>
@@ -768,8 +780,9 @@ psql --username=postgres --file=script.sql postgres
<para>
<application>pg_upgrade</application> creates various working files, such
- as schema dumps, in the current working directory. For security, be sure
- that that directory is not readable or writable by any other users.
+ as schema dumps, stored within <literal>pg_upgrade_output.d</literal> in
+ the current working directory. The location can be controlled with
+ <option>--outputdir</option>
</para>
<para>
--
2.34.1
signature.asc
Description: PGP signature
