Re: pg_upgrade and PGPORT

pg_upgrade is a bit schizophrenic concerning the PGPORT environment
variable. On the one hand, there is this code in option.c that wants to
make use of it:

old_cluster.port = getenv("PGPORT") ? atoi(getenv("PGPORT")) : DEF_PGPORT;
new_cluster.port = getenv("PGPORT") ? atoi(getenv("PGPORT")) : DEF_PGPORT;

On the other hand, check.c will reject a set PGPORT because it's a libpq
environment variable. Should we make an exception for PGPORT, like we
did for PGCLIENTENCODING?

Peter Eisentraut wrote:
> pg_upgrade is a bit schizophrenic concerning the PGPORT environment
> variable. On the one hand, there is this code in option.c that wants to
> make use of it:
>
> old_cluster.port = getenv("PGPORT") ? atoi(getenv("PGPORT")) : DEF_PGPORT;
> new_cluster.port = getenv("PGPORT") ? atoi(getenv("PGPORT")) : DEF_PGPORT;
>
> On the other hand, check.c will reject a set PGPORT because it's a libpq
> environment variable. Should we make an exception for PGPORT, like we
> did for PGCLIENTENCODING?

Wow, good question. Passing stuff in via libpq is certainly complex.

I ran a test and it looks like the command-line flag overrides the
PGPORT environment variable:

$ export PGPORT=3333
$ psql test
psql: could not connect to server: No such file or directory
Is the server running locally and accepting
connections on Unix domain socket "/tmp/.s.PGSQL.3333"?
$ psql -p 5432 test
psql (9.1beta1)
Type "help" for help.

test=>

I assume it is just like PGCLIENTENCODING. PGCLIENTENCODING was easier
to ignore because we need it for error messages.

Are there other cases we should allow too?

A larger question is whether we should just disable all the checks for
environment variables. The C comment says:

* check_for_libpq_envvars()
*
* tests whether any libpq environment variables are set.
* Since pg_upgrade connects to both the old and the new server,
* it is potentially dangerous to have any of these set.
*
* If any are found, will log them and cancel.

I am not sure what to do.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> A larger question is whether we should just disable all the checks for
> environment variables. The C comment says:

> * check_for_libpq_envvars()
> *
> * tests whether any libpq environment variables are set.
> * Since pg_upgrade connects to both the old and the new server,
> * it is potentially dangerous to have any of these set.
> *
> * If any are found, will log them and cancel.

> I am not sure what to do.

Well, the risk mentioned in that comment certainly seems real.

An alternative solution that might be more user-friendly is to ensure
that the connection strings pg_upgrade uses specify all important
options, leaving nothing to be overridden by environment variables.
Then you don't need to make the user adjust his environment.

Or you could just "unsetenv" instead of complaining.

I would like to think that eventually pg_upgrade won't start a
postmaster at all, but connect using something more like a standalone
backend. So someday the issue might go away --- but that someday
isn't especially close.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > A larger question is whether we should just disable all the checks for
> > environment variables. The C comment says:
>
> > * check_for_libpq_envvars()
> > *
> > * tests whether any libpq environment variables are set.
> > * Since pg_upgrade connects to both the old and the new server,
> > * it is potentially dangerous to have any of these set.
> > *
> > * If any are found, will log them and cancel.
>
> > I am not sure what to do.
>
> Well, the risk mentioned in that comment certainly seems real.
>
> An alternative solution that might be more user-friendly is to ensure
> that the connection strings pg_upgrade uses specify all important
> options, leaving nothing to be overridden by environment variables.
> Then you don't need to make the user adjust his environment.

Well, they can use the same port number for both servers. In fact I
just use the compiled-default of 5432 when I am testing. No reason they
could not supply value in an environment variable but it would have to
be the same for old and new server (the two servers never run at the
same time).

> Or you could just "unsetenv" instead of complaining.

I think it is really PGDATA that we certainly can't inherit from an
environment variable.

> I would like to think that eventually pg_upgrade won't start a
> postmaster at all, but connect using something more like a standalone
> backend. So someday the issue might go away --- but that someday
> isn't especially close.

That standalone backend is going to have to understand pg_dump SQL
output, with \connect, etc.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Wed, May 11, 2011 at 2:18 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Or you could just "unsetenv" instead of complaining.

+1 for that.

> I would like to think that eventually pg_upgrade won't start a
> postmaster at all, but connect using something more like a standalone
> backend.  So someday the issue might go away --- but that someday
> isn't especially close.

And +1 for that, too.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Wed, May 11, 2011 at 2:18 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Or you could just "unsetenv" instead of complaining.
>
> +1 for that.

OK, the attached patch does this, but allows PGCLIENTENCODING to be
passed in. The new output looks like:

Performing Consistency Checks
-----------------------------
ignoring libpq environment variable PGPORT
Checking old data directory (/u/pgsql.old/data) ok
Checking old bin directory (/u/pgsql.old/bin) ok
Checking new data directory (/u/pgsql/data) ok
Checking new bin directory (/u/pgsql/bin) ok

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On ons, 2011-05-11 at 18:36 -0400, Bruce Momjian wrote:
> Robert Haas wrote:
> > On Wed, May 11, 2011 at 2:18 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > Or you could just "unsetenv" instead of complaining.
> >
> > +1 for that.
>
> OK, the attached patch does this, but allows PGCLIENTENCODING to be
> passed in. The new output looks like:
>
> Performing Consistency Checks
> -----------------------------
> ignoring libpq environment variable PGPORT

