Horton

Thread

Commits

  1. Allow psql to re-use connection parameters after a connection loss.

  2. Clean up some unpleasant behaviors in psql's \connect command.

  3. Fix connection string handling in psql's \connect command.

  1. Mop-up around psql's \connect behavior

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-10-21T22:59:04Z

    While working on commit 85c54287a, I noticed a few things I did not
much care for in do_connect().  These don't quite seem to rise to
the level of back-patchable bugs, but they're still not great:

* The initial stanza that complains about

	if (!o_conn && (!dbname || !user || !host || !port))

seems woefully obsolete.  In the first place, it's pretty silly
to equate a "complete connection specification" with having just
those four values; the whole point of 85c54287a and predecessors
is that other settings such as sslmode may be just as important.
In the second place, this fails to consider the possibility that
we only have a connstring parameter --- which may nonetheless
provide all the required settings.  And in the third place,
this clearly wasn't revisited when we added explicit control of
whether or not we're supposed to re-use parameters from the old
connection.  It's very silly to insist on having an o_conn if we're
going to ignore it anyway.

I think the reason we've not had complaints about this is that the
situation normally doesn't arise in interactive sessions (since we
won't release the old connection voluntarily), while scripts are
likely not designed to cope with connection losses anyway.  These
facts militate against spending a whole lot of effort on a fix,
but still we ought to reduce the silliness factor.  What I propose
is to complain if we have no o_conn *and* we are asked to re-use
parameters from it.  Otherwise, it's fine.

* I really don't like the bit about silently ignoring user, host,
and port parameters if we see that the first parameter is a connstring.
That's as user-unfriendly as can be.  It should be a syntax error
to specify both; the documentation certainly implies that it is.

* The old-style-syntax code path understands that it should re-use
the old password (if any) when the user, host, and port settings
haven't changed.  The connstring code path was too lazy to make
that work, but now that we're deconstructing the connstring there's
very little excuse for not having it act the same way.

The attached patch fixes these things and documents the password
behavior, which for some reason went unmentioned before.  Along
the way I simplified the mechanism for re-using a password a bit;
there's no reason to treat it so much differently from re-using
other parameters.

Any objections?

			regards, tom lane

  2. Re: Mop-up around psql's \connect behavior

    Chapman Flack <chap@anastigmatix.net> — 2020-10-21T23:04:49Z

    On 10/21/20 18:59, Tom Lane wrote:

> I think the reason we've not had complaints about this is that the
> situation normally doesn't arise in interactive sessions (since we
> won't release the old connection voluntarily), while scripts are
> likely not designed to cope with connection losses anyway.  These
> facts militate against spending a whole lot of effort on a fix,
> but still we ought to reduce the silliness factor.  What I propose
> is to complain if we have no o_conn *and* we are asked to re-use
> parameters from it.  Otherwise, it's fine.

I've been getting around it just by saying

  \c "connstring" . . .

which works. It gives me a tiny thrill every time I do it, like I'm
getting away with something. Which is why I haven't been complaining.

I suppose I wouldn't complain if it were fixed, either.

Regards,
-Chap

  3. Re: Mop-up around psql's \connect behavior

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-10-22T03:05:15Z

    At Wed, 21 Oct 2020 18:59:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> While working on commit 85c54287a, I noticed a few things I did not
> much care for in do_connect().  These don't quite seem to rise to
> the level of back-patchable bugs, but they're still not great:
> 
> * The initial stanza that complains about
> 
> 	if (!o_conn && (!dbname || !user || !host || !port))
> 
> seems woefully obsolete.  In the first place, it's pretty silly
> to equate a "complete connection specification" with having just
> those four values; the whole point of 85c54287a and predecessors
> is that other settings such as sslmode may be just as important.
> In the second place, this fails to consider the possibility that
> we only have a connstring parameter --- which may nonetheless
> provide all the required settings.  And in the third place,
> this clearly wasn't revisited when we added explicit control of
> whether or not we're supposed to re-use parameters from the old
> connection.  It's very silly to insist on having an o_conn if we're
> going to ignore it anyway.

Sounds reasonable.

