Support custom socket directory in pg_upgrade

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

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,

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

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.