Обсуждение: Danger of automatic connection reset in psql

Since this is already an improvement, I'm attaching a patch.

If on the other hand, someone is pasting into psql's terminal a block of commands enclosed in BEGIN/COMMIT, the same bug is triggered: BEGIN doesn't have effect and the rest of commands run outside of transaction.

Is it possible at all to protect against the latter case?  How?

A couple of doubts/suggestions:
1. Should pset.conn_was_reset be set to false, when we make a
connection in do_connection()? Usually pset.conn_was_reset would be
initialised with 0 since it's a global variable, so this may not be
necessary. But as a precaution should we do this?

2. Comment below should be updated to reflect the new change
            /* fall out of loop if lexer reached EOL */
-           if (scan_result == PSCAN_INCOMPLETE ||
+           if (pset.conn_was_reset ||
+               scan_result == PSCAN_INCOMPLETE ||

3. What happens when the connection is reset while the source is a
file say specified by \i in an interactive shell?

On 11/4/16 4:04 AM, Oleksandr Shulgin wrote:

The psql process even exits with an error code 2, which might be not
that expected.  We could stop reading the file and reset connection
afterwards, but this is probably not that easy to achieve (think of
nested \i calls).

Well, if you stop reading from the file then I don't think more \i's will matter, no? I'd certainly like to see improvement here, because the difference in behavior with \i is annoying.

On the bigger question of how to better protect all these cases (cut & paste, etc), I think the only robust way to do that is for psql to track intended transaction status itself. With the amount of parsing it's already doing, maybe that wouldn't be that hard to add. It looks like this might require extra libpq calls to constantly check in on server status; I'm a bit surprised that result objects don't include that info.

>
> How about, instead of all this, adding an option to psql to suppress
> the automatic reconnect behavior?  When enabled, psql just exits
> instead of trying to reconnect.
>
+1. But, existing users may not notice addition of the new option and
still continue to face problem. If we add the option and make it
default not to reconnect, they will notice it and use option to get
older behaviour, but that will break applications relying on the
current behaviour. Either way, users will have at least something to
control the connection reset.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2016-11-11 5:14 GMT+01:00 Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>:

>
> How about, instead of all this, adding an option to psql to suppress
> the automatic reconnect behavior?  When enabled, psql just exits
> instead of trying to reconnect.
>
+1. But, existing users may not notice addition of the new option and
still continue to face problem. If we add the option and make it
default not to reconnect, they will notice it and use option to get
older behaviour, but that will break applications relying on the
current behaviour. Either way, users will have at least something to
control the connection reset.

The reconnect in not interactive mode is a bad idea - and there should be disabled everywhere (it cannot to break any application - the behave of script when a server is restarted must be undefined (100% if ON_ERROR_STOP is active). In interactive mode it can be controlled by some psql session variables like AUTOCOMMIT.

On 11/14/16 5:41 AM, Oleksandr Shulgin wrote:

Automatic connection reset is a nice feature for server development,
IMO.  Is it really useful for anything else is a good question.

I use it all the time for application development; my rebuild script will forcibly kick everyone out to re-create the database. I put that in because I invariably end up with a random psql sitting somewhere that I don't want to track down.

What currently stinks though is if the connection is dead and the next command I run is a \i, psql just dies instead of re-connecting. It'd be nice if before reading the script it checked connection status and attempted a reconnect.

At least an option to control that behavior seems like a good idea,
maybe even set it to 'no reconnect' by default, so that people who
really use it can make conscious choice about enabling it in their
.psqlrc or elsewhere.

+1, I don't think it needs to be the default.

On Mon, Nov 21, 2016 at 4:55 AM, Oleksandr Shulgin
<oleksandr.shulgin@zalando.de> wrote:
> On Tue, Nov 15, 2016 at 4:10 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
>>
>> On 11/14/16 5:41 AM, Oleksandr Shulgin wrote:
>>>
>>> Automatic connection reset is a nice feature for server development,
>>> IMO.  Is it really useful for anything else is a good question.
>>
>>
>> I use it all the time for application development; my rebuild script will
>> forcibly kick everyone out to re-create the database. I put that in because
>> I invariably end up with a random psql sitting somewhere that I don't want
>> to track down.
>>
>> What currently stinks though is if the connection is dead and the next
>> command I run is a \i, psql just dies instead of re-connecting. It'd be nice
>> if before reading the script it checked connection status and attempted a
>> reconnect.
>>
>>> At least an option to control that behavior seems like a good idea,
>>> maybe even set it to 'no reconnect' by default, so that people who
>>> really use it can make conscious choice about enabling it in their
>>> .psqlrc or elsewhere.
>>
>>
>> +1, I don't think it needs to be the default.
>
>
> So if we go in this direction, should the option be specified from command
> line or available via psqlrc (or both?)  I think both make sense.
>
> What should be the option and control variable names?  Something like:
> --reconnect and RECONNECT?  Should we allow reconnect in non-interactive
> mode?  I have no use case for that, but it might be different for others.
> If non-interactive is not supported then it could be a simple boolean
> variable, otherwise we might want something like a tri-state: on / off /
> interactive (the last one being the default).

I think it should just be another psql special variable, like
AUTOCOMMIT or VERBOSITY.  If the user wants to set it on the command
line, they can just use -v.  We don't need a separate, dedicated
option for this, I think.

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

2016-11-22 3:46 GMT+01:00 Robert Haas <robertmhaas@gmail.com>:

On Mon, Nov 21, 2016 at 4:55 AM, Oleksandr Shulgin
<oleksandr.shulgin@zalando.de> wrote:
> On Tue, Nov 15, 2016 at 4:10 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
>>
>> On 11/14/16 5:41 AM, Oleksandr Shulgin wrote:
>>>
>>> Automatic connection reset is a nice feature for server development,
>>> IMO.  Is it really useful for anything else is a good question.
>>
>>
>> I use it all the time for application development; my rebuild script will
>> forcibly kick everyone out to re-create the database. I put that in because
>> I invariably end up with a random psql sitting somewhere that I don't want
>> to track down.
>>
>> What currently stinks though is if the connection is dead and the next
>> command I run is a \i, psql just dies instead of re-connecting. It'd be nice
>> if before reading the script it checked connection status and attempted a
>> reconnect.
>>
>>> At least an option to control that behavior seems like a good idea,
>>> maybe even set it to 'no reconnect' by default, so that people who
>>> really use it can make conscious choice about enabling it in their
>>> .psqlrc or elsewhere.
>>
>>
>> +1, I don't think it needs to be the default.
>
>
> So if we go in this direction, should the option be specified from command
> line or available via psqlrc (or both?)  I think both make sense.
>
> What should be the option and control variable names?  Something like:
> --reconnect and RECONNECT?  Should we allow reconnect in non-interactive
> mode?  I have no use case for that, but it might be different for others.
> If non-interactive is not supported then it could be a simple boolean
> variable, otherwise we might want something like a tri-state: on / off /
> interactive (the last one being the default).

I think it should just be another psql special variable, like
AUTOCOMMIT or VERBOSITY.  If the user wants to set it on the command
line, they can just use -v.  We don't need a separate, dedicated
option for this, I think.

In this case depends on a default. For almost all scripts the sensible value is "without reconnect". It be unfriendly to use this setting via -v variable.

On Tue, Nov 22, 2016 at 5:28 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2016-11-22 3:46 GMT+01:00 Robert Haas <robertmhaas@gmail.com>:

On Mon, Nov 21, 2016 at 4:55 AM, Oleksandr Shulgin
<oleksandr.shulgin@zalando.de> wrote:
> On Tue, Nov 15, 2016 at 4:10 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
>>
>> On 11/14/16 5:41 AM, Oleksandr Shulgin wrote:
>>>
>>> Automatic connection reset is a nice feature for server development,
>>> IMO.  Is it really useful for anything else is a good question.
>>
>>
>> I use it all the time for application development; my rebuild script will
>> forcibly kick everyone out to re-create the database. I put that in because
>> I invariably end up with a random psql sitting somewhere that I don't want
>> to track down.
>>
>> What currently stinks though is if the connection is dead and the next
>> command I run is a \i, psql just dies instead of re-connecting. It'd be nice
>> if before reading the script it checked connection status and attempted a
>> reconnect.
>>
>>> At least an option to control that behavior seems like a good idea,
>>> maybe even set it to 'no reconnect' by default, so that people who
>>> really use it can make conscious choice about enabling it in their
>>> .psqlrc or elsewhere.
>>
>>
>> +1, I don't think it needs to be the default.
>
>
> So if we go in this direction, should the option be specified from command
> line or available via psqlrc (or both?)  I think both make sense.
>
> What should be the option and control variable names?  Something like:
> --reconnect and RECONNECT?  Should we allow reconnect in non-interactive
> mode?  I have no use case for that, but it might be different for others.
> If non-interactive is not supported then it could be a simple boolean
> variable, otherwise we might want something like a tri-state: on / off /
> interactive (the last one being the default).

I think it should just be another psql special variable, like
AUTOCOMMIT or VERBOSITY.  If the user wants to set it on the command
line, they can just use -v.  We don't need a separate, dedicated
option for this, I think.

That makes sense to me.

In this case depends on a default. For almost all scripts the sensible value is "without reconnect". It be unfriendly to use this setting via -v variable.

Well, if you're running a script it should not be affected as long as default value for this new variable is "interactive" or "off" (and you didn't override it in psqlrc).  If you really want to get a "reconnect even from the script" type of behavior, then you'll have to use -v or set the variable from inside the script itself to "on".

--

Alex

2016-11-22 13:02 GMT+01:00 Oleksandr Shulgin <oleksandr.shulgin@zalando.de>:

On Tue, Nov 22, 2016 at 5:28 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2016-11-22 3:46 GMT+01:00 Robert Haas <robertmhaas@gmail.com>:

On Mon, Nov 21, 2016 at 4:55 AM, Oleksandr Shulgin
<oleksandr.shulgin@zalando.de> wrote:
> On Tue, Nov 15, 2016 at 4:10 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
>>
>> On 11/14/16 5:41 AM, Oleksandr Shulgin wrote:
>>>
>>> Automatic connection reset is a nice feature for server development,
>>> IMO.  Is it really useful for anything else is a good question.
>>
>>
>> I use it all the time for application development; my rebuild script will
>> forcibly kick everyone out to re-create the database. I put that in because
>> I invariably end up with a random psql sitting somewhere that I don't want
>> to track down.
>>
>> What currently stinks though is if the connection is dead and the next
>> command I run is a \i, psql just dies instead of re-connecting. It'd be nice
>> if before reading the script it checked connection status and attempted a
>> reconnect.
>>
>>> At least an option to control that behavior seems like a good idea,
>>> maybe even set it to 'no reconnect' by default, so that people who
>>> really use it can make conscious choice about enabling it in their
>>> .psqlrc or elsewhere.
>>
>>
>> +1, I don't think it needs to be the default.
>
>
> So if we go in this direction, should the option be specified from command
> line or available via psqlrc (or both?)  I think both make sense.
>
> What should be the option and control variable names?  Something like:
> --reconnect and RECONNECT?  Should we allow reconnect in non-interactive
> mode?  I have no use case for that, but it might be different for others.
> If non-interactive is not supported then it could be a simple boolean
> variable, otherwise we might want something like a tri-state: on / off /
> interactive (the last one being the default).

I think it should just be another psql special variable, like
AUTOCOMMIT or VERBOSITY.  If the user wants to set it on the command
line, they can just use -v.  We don't need a separate, dedicated
option for this, I think.

That makes sense to me.

In this case depends on a default. For almost all scripts the sensible value is "without reconnect". It be unfriendly to use this setting via -v variable.

Well, if you're running a script it should not be affected as long as default value for this new variable is "interactive" or "off" (and you didn't override it in psqlrc).  If you really want to get a "reconnect even from the script" type of behavior, then you'll have to use -v or set the variable from inside the script itself to "on".

ok