> I think the reason we've not had complaints about this is that the
> situation normally doesn't arise in interactive sessions (since we
> won't release the old connection voluntarily), while scripts are
> likely not designed to cope with connection losses anyway.  These
> facts militate against spending a whole lot of effort on a fix,
> but still we ought to reduce the silliness factor.  What I propose
> is to complain if we have no o_conn *and* we are asked to re-use
> parameters from it.  Otherwise, it's fine.

The reason I haven't complain about this is I don't reconnect by \c
after involuntary disconnection. (That is, C-d then psql again:p) But
once it got on my mind, it might be strange that just \c or \c
-reuse-previous=y doesn't reconnect a broken session. It might be
better we reuse the previous connection parameter even if the
connection has been lost, but this would be another issue.

> * I really don't like the bit about silently ignoring user, host,
> and port parameters if we see that the first parameter is a connstring.
> That's as user-unfriendly as can be.  It should be a syntax error
> to specify both; the documentation certainly implies that it is.

+1

> * The old-style-syntax code path understands that it should re-use
> the old password (if any) when the user, host, and port settings
> haven't changed.  The connstring code path was too lazy to make
> that work, but now that we're deconstructing the connstring there's
> very little excuse for not having it act the same way.

+1 (I thought sslmode might affect but that is wrong since cert
authenticaion cannot be turned off from command line.)

> The attached patch fixes these things and documents the password
> behavior, which for some reason went unmentioned before.  Along
> the way I simplified the mechanism for re-using a password a bit;
> there's no reason to treat it so much differently from re-using
> other parameters.

Looks fine.

> Any objections?

Nope from me.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  4. Re: Mop-up around psql's \connect behavior

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-10-22T04:34:20Z

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Wed, 21 Oct 2020 18:59:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
>> ...  What I propose
>> is to complain if we have no o_conn *and* we are asked to re-use
>> parameters from it.  Otherwise, it's fine.

> The reason I haven't complain about this is I don't reconnect by \c
> after involuntary disconnection. (That is, C-d then psql again:p)

Yeah, me too.

> But once it got on my mind, it might be strange that just \c or \c
> -reuse-previous=y doesn't reconnect a broken session. It might be
> better we reuse the previous connection parameter even if the
> connection has been lost, but this would be another issue.

I did actually look into saving the active connection's PQconninfo
immediately at connection establishment and then referring to it in any
subsequent \connect.  Then things could work the same even if the original
connection had failed meanwhile.  But there are technical details that
make that a lot harder than it seems on the surface --- mainly, that the
way do_connect works now requires that it have a copy of the PQconninfo
data that it can scribble on, and that won't do if we need the saved
PQconninfo to persist when a \connect attempt fails.  That could be dealt
with with enough new code, but I didn't think it was worth the trouble.
(Note that we developers face the server-crashed scenario a whole lot more
often than normal people ;-), so we probably overrate how useful it'd be
to be able to reconnect in that case.)

>> * The old-style-syntax code path understands that it should re-use
>> the old password (if any) when the user, host, and port settings
>> haven't changed.  The connstring code path was too lazy to make
>> that work, but now that we're deconstructing the connstring there's
>> very little excuse for not having it act the same way.

> +1 (I thought sslmode might affect but that is wrong since cert
> authenticaion cannot be turned off from command line.)

Yeah.  That could affect whether the server asks for a password at
all, but you have to really stretch to think of cases where it could
result in needing a *different* password.  An example perhaps is
where pg_hba.conf is configured to do, say, LDAP auth on SSL connections
and normal password auth on non-SSL, and the LDAP server has a different
password than what is in pg_authid.  But that seems like something nobody
could want.  Also notice that unlike the previous code, with my patch
do_connect will always (barring --no-password) give you an opportunity
to interactively supply a password, even if we initially reused an
old password and it didn't work.  So it seems like this will cope
even if you do have a setup as wacko as that.

			regards, tom lane

  5. Re: Mop-up around psql's \connect behavior

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-10-22T05:26:33Z

    At Thu, 22 Oct 2020 00:34:20 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > At Wed, 21 Oct 2020 18:59:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> > But once it got on my mind, it might be strange that just \c or \c