I haven't tried it, but I suppose option.c will now make use of PGPORT
and then later you get that message that it was ignored?

On Thu, May 12, 2011 at 1:22 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On ons, 2011-05-11 at 18:36 -0400, Bruce Momjian wrote:
>> Robert Haas wrote:
>> > On Wed, May 11, 2011 at 2:18 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> > > Or you could just "unsetenv" instead of complaining.
>> >
>> > +1 for that.
>>
>> OK, the attached patch does this, but allows PGCLIENTENCODING to be
>> passed in.  The new output looks like:
>>
>>       Performing Consistency Checks
>>       -----------------------------
>>       ignoring libpq environment variable PGPORT
>
> I haven't tried it, but I suppose option.c will now make use of PGPORT
> and then later you get that message that it was ignored?

Either way, it hardly seems necessary to emit a log message stating
that you are unsetting an environment variable.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Thu, May 12, 2011 at 1:22 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> > On ons, 2011-05-11 at 18:36 -0400, Bruce Momjian wrote:
> >> Robert Haas wrote:
> >> > On Wed, May 11, 2011 at 2:18 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> > > Or you could just "unsetenv" instead of complaining.
> >> >
> >> > +1 for that.
> >>
> >> OK, the attached patch does this, but allows PGCLIENTENCODING to be
> >> passed in. ?The new output looks like:
> >>
> >> ? ? ? Performing Consistency Checks
> >> ? ? ? -----------------------------
> >> ? ? ? ignoring libpq environment variable PGPORT
> >
> > I haven't tried it, but I suppose option.c will now make use of PGPORT
> > and then later you get that message that it was ignored?
>
> Either way, it hardly seems necessary to emit a log message stating
> that you are unsetting an environment variable.

I think the whole idea of worrying about libpq environment variables is
useless. I looked at the list of libpq environment variables and I saw
a lot of useful ones, like PGUSER and PGPASSFILE, which we currently
throw an error.

I propose we only disable the use of PGHOST and even then that prevents
users from controlling tcp vs. unix domain connections.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:
> > >> ? ? ? Performing Consistency Checks
> > >> ? ? ? -----------------------------
> > >> ? ? ? ignoring libpq environment variable PGPORT
> > >
> > > I haven't tried it, but I suppose option.c will now make use of PGPORT
> > > and then later you get that message that it was ignored?
> >
> > Either way, it hardly seems necessary to emit a log message stating
> > that you are unsetting an environment variable.
>
> I think the whole idea of worrying about libpq environment variables is
> useless. I looked at the list of libpq environment variables and I saw
> a lot of useful ones, like PGUSER and PGPASSFILE, which we currently
> throw an error.
>
> I propose we only disable the use of PGHOST and even then that prevents
> users from controlling tcp vs. unix domain connections.

OK, it turns out the environment variable handling in pg_upgrade was
worse than I thought. This patch:

o disables only PGHOST and only if it is set to a non-local value;
all other environment variables are honored; PGDATA isn't even seen
by libpq
o push --user value into the PGUSER environment variable so pg_ctl -w
uses it; pg_ctl has no --user flag; this is important for pre-9.1
pg_ctl binaries
o move putenv() function to utils.c now that it is used by option.c
o allow pg_ctl failure to continue with a connection request to get a
possible error message, then exit
o update document to be clearer and mention environment variables

Patch attached.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:
> Bruce Momjian wrote:
> > > >> ? ? ? Performing Consistency Checks
> > > >> ? ? ? -----------------------------
> > > >> ? ? ? ignoring libpq environment variable PGPORT
> > > >
> > > > I haven't tried it, but I suppose option.c will now make use of PGPORT
> > > > and then later you get that message that it was ignored?
> > >
> > > Either way, it hardly seems necessary to emit a log message stating
> > > that you are unsetting an environment variable.
> >
> > I think the whole idea of worrying about libpq environment variables is
> > useless. I looked at the list of libpq environment variables and I saw
> > a lot of useful ones, like PGUSER and PGPASSFILE, which we currently
> > throw an error.
> >
> > I propose we only disable the use of PGHOST and even then that prevents
> > users from controlling tcp vs. unix domain connections.
>
> OK, it turns out the environment variable handling in pg_upgrade was
> worse than I thought. This patch:
>
> o disables only PGHOST and only if it is set to a non-local value;
> all other environment variables are honored; PGDATA isn't even seen
> by libpq
> o push --user value into the PGUSER environment variable so pg_ctl -w
> uses it; pg_ctl has no --user flag; this is important for pre-9.1
> pg_ctl binaries
> o move putenv() function to utils.c now that it is used by option.c
> o allow pg_ctl failure to continue with a connection request to get a
> possible error message, then exit
> o update document to be clearer and mention environment variables
>
> Patch attached.

I have applied the updated attached patch which allows pg_upgrade to
honor environment variable. This updated version is clearer about
handling of PGHOST, and adds PGHOSTADDR checks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +