Support custom socket directory in pg_upgrade
Having hit the maximum socketdir length error a number of times in pg_upgrade,
especially when running tests in a deep directory hierarchy, I figured it was
time to see if anyone else has had the same problem? The attached patch is
what I run with locally to avoid the issue, it adds a --socketdir=PATH option
to pg_upgrade which overrides the default use of CWD. Is that something that
could be considered?
cheers ./daniel
Attachments:
pg_upgrade_sockdir.patchapplication/octet-stream; name=pg_upgrade_sockdir.patch; x-unix-mode=0644Download
From bd7c4252fc907c7406aa4ca0438e74f494b2d3c3 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Tue, 9 Oct 2018 15:01:30 +0200
Subject: [PATCH] Support custom socket directory during upgrades
The upgrade process will use CWD as the location for sockets dir, which
is limited by the sockaddr_un structure to around 100 bytes. In order to
avoid hitting the limit, support an optional command line parameter for
overriding the socket directory.
---
doc/src/sgml/ref/pgupgrade.sgml | 8 ++++++++
src/bin/pg_upgrade/option.c | 26 +++++++++++++++++++++-----
src/bin/pg_upgrade/pg_upgrade.h | 2 ++
3 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml
index d51146d641..750f080282 100644
--- a/doc/src/sgml/ref/pgupgrade.sgml
+++ b/doc/src/sgml/ref/pgupgrade.sgml
@@ -163,6 +163,14 @@
</para></listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-s</option></term>
+ <term><option>--socketdir=</option><replaceable>dir</replaceable></term>
+ <listitem><para>directory to use for sockets during upgrade, default is
+ current working directory
+ </para></listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-U</option> <replaceable>username</replaceable></term>
<term><option>--username=</option><replaceable>username</replaceable></term>
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 9dbc9225a6..84ee7edab8 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -52,6 +52,7 @@ parseCommandLine(int argc, char *argv[])
{"link", no_argument, NULL, 'k'},
{"retain", no_argument, NULL, 'r'},
{"jobs", required_argument, NULL, 'j'},
+ {"socketdir", required_argument, NULL, 's'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -64,6 +65,8 @@ parseCommandLine(int argc, char *argv[])
user_opts.transfer_mode = TRANSFER_MODE_COPY;
+ user_opts.socketdir = NULL;
+
os_info.progname = get_progname(argv[0]);
/* Process libpq env. variables; load values here for usage() output */
@@ -100,7 +103,7 @@ parseCommandLine(int argc, char *argv[])
if ((log_opts.internal = fopen_priv(INTERNAL_LOG_FILE, "a")) == NULL)
pg_fatal("could not write to log file \"%s\"\n", INTERNAL_LOG_FILE);
- while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rU:v",
+ while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rs:U:v",
long_options, &optindex)) != -1)
{
switch (option)
@@ -186,6 +189,10 @@ parseCommandLine(int argc, char *argv[])
log_opts.retain = true;
break;
+ case 's':
+ user_opts.socketdir = pg_strdup(optarg);
+ break;
+
case 'U':
pg_free(os_info.user);
os_info.user = pg_strdup(optarg);
@@ -291,6 +298,7 @@ usage(void)
printf(_(" -P, --new-port=PORT new cluster port number (default %d)\n"), new_cluster.port);
printf(_(" -r, --retain retain SQL and log files after success\n"));
printf(_(" -U, --username=NAME cluster superuser (default \"%s\")\n"), os_info.user);
+ printf(_(" -s, --socketdir=DIR socket directory during upgrades (default CWD)\n"));
printf(_(" -v, --verbose enable verbose internal logging\n"));
printf(_(" -V, --version display version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
@@ -456,10 +464,18 @@ get_sock_dir(ClusterInfo *cluster, bool live_check)
{
if (!live_check)
{
- /* Use the current directory for the socket */
- cluster->sockdir = pg_malloc(MAXPGPATH);
- if (!getcwd(cluster->sockdir, MAXPGPATH))
- pg_fatal("could not determine current directory\n");
+ if (user_opts.socketdir)
+ cluster->sockdir = user_opts.socketdir;
+ else
+ {
+ /*
+ * If the user hasn't requested a specific path, use the
+ * current directory for the socket
+ */
+ cluster->sockdir = pg_malloc(MAXPGPATH);
+ if (!getcwd(cluster->sockdir, MAXPGPATH))
+ pg_log(PG_FATAL, "cannot find current directory\n");
+ }
}
else
{
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index f83a3eeb67..d745ee101c 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -298,6 +298,8 @@ typedef struct
* changes */
transferMode transfer_mode; /* copy files or link them? */
int jobs;
+ char *socketdir; /* directory to use for sockets, NULL means
+ * using the default of CWD */
} UserOpts;
typedef struct
--
2.14.1.145.gb3622a4ee
Daniel Gustafsson <daniel@yesql.se> writes:
Having hit the maximum socketdir length error a number of times in pg_upgrade,
especially when running tests in a deep directory hierarchy, I figured it was
time to see if anyone else has had the same problem? The attached patch is
what I run with locally to avoid the issue, it adds a --socketdir=PATH option
to pg_upgrade which overrides the default use of CWD. Is that something that
could be considered?
I think you could simplify matters if you installed the CWD default value
during option processing.
regards, tom lane
On 9 Oct 2018, at 16:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
Having hit the maximum socketdir length error a number of times in pg_upgrade,
especially when running tests in a deep directory hierarchy, I figured it was
time to see if anyone else has had the same problem? The attached patch is
what I run with locally to avoid the issue, it adds a --socketdir=PATH option
to pg_upgrade which overrides the default use of CWD. Is that something that
could be considered?I think you could simplify matters if you installed the CWD default value
during option processing.
The attached v2 tries to make the socketdir more like the other configurable
directories in pg_upgrade (adding an envvar for it etc). Is that more in line
with what you were suggesting? make -C src/bin/pg_upgrade check passes with
this, both unmodified and with a -s in the test script to override it. Also
fixed incorrect syntax in the docs part from v1.
cheers ./daniel
Attachments:
pg_upgrade_sockdir-v2.patchapplication/octet-stream; name=pg_upgrade_sockdir-v2.patch; x-unix-mode=0644Download
From 5c4363ea0ea2220c45b262eb2af3e5c57499bf65 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Tue, 9 Oct 2018 15:01:30 +0200
Subject: [PATCH] Support custom socket directory during upgrades
The upgrade process will use CWD as the location for sockets dir, which
is limited by the sockaddr_un structure to around 100 bytes. In order to
avoid hitting the limit, support an optional command line parameter for
overriding the socket directory.
---
doc/src/sgml/ref/pgupgrade.sgml | 8 ++++++++
src/bin/pg_upgrade/option.c | 20 +++++++++++++-------
src/bin/pg_upgrade/pg_upgrade.h | 2 ++
3 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml
index d51146d641..83f08a9d70 100644
--- a/doc/src/sgml/ref/pgupgrade.sgml
+++ b/doc/src/sgml/ref/pgupgrade.sgml
@@ -163,6 +163,14 @@
</para></listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-s</option></term>
+ <term><option>--socketdir=</option><replaceable>dir</replaceable></term>
+ <listitem><para>directory to use for sockets during upgrade, default is
+ current working directory
+ </para></listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-U</option> <replaceable>username</replaceable></term>
<term><option>--username=</option><replaceable>username</replaceable></term>
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 9dbc9225a6..3ace73b7f1 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -52,6 +52,7 @@ parseCommandLine(int argc, char *argv[])
{"link", no_argument, NULL, 'k'},
{"retain", no_argument, NULL, 'r'},
{"jobs", required_argument, NULL, 'j'},
+ {"socketdir", required_argument, NULL, 's'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -64,6 +65,9 @@ parseCommandLine(int argc, char *argv[])
user_opts.transfer_mode = TRANSFER_MODE_COPY;
+ user_opts.socketdir = pg_malloc(MAXPGPATH);
+ getcwd(user_opts.socketdir, MAXPGPATH);
+
os_info.progname = get_progname(argv[0]);
/* Process libpq env. variables; load values here for usage() output */
@@ -100,7 +104,7 @@ parseCommandLine(int argc, char *argv[])
if ((log_opts.internal = fopen_priv(INTERNAL_LOG_FILE, "a")) == NULL)
pg_fatal("could not write to log file \"%s\"\n", INTERNAL_LOG_FILE);
- while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rU:v",
+ while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rs:U:v",
long_options, &optindex)) != -1)
{
switch (option)
@@ -186,6 +190,10 @@ parseCommandLine(int argc, char *argv[])
log_opts.retain = true;
break;
+ case 's':
+ strlcpy(user_opts.socketdir, optarg, MAXPGPATH);
+ break;
+
case 'U':
pg_free(os_info.user);
os_info.user = pg_strdup(optarg);
@@ -246,6 +254,8 @@ parseCommandLine(int argc, char *argv[])
"PGDATAOLD", "-d", _("old cluster data resides"));
check_required_directory(&new_cluster.pgdata, &new_cluster.pgconfig,
"PGDATANEW", "-D", _("new cluster data resides"));
+ check_required_directory(&user_opts.socketdir, NULL, "PGSOCKETDIR", "-s",
+ _("sockets will be created"));
#ifdef WIN32
@@ -291,6 +301,7 @@ usage(void)
printf(_(" -P, --new-port=PORT new cluster port number (default %d)\n"), new_cluster.port);
printf(_(" -r, --retain retain SQL and log files after success\n"));
printf(_(" -U, --username=NAME cluster superuser (default \"%s\")\n"), os_info.user);
+ printf(_(" -s, --socketdir=DIR socket directory during upgrades (default CWD)\n"));
printf(_(" -v, --verbose enable verbose internal logging\n"));
printf(_(" -V, --version display version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
@@ -455,12 +466,7 @@ get_sock_dir(ClusterInfo *cluster, bool live_check)
if (GET_MAJOR_VERSION(cluster->major_version) >= 901)
{
if (!live_check)
- {
- /* Use the current directory for the socket */
- cluster->sockdir = pg_malloc(MAXPGPATH);
- if (!getcwd(cluster->sockdir, MAXPGPATH))
- pg_fatal("could not determine current directory\n");
- }
+ cluster->sockdir = user_opts.socketdir;
else
{
/*
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index f83a3eeb67..d745ee101c 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -298,6 +298,8 @@ typedef struct
* changes */
transferMode transfer_mode; /* copy files or link them? */
int jobs;
+ char *socketdir; /* directory to use for sockets, NULL means
+ * using the default of CWD */
} UserOpts;
typedef struct
--
2.14.1.145.gb3622a4ee
Hi,
I reviewed `pg_upgrade_sockdir-v2.patch`.
I checked `-s` option on OSX. I confirmed that all tools, which are
internally invoked such as pg_dumpall and pg_restore, used the specified
socket and pg_upgrade worked as expected.
I think this patch is fine.
Best regards,
Show quoted text
On 2018/10/09 21:26, Daniel Gustafsson wrote:
On 9 Oct 2018, at 16:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
Having hit the maximum socketdir length error a number of times in pg_upgrade,
especially when running tests in a deep directory hierarchy, I figured it was
time to see if anyone else has had the same problem? The attached patch is
what I run with locally to avoid the issue, it adds a --socketdir=PATH option
to pg_upgrade which overrides the default use of CWD. Is that something that
could be considered?I think you could simplify matters if you installed the CWD default value
during option processing.The attached v2 tries to make the socketdir more like the other configurable
directories in pg_upgrade (adding an envvar for it etc). Is that more in line
with what you were suggesting? make -C src/bin/pg_upgrade check passes with
this, both unmodified and with a -s in the test script to override it. Also
fixed incorrect syntax in the docs part from v1.cheers ./daniel
On Wed, Oct 10, 2018 at 9:27 AM Daniel Gustafsson <daniel@yesql.se> wrote:
On 9 Oct 2018, at 16:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:Having hit the maximum socketdir length error a number of times in pg_upgrade,
especially when running tests in a deep directory hierarchy, I figured it was
time to see if anyone else has had the same problem? The attached patch is
what I run with locally to avoid the issue, it adds a --socketdir=PATH option
to pg_upgrade which overrides the default use of CWD. Is that something that
could be considered?I think you could simplify matters if you installed the CWD default value
during option processing.The attached v2 tries to make the socketdir more like the other configurable
directories in pg_upgrade (adding an envvar for it etc). Is that more in line
with what you were suggesting? make -C src/bin/pg_upgrade check passes with
this, both unmodified and with a -s in the test script to override it. Also
fixed incorrect syntax in the docs part from v1.
I think PGSOCKETDIR should be mentioned in the documentation like the
other environment variables, and also I'm wondering if it actually
works: you set it to the current working directory first, then parse
the command line option if present, and then read the env var only if
not already set: but it's always going to be, isn't it? Perhaps you
should use getcwd() only if all else fails?
--
Thomas Munro
http://www.enterprisedb.com
On 09/10/2018 15:09, Daniel Gustafsson wrote:
Having hit the maximum socketdir length error a number of times in pg_upgrade,
especially when running tests in a deep directory hierarchy, I figured it was
time to see if anyone else has had the same problem? The attached patch is
what I run with locally to avoid the issue, it adds a --socketdir=PATH option
to pg_upgrade which overrides the default use of CWD. Is that something that
could be considered?
Why not always create a temporary directory and put it there. Then we
don't need an option. It's not like the current directory is a
particularly good choice anyway.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 6 Nov 2018, at 09:19, Thomas Munro <thomas.munro@enterprisedb.com> wrote:
On Wed, Oct 10, 2018 at 9:27 AM Daniel Gustafsson <daniel@yesql.se <mailto:daniel@yesql.se>> wrote:
On 9 Oct 2018, at 16:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:Having hit the maximum socketdir length error a number of times in pg_upgrade,
especially when running tests in a deep directory hierarchy, I figured it was
time to see if anyone else has had the same problem? The attached patch is
what I run with locally to avoid the issue, it adds a --socketdir=PATH option
to pg_upgrade which overrides the default use of CWD. Is that something that
could be considered?I think you could simplify matters if you installed the CWD default value
during option processing.The attached v2 tries to make the socketdir more like the other configurable
directories in pg_upgrade (adding an envvar for it etc). Is that more in line
with what you were suggesting? make -C src/bin/pg_upgrade check passes with
this, both unmodified and with a -s in the test script to override it. Also
fixed incorrect syntax in the docs part from v1.I think PGSOCKETDIR should be mentioned in the documentation like the
other environment variables,
Of course, fixed.
and also I'm wondering if it actually
works: you set it to the current working directory first, then parse
the command line option if present, and then read the env var only if
not already set: but it's always going to be, isn't it? Perhaps you
should use getcwd() only if all else fails?
Yes, you’re right, I had a thinko in my patch as well as in the testing of it.
The attached version sets cwd as the default in case all else fails. Extending
check_required_directory() to do this may not be to everyones liking, but it
seemed the cleanest option to me.
cheers ./daniel
Attachments:
pg_upgrade_sockdir-v3.patchapplication/octet-stream; name=pg_upgrade_sockdir-v3.patch; x-unix-mode=0644Download
From 616af29a17c0d18b3c9681855f54cead0d2f66ae Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Tue, 9 Oct 2018 15:01:30 +0200
Subject: [PATCH] Support custom socket directory during upgrades
The upgrade process will use CWD as the location for sockets dir, which
is limited by the sockaddr_un structure to around 100 bytes. In order to
avoid hitting the limit, support an optional command line parameter, with
an associated environment variable for overriding the socket directory.
---
doc/src/sgml/ref/pgupgrade.sgml | 8 +++++++
src/bin/pg_upgrade/option.c | 49 ++++++++++++++++++++++++++---------------
src/bin/pg_upgrade/pg_upgrade.h | 2 ++
3 files changed, 41 insertions(+), 18 deletions(-)
diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml
index d51146d641..31a4144207 100644
--- a/doc/src/sgml/ref/pgupgrade.sgml
+++ b/doc/src/sgml/ref/pgupgrade.sgml
@@ -163,6 +163,14 @@
</para></listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-s</option></term>
+ <term><option>--socketdir=</option><replaceable>dir</replaceable></term>
+ <listitem><para>directory to use for sockets during upgrade, default is
+ current working directory; environment variable <envar>PGSOCKETDIR</envar>
+ </para></listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-U</option> <replaceable>username</replaceable></term>
<term><option>--username=</option><replaceable>username</replaceable></term>
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 9dbc9225a6..9c58125972 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -22,7 +22,8 @@
static void usage(void);
static void check_required_directory(char **dirpath, char **configpath,
- const char *envVarName, const char *cmdLineOption, const char *description);
+ const char *envVarName, const char *cmdLineOption,
+ const char *description, const char *defaultValue);
#define FIX_DEFAULT_READ_ONLY "-c default_transaction_read_only=false"
@@ -52,6 +53,7 @@ parseCommandLine(int argc, char *argv[])
{"link", no_argument, NULL, 'k'},
{"retain", no_argument, NULL, 'r'},
{"jobs", required_argument, NULL, 'j'},
+ {"socketdir", required_argument, NULL, 's'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -61,6 +63,7 @@ parseCommandLine(int argc, char *argv[])
FILE *fp;
char **filename;
time_t run_time = time(NULL);
+ char default_sockdir[MAXPGPATH];
user_opts.transfer_mode = TRANSFER_MODE_COPY;
@@ -100,7 +103,7 @@ parseCommandLine(int argc, char *argv[])
if ((log_opts.internal = fopen_priv(INTERNAL_LOG_FILE, "a")) == NULL)
pg_fatal("could not write to log file \"%s\"\n", INTERNAL_LOG_FILE);
- while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rU:v",
+ while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rs:U:v",
long_options, &optindex)) != -1)
{
switch (option)
@@ -186,6 +189,11 @@ parseCommandLine(int argc, char *argv[])
log_opts.retain = true;
break;
+ case 's':
+ user_opts.socketdir = pg_malloc0(MAXPGPATH);
+ strlcpy(user_opts.socketdir, optarg, MAXPGPATH);
+ break;
+
case 'U':
pg_free(os_info.user);
os_info.user = pg_strdup(optarg);
@@ -239,13 +247,18 @@ parseCommandLine(int argc, char *argv[])
/* Get values from env if not already set */
check_required_directory(&old_cluster.bindir, NULL, "PGBINOLD", "-b",
- _("old cluster binaries reside"));
+ _("old cluster binaries reside"), NULL);
check_required_directory(&new_cluster.bindir, NULL, "PGBINNEW", "-B",
- _("new cluster binaries reside"));
+ _("new cluster binaries reside"), NULL);
check_required_directory(&old_cluster.pgdata, &old_cluster.pgconfig,
- "PGDATAOLD", "-d", _("old cluster data resides"));
+ "PGDATAOLD", "-d", _("old cluster data resides"),
+ NULL);
check_required_directory(&new_cluster.pgdata, &new_cluster.pgconfig,
- "PGDATANEW", "-D", _("new cluster data resides"));
+ "PGDATANEW", "-D", _("new cluster data resides"),
+ NULL);
+ getcwd(default_sockdir, MAXPGPATH);
+ check_required_directory(&user_opts.socketdir, NULL, "PGSOCKETDIR", "-s",
+ _("sockets will be created"), default_sockdir);
#ifdef WIN32
@@ -291,6 +304,7 @@ usage(void)
printf(_(" -P, --new-port=PORT new cluster port number (default %d)\n"), new_cluster.port);
printf(_(" -r, --retain retain SQL and log files after success\n"));
printf(_(" -U, --username=NAME cluster superuser (default \"%s\")\n"), os_info.user);
+ printf(_(" -s, --socketdir=DIR socket directory during upgrades (default CWD)\n"));
printf(_(" -v, --verbose enable verbose internal logging\n"));
printf(_(" -V, --version display version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
@@ -335,29 +349,33 @@ usage(void)
* envVarName - the name of an environment variable to get if dirpath is NULL
* cmdLineOption - the command line option corresponds to this directory (-o, -O, -n, -N)
* description - a description of this directory option
+ * defaultValue - a default value to assign in case the environment variable
+ * isn't set
*
* We use the last two arguments to construct a meaningful error message if the
- * user hasn't provided the required directory name.
+ * user hasn't provided the required directory name, and a default isn't
+ * specified.
*/
static void
check_required_directory(char **dirpath, char **configpath,
const char *envVarName, const char *cmdLineOption,
- const char *description)
+ const char *description, const char *defaultValue)
{
if (*dirpath == NULL || strlen(*dirpath) == 0)
{
const char *envVar;
if ((envVar = getenv(envVarName)) && strlen(envVar))
- {
*dirpath = pg_strdup(envVar);
- if (configpath)
- *configpath = pg_strdup(envVar);
- }
+ else if (defaultValue)
+ *dirpath = pg_strdup(defaultValue);
else
pg_fatal("You must identify the directory where the %s.\n"
"Please use the %s command-line option or the %s environment variable.\n",
description, cmdLineOption, envVarName);
+
+ if (configpath)
+ *configpath = pg_strdup(*dirpath);
}
/*
@@ -455,12 +473,7 @@ get_sock_dir(ClusterInfo *cluster, bool live_check)
if (GET_MAJOR_VERSION(cluster->major_version) >= 901)
{
if (!live_check)
- {
- /* Use the current directory for the socket */
- cluster->sockdir = pg_malloc(MAXPGPATH);
- if (!getcwd(cluster->sockdir, MAXPGPATH))
- pg_fatal("could not determine current directory\n");
- }
+ cluster->sockdir = user_opts.socketdir;
else
{
/*
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index f83a3eeb67..d745ee101c 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -298,6 +298,8 @@ typedef struct
* changes */
transferMode transfer_mode; /* copy files or link them? */
int jobs;
+ char *socketdir; /* directory to use for sockets, NULL means
+ * using the default of CWD */
} UserOpts;
typedef struct
--
2.14.1.145.gb3622a4ee
On 7 Nov 2018, at 08:23, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
On 09/10/2018 15:09, Daniel Gustafsson wrote:
Having hit the maximum socketdir length error a number of times in pg_upgrade,
especially when running tests in a deep directory hierarchy, I figured it was
time to see if anyone else has had the same problem? The attached patch is
what I run with locally to avoid the issue, it adds a --socketdir=PATH option
to pg_upgrade which overrides the default use of CWD. Is that something that
could be considered?Why not always create a temporary directory and put it there. Then we
don't need an option. It's not like the current directory is a
particularly good choice anyway.
I agree that cwd isn’t a terribly good default, but is there a good way to
identify a suitable temporary directory to use across all platforms (mostly
thinking about Windows)? Overloading PGDATA/base/pgsql_tmp (or similar) in
either the new or old datadir seems ugly, and risks running into the sockdir
limitation this patch is intending to solve.
cheers ./daniel
Daniel Gustafsson <daniel@yesql.se> writes:
On 7 Nov 2018, at 08:23, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
Why not always create a temporary directory and put it there. Then we
don't need an option. It's not like the current directory is a
particularly good choice anyway.
I agree that cwd isn’t a terribly good default, but is there a good way to
identify a suitable temporary directory to use across all platforms (mostly
thinking about Windows)?
Windows isn't a problem because it doesn't do Unix-style sockets anyway.
However, I agree that Peter's proposal is just begging the question of
how we'd identify a better default choice than cwd.
Also, even if we had an arguably-better idea, I suspect that there would
always be cases where it didn't work. For example, one idea is to make
a temporary directory under the installation's normal socket directory
(thus, /tmp/pgXXXX/ or some such). But, if the normal socket directory
is not /tmp, we might find that pg_upgrade can't write there.
So I'm inclined to think that a --socketdir option is a reasonable
feature independently of whether someone comes up with a different
proposal for the default location.
regards, tom lane
On 12/11/2018 20:00, Tom Lane wrote:
Also, even if we had an arguably-better idea, I suspect that there would
always be cases where it didn't work. For example, one idea is to make
a temporary directory under the installation's normal socket directory
(thus, /tmp/pgXXXX/ or some such). But, if the normal socket directory
is not /tmp, we might find that pg_upgrade can't write there.
We do exactly that in pg_regress and it's never been a problem.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
On 12/11/2018 20:00, Tom Lane wrote:
Also, even if we had an arguably-better idea, I suspect that there would
always be cases where it didn't work. For example, one idea is to make
a temporary directory under the installation's normal socket directory
(thus, /tmp/pgXXXX/ or some such). But, if the normal socket directory
is not /tmp, we might find that pg_upgrade can't write there.
We do exactly that in pg_regress and it's never been a problem.
Yeah, but pg_upgrade is used by a much wider variety of people
than pg_regress.
regards, tom lane
I wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
On 12/11/2018 20:00, Tom Lane wrote:
Also, even if we had an arguably-better idea, I suspect that there would
always be cases where it didn't work. For example, one idea is to make
a temporary directory under the installation's normal socket directory
(thus, /tmp/pgXXXX/ or some such). But, if the normal socket directory
is not /tmp, we might find that pg_upgrade can't write there.
We do exactly that in pg_regress and it's never been a problem.
Yeah, but pg_upgrade is used by a much wider variety of people
than pg_regress.
Further point about that: pg_regress's method of creating a temp
directory under /tmp is secure only on machines with the stickybit
set on /tmp; otherwise it's possible for an attacker to rename the
temp dir out of the way and inject his own socket. We agreed that
that was an okay risk to take for testing purposes, but I'm much
less willing to assume that it's okay for production use with
pg_upgrade. So I think we should keep the default situation being
that we put the socket in cwd, which we're already assuming is
secure because that's where we put the transient dump files.
That implies that we need this switch for use-cases where cwd
isn't workable due to long pathname or permissions considerations.
regards, tom lane
On 15 Nov 2018, at 22:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Further point about that: pg_regress's method of creating a temp
directory under /tmp is secure only on machines with the stickybit
set on /tmp; otherwise it's possible for an attacker to rename the
temp dir out of the way and inject his own socket. We agreed that
that was an okay risk to take for testing purposes, but I'm much
less willing to assume that it's okay for production use with
pg_upgrade.
That’s a good point, it’s not an assumption I’d be comfortable with when it
deals with system upgrades.
cheers ./daniel
On 2018-Nov-12, Tom Lane wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
On 12/11/2018 20:00, Tom Lane wrote:
Also, even if we had an arguably-better idea, I suspect that there would
always be cases where it didn't work. For example, one idea is to make
a temporary directory under the installation's normal socket directory
(thus, /tmp/pgXXXX/ or some such). But, if the normal socket directory
is not /tmp, we might find that pg_upgrade can't write there.We do exactly that in pg_regress and it's never been a problem.
Yeah, but pg_upgrade is used by a much wider variety of people
than pg_regress.
Surely they can just set TMPDIR if /tmp is not writable? If TMPDIR is
set and not writable, bark at the user for it.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 12/11/2018 20:00, Tom Lane wrote:
Also, even if we had an arguably-better idea, I suspect that there would
always be cases where it didn't work.
Surely they can just set TMPDIR if /tmp is not writable? If TMPDIR is
set and not writable, bark at the user for it.
(1) There was nothing about TMPDIR in Peter's proposal, nor would an
implementation based on mkdtemp(3) automatically do that for us.
(2) If you accept the proposition that we must provide a user knob of
some sort, why shouldn't it be a command line switch rather than an
environment variable? The former are much easier to document and to
discover.
regards, tom lane
Daniel Gustafsson <daniel@yesql.se> writes:
[ pg_upgrade_sockdir-v3.patch ]
BTW, I notice that cfbot doesn't like this now that Thomas is running it
with -Werror:
option.c: In function ‘parseCommandLine’:
option.c:265:8: error: ignoring return value of ‘getcwd’, declared with attribute warn_unused_result [-Werror=unused-result]
getcwd(default_sockdir, MAXPGPATH);
^
cc1: all warnings being treated as errors
Failing to check a syscall's result isn't per project style even if
the tools would let you get away with it. Other places in that same
file do
if (!getcwd(cwd, MAXPGPATH))
pg_fatal("could not determine current directory\n");
regards, tom lane
I wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 12/11/2018 20:00, Tom Lane wrote:
Also, even if we had an arguably-better idea, I suspect that there would
always be cases where it didn't work.
Surely they can just set TMPDIR if /tmp is not writable? If TMPDIR is
set and not writable, bark at the user for it.
(1) There was nothing about TMPDIR in Peter's proposal, nor would an
implementation based on mkdtemp(3) automatically do that for us.
(2) If you accept the proposition that we must provide a user knob of
some sort, why shouldn't it be a command line switch rather than an
environment variable? The former are much easier to document and to
discover.
So we seem to be at an impasse here. By my count, three people have
expressed support for the patch's approach of adding a socket-directory
option, while two people seem to prefer the idea of putting pg_upgrade's
socket under /tmp (possibly with a way to override that). That's not
enough of a consensus to proceed with either approach, really, but
we ought to do something because the problem is real.
Given that we have a patch for this approach, and no patch has been
offered for the /tmp approach, I'm kind of inclined to exercise
committer's discretion and proceed with this patch. Will anybody
be seriously annoyed if I do?
regards, tom lane
On 30/11/2018 17:58, Tom Lane wrote:
So we seem to be at an impasse here. By my count, three people have
expressed support for the patch's approach of adding a socket-directory
option, while two people seem to prefer the idea of putting pg_upgrade's
socket under /tmp (possibly with a way to override that). That's not
enough of a consensus to proceed with either approach, really, but
we ought to do something because the problem is real.Given that we have a patch for this approach, and no patch has been
offered for the /tmp approach, I'm kind of inclined to exercise
committer's discretion and proceed with this patch. Will anybody
be seriously annoyed if I do?
I think it's fair to proceed and leave open that someone submits a
(possibly) better patch for a different approach in the future.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
On 30/11/2018 17:58, Tom Lane wrote:
Given that we have a patch for this approach, and no patch has been
offered for the /tmp approach, I'm kind of inclined to exercise
committer's discretion and proceed with this patch. Will anybody
be seriously annoyed if I do?
I think it's fair to proceed and leave open that someone submits a
(possibly) better patch for a different approach in the future.
I don't think we'd be able to remove the --socketdir switch once added,
but certainly it doesn't preclude changing the algorithm for default
socket placement.
Pushed with minor code cleanup.
regards, tom lane
On Sat, Nov 17, 2018 at 10:15:08PM +0100, Daniel Gustafsson wrote:
On 15 Nov 2018, at 22:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Further point about that: pg_regress's method of creating a temp
directory under /tmp is secure only on machines with the stickybit
set on /tmp; otherwise it's possible for an attacker to rename the
temp dir out of the way and inject his own socket. We agreed that
that was an okay risk to take for testing purposes, but I'm much
less willing to assume that it's okay for production use with
pg_upgrade.That’s a good point, it’s not an assumption I’d be comfortable with when it
deals with system upgrades.
As in /messages/by-id/flat/20140329222934.GC170273@tornado.leadboat.com, I
maintain that insecure /tmp is not worth worrying about in any part of
PostgreSQL.