> > -reuse-previous=y doesn't reconnect a broken session. It might be
> > better we reuse the previous connection parameter even if the
> > connection has been lost, but this would be another issue.
> 
> I did actually look into saving the active connection's PQconninfo
> immediately at connection establishment and then referring to it in any
> subsequent \connect.  Then things could work the same even if the original
> connection had failed meanwhile.  But there are technical details that
> make that a lot harder than it seems on the surface --- mainly, that the
> way do_connect works now requires that it have a copy of the PQconninfo
> data that it can scribble on, and that won't do if we need the saved
> PQconninfo to persist when a \connect attempt fails.  That could be dealt
> with with enough new code, but I didn't think it was worth the trouble.

Agreed.

> (Note that we developers face the server-crashed scenario a whole lot more
> often than normal people ;-), so we probably overrate how useful it'd be
> to be able to reconnect in that case.)

Agreed^2.

> >> * The old-style-syntax code path understands that it should re-use
> >> the old password (if any) when the user, host, and port settings
> >> haven't changed.  The connstring code path was too lazy to make
> >> that work, but now that we're deconstructing the connstring there's
> >> very little excuse for not having it act the same way.
> 
> > +1 (I thought sslmode might affect but that is wrong since cert
> > authenticaion cannot be turned off from command line.)
> 
> Yeah.  That could affect whether the server asks for a password at
> all, but you have to really stretch to think of cases where it could
> result in needing a *different* password.  An example perhaps is
> where pg_hba.conf is configured to do, say, LDAP auth on SSL connections
> and normal password auth on non-SSL, and the LDAP server has a different
> password than what is in pg_authid.  But that seems like something nobody
> could want.  Also notice that unlike the previous code, with my patch
> do_connect will always (barring --no-password) give you an opportunity
> to interactively supply a password, even if we initially reused an
> old password and it didn't work.  So it seems like this will cope
> even if you do have a setup as wacko as that.

I thought of that scenarios and conclused as the same. Sounds
reasonable.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  6. Re: Mop-up around psql's \connect behavior

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-10-22T19:23:04Z

    I wrote:
> I did actually look into saving the active connection's PQconninfo
> immediately at connection establishment and then referring to it in any
> subsequent \connect.  Then things could work the same even if the original
> connection had failed meanwhile.  But there are technical details that
> make that a lot harder than it seems on the surface --- mainly, that the
> way do_connect works now requires that it have a copy of the PQconninfo
> data that it can scribble on, and that won't do if we need the saved
> PQconninfo to persist when a \connect attempt fails.  That could be dealt
> with with enough new code, but I didn't think it was worth the trouble.

Actually ... I'd no sooner pushed that patch than I realized that there
*is* an easy, if rather grotty, way to deal with this.  We can just not
issue PQfinish on the old/dead connection until we've successfully made
a new one.  PQconninfo doesn't care if the connection is in BAD state.

To avoid introducing weird behaviors, we can't keep the logically-dead
connection in pset.db, but introducing a separate variable to hold such
a connection doesn't seem too awful.  So that leads me to the attached
patch, which is able to reconnect even if we lost the connection:

regression=# select 1;
 ?column? 
----------
        1
(1 row)

-- in another window, stop the server, then:

regression=# select 1;
FATAL:  terminating connection due to administrator command
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

--- now restart the server, and:

!?> \c
You are now connected to database "regression" as user "postgres" via socket in "/tmp" at port "5432".

I would not have wanted to accept a patch that did it the other way,
because it would have been a mess, but this seems small enough to
be worth doing.  The only real objection I can see is that it could
hold a server connection open when the user thinks there is none;
but that could only happen in a non-interactive script, and it does
not seem like a big problem in that case.  We could alternatively
not stash the "dead" connection after a non-interactive \connect
failure, but I doubt that's better.

			regards, tom lane

  7. Re: Mop-up around psql's \connect behavior

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-10-23T05:28:38Z

    At Thu, 22 Oct 2020 15:23:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> I wrote:
> > I did actually look into saving the active connection's PQconninfo
> > immediately at connection establishment and then referring to it in any
> > subsequent \connect.  Then things could work the same even if the original
> > connection had failed meanwhile.  But there are technical details that
> > make that a lot harder than it seems on the surface --- mainly, that the
> > way do_connect works now requires that it have a copy of the PQconninfo
> > data that it can scribble on, and that won't do if we need the saved
> > PQconninfo to persist when a \connect attempt fails.  That could be dealt
> > with with enough new code, but I didn't think it was worth the trouble.
> 
> Actually ... I'd no sooner pushed that patch than I realized that there
> *is* an easy, if rather grotty, way to deal with this.  We can just not
> issue PQfinish on the old/dead connection until we've successfully made
> a new one.  PQconninfo doesn't care if the connection is in BAD state.
> 
> To avoid introducing weird behaviors, we can't keep the logically-dead
> connection in pset.db, but introducing a separate variable to hold such
> a connection doesn't seem too awful.  So that leads me to the attached
> patch, which is able to reconnect even if we lost the connection:

Sounds reasonable.

> regression=# select 1;
>  ?column? 
> ----------
>         1
> (1 row)
> 
> -- in another window, stop the server, then:
> 
> regression=# select 1;
> FATAL:  terminating connection due to administrator command
> server closed the connection unexpectedly
>         This probably means the server terminated abnormally
>         before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> 
> --- now restart the server, and:
> 
> !?> \c
> You are now connected to database "regression" as user "postgres" via socket in "/tmp" at port "5432".

Looks good to me.  I'm very happy with the result.

> I would not have wanted to accept a patch that did it the other way,
> because it would have been a mess, but this seems small enough to
> be worth doing.  The only real objection I can see is that it could
> hold a server connection open when the user thinks there is none;
> but that could only happen in a non-interactive script, and it does
> not seem like a big problem in that case.  We could alternatively
> not stash the "dead" connection after a non-interactive \connect
> failure, but I doubt that's better.

Agreed. Thanks!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  8. Re: Mop-up around psql's \connect behavior

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-10-23T21:12:44Z

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Thu, 22 Oct 2020 15:23:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
>> ... The only real objection I can see is that it could
>> hold a server connection open when the user thinks there is none;
>> but that could only happen in a non-interactive script, and it does
>> not seem like a big problem in that case.  We could alternatively
>> not stash the "dead" connection after a non-interactive \connect
>> failure, but I doubt that's better.

> Agreed. Thanks!

After further thought I decided we *must* do it as per my "alternative"
idea.  Consider a script containing
	\c db1 user1 live_server
	\c db2 user2 dead_server
	\c db3
The script would be expecting to connect to db3 at dead_server, but
if we re-use parameters from the first connection then it might
successfully connect to db3 at live_server.  This'd defeat the goal
of not letting a script accidentally execute commands against the
wrong database.

So we have to not save the connection after a failed script \connect.
However, it seems OK to save after a connection loss whether we're
in a script or not; that is,

	\c db1 user1 server1
	...
	(connection dies here)
	...  --- these commands will fail
	\c db2

The script will be expecting the second \c to re-use parameters
from the first one, and that will still work as expected.

I went ahead and pushed it after adjusting that.

			regards, tom lane

  9. Re: Mop-up around psql's \connect behavior

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-10-26T00:51:41Z

    At Fri, 23 Oct 2020 17:12:44 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > At Thu, 22 Oct 2020 15:23:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> >> ... The only real objection I can see is that it could
> >> hold a server connection open when the user thinks there is none;
> >> but that could only happen in a non-interactive script, and it does
> >> not seem like a big problem in that case.  We could alternatively
> >> not stash the "dead" connection after a non-interactive \connect
> >> failure, but I doubt that's better.
> 
> > Agreed. Thanks!
> 
> After further thought I decided we *must* do it as per my "alternative"
> idea.  Consider a script containing
> 	\c db1 user1 live_server
> 	\c db2 user2 dead_server
> 	\c db3
> The script would be expecting to connect to db3 at dead_server, but
> if we re-use parameters from the first connection then it might
> successfully connect to db3 at live_server.  This'd defeat the goal
> of not letting a script accidentally execute commands against the
> wrong database.

Hmm. True.

> So we have to not save the connection after a failed script \connect.

Yes, we shouldn't save a connection parameters that haven't made a
connection.

> However, it seems OK to save after a connection loss whether we're
> in a script or not; that is,
> 
> 	\c db1 user1 server1
> 	...
> 	(connection dies here)
> 	...  --- these commands will fail
> 	\c db2
> 
> The script will be expecting the second \c to re-use parameters
> from the first one, and that will still work as expected.

Agreed.

> I went ahead and pushed it after adjusting that.

Thanks!

-- 
Kyotaro Horiguchi
NTT Open Source Software Center