Обсуждение: 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
Вложения
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 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
Вложения
> 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 https://postgr.es/m/flat/20140329222934.GC170273@tornado.leadboat.com, I maintain that insecure /tmp is not worth worrying about in any part of PostgreSQL.