Support custom socket directory in pg_upgrade

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

Support custom socket directory in pg_upgrade

Daniel Gustafsson
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


pg_upgrade_sockdir.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Tom Lane-2
Daniel Gustafsson <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Daniel Gustafsson
> On 9 Oct 2018, at 16:22, Tom Lane <[hidden email]> wrote:

>
> Daniel Gustafsson <[hidden email]> 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






pg_upgrade_sockdir-v2.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Hironobu SUZUKI
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 <[hidden email]> wrote:
>>
>> Daniel Gustafsson <[hidden email]> 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
>
>
>
>
>
>


Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Thomas Munro-3
In reply to this post by Daniel Gustafsson
On Wed, Oct 10, 2018 at 9:27 AM Daniel Gustafsson <[hidden email]> wrote:

> > On 9 Oct 2018, at 16:22, Tom Lane <[hidden email]> wrote:
> > Daniel Gustafsson <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Peter Eisentraut-6
In reply to this post by Daniel Gustafsson
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

Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Daniel Gustafsson
In reply to this post by Thomas Munro-3
> On 6 Nov 2018, at 09:19, Thomas Munro <[hidden email]> wrote:

>
> On Wed, Oct 10, 2018 at 9:27 AM Daniel Gustafsson <[hidden email] <mailto:[hidden email]>> wrote:
>>> On 9 Oct 2018, at 16:22, Tom Lane <[hidden email]> wrote:
>>> Daniel Gustafsson <[hidden email]> 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


pg_upgrade_sockdir-v3.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Daniel Gustafsson
In reply to this post by Peter Eisentraut-6
> On 7 Nov 2018, at 08:23, Peter Eisentraut <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Tom Lane-2
Daniel Gustafsson <[hidden email]> writes:
>> On 7 Nov 2018, at 08:23, Peter Eisentraut <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Peter Eisentraut-6
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

Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Tom Lane-2
Peter Eisentraut <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Tom Lane-2
I wrote:
> Peter Eisentraut <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Daniel Gustafsson
> On 15 Nov 2018, at 22:42, Tom Lane <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Alvaro Herrera-9
In reply to this post by Tom Lane-2
On 2018-Nov-12, Tom Lane wrote:

> Peter Eisentraut <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Tom Lane-2
Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Tom Lane-2
In reply to this post by Daniel Gustafsson
Daniel Gustafsson <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Tom Lane-2
In reply to this post by Tom Lane-2
I wrote:
> Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Peter Eisentraut-6
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

Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Tom Lane-2
Peter Eisentraut <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Support custom socket directory in pg_upgrade

Noah Misch-2
In reply to this post by Daniel Gustafsson
On Sat, Nov 17, 2018 at 10:15:08PM +0100, Daniel Gustafsson wrote:

> > On 15 Nov 2018, at 22:42, Tom Lane <[hidden email]> 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@..., I
maintain that insecure /tmp is not worth worrying about in any part of
PostgreSQL.