Horton

Thread

Commits

  1. Handle corner cases correctly in psql's reconnection logic.

  1. psql should re-read connection variables after connection reset

    Peter Billen <peter.billen@gmail.com> — 2019-05-28T09:56:03Z

    Hi,

After a connection reset, psql should re-read the connection variables.
This was was initially reported by ysch on IRC and confirmed in the code by
Zr40. All I'm doing here is making sure that it is reported, as per ysch's
request.

I quickly verified this as following:

   1. start 11 instance
   2. psql into it
   3. stop 11 instance
   4. start 10 instance
   5. in the existing psql session, first trigger a reconnect ('select 1')
and then '\df', which depends on the server version. I got:

ERROR:  column p.prokind does not exist
LINE 5:  CASE p.prokind

This happens because the psql session believes it is still connected to the
11 instance, as confirmed by Zr40:

[11:32:16] <Zr40> CheckConnection (
https://github.com/postgres/postgres/blob/master/src/bin/psql/common.c#L386)
doesn't call SyncVariables (
https://github.com/postgres/postgres/blob/master/src/bin/psql/command.c#L3286)
which is called on startup (
https://github.com/postgres/postgres/blob/master/src/bin/psql/startup.c#L312
)

Best regards.

  2. Re: psql should re-read connection variables after connection reset

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-02T21:24:35Z

    Peter Billen <peter.billen@gmail.com> writes:
> After a connection reset, psql should re-read the connection variables.
> This was was initially reported by ysch on IRC and confirmed in the code by
> Zr40. All I'm doing here is making sure that it is reported, as per ysch's
> request.

> I quickly verified this as following:

>    1. start 11 instance
>    2. psql into it
>    3. stop 11 instance
>    4. start 10 instance
>    5. in the existing psql session, first trigger a reconnect ('select 1')
> and then '\df', which depends on the server version. I got:

Hm.  I'm not entirely convinced that that needs to be a supported
situation.  However, it is a bit odd that CheckConnection is willing to
do UnsyncVariables in one code path but not SyncVariables in the other.

I wonder whether we should also do connection_warnings(false) in this
code path.  That could be annoyingly chatty on SSL or GSS connections,
though.  It's too bad that printSSLInfo and printGSSInfo don't have
any notion like "print only if different from before".  More, I can
think of cases where they're not chatty enough: if before you had an
SSL-encrypted connection, and now you don't, that seems like something
you might wish to know about.

			regards, tom lane

  3. Re: psql should re-read connection variables after connection reset

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-08-31T19:40:18Z

    I wrote:
> Peter Billen <peter.billen@gmail.com> writes:
>> After a connection reset, psql should re-read the connection variables.
>> This was was initially reported by ysch on IRC and confirmed in the code by
>> Zr40. All I'm doing here is making sure that it is reported, as per ysch's
>> request.
>> I quickly verified this as following:
>> 1. start 11 instance
>> 2. psql into it
>> 3. stop 11 instance
>> 4. start 10 instance
>> 5. in the existing psql session, first trigger a reconnect ('select 1')
>> and then '\df', which depends on the server version. I got:

> Hm.  I'm not entirely convinced that that needs to be a supported
> situation.  However, it is a bit odd that CheckConnection is willing to
> do UnsyncVariables in one code path but not SyncVariables in the other.

After further thought, there's really no reason *not* to do SyncVariables
here; in normal cases it would accomplish nothing, but it won't cost
much compared to all the other overhead of establishing a connection.

> I wonder whether we should also do connection_warnings(false) in this
> code path.  That could be annoyingly chatty on SSL or GSS connections,
> though.  It's too bad that printSSLInfo and printGSSInfo don't have
> any notion like "print only if different from before".  More, I can
> think of cases where they're not chatty enough: if before you had an
> SSL-encrypted connection, and now you don't, that seems like something
> you might wish to know about.

And here, my inclination is just to do connection_warnings(false) and
call it good.  That means you'll get an SSL (resp. GSS) banner if
SSL is active and no banner if it isn't.  A user who's not attuned
to what to expect might not realize the significance of not seeing
anything, but this is such a corner case that I'm unconvinced that
it's worth working harder.

However, it does seem like the Windows-code-page warning is useless
to repeat --- AFAICS that state can't change from one connection
to the next.  So I'm inclined to fix it to only print that at
startup.

In sum, then, something like the attached.  But I'm unsure about
whether to back-patch this.  Maybe, in the stable branches, we
should only back-patch addition of SyncVariables()?  But it
hardly seems likely that anyone's app could be broken by this ---
connection_warnings() doesn't print anything in noninteractive
cases.

			regards, tom lane