Обсуждение: BUG #10794: psql sometimes ignores .psqlrc
The following bug has been logged on the website: Bug reference: 10794 Logged by: Marko Tiikkaja Email address: marko@joh.to PostgreSQL version: 9.3.4 Operating system: OS X, Linux at least Description: Hi, If I press ctrl-C while psql is prompting for a password but then log in (either because I used -W and the server isn't expecting a password or if I subsequently typed in the correct password), .psqlrc isn't processed. This seems quite dangerous if the user is e.g. assuming a specific value for AUTOCOMMIT. I hear the behaviour is gone in HEAD; might have been fixed by commit 9099e4afe0e4101bc79f078be3e15639a048e633, not sure. I think something deserves to be back patched.
On 6/28/14, 11:13 PM, marko@joh.to wrote: > If I press ctrl-C while psql is prompting for a password but then log in > (either because I used -W and the server isn't expecting a password or if I > subsequently typed in the correct password), .psqlrc isn't processed. This > seems quite dangerous if the user is e.g. assuming a specific value for > AUTOCOMMIT. (seeing that back branch releases are upcoming..) Ping? .marko
Marko Tiikkaja <marko@joh.to> writes:
> On 6/28/14, 11:13 PM, marko@joh.to wrote:
>> If I press ctrl-C while psql is prompting for a password but then log in
>> (either because I used -W and the server isn't expecting a password or if I
>> subsequently typed in the correct password), .psqlrc isn't processed. This
>> seems quite dangerous if the user is e.g. assuming a specific value for
>> AUTOCOMMIT.
> (seeing that back branch releases are upcoming..) Ping?
I can reproduce that in 9.3, but not in 9.4/HEAD: now, if you control-C at
the password prompt, the program just exits instantly. That's evidently a
result of commit 9099e4afe, which postponed the installation of the SIGINT
handler till after the password prompt. What is happening in the earlier
branches is that the SIGINT handler is setting cancel_pressed, which
doesn't affect collection of the password, but which the main loop then
takes as an indication that you wanted to abandon processing of the script
file (ie, .psqlrc).
There's a bit of a race condition here, in that if you press control-C
just *after* connection setup, it will still allow psql to start up after
having not processed the .psqlrc file. I think though that we'd be ill
advised to try to prevent that by postponing control-C setup even further;
what if the .psqlrc file is broken and initiates some long-running
operation?
So I'm satisfied with the behavior of HEAD in this area. There's a case
to be made for back-patching commit 9099e4afe into older branches, but
I'm not sure if that'd be a good idea or not. People might be depending
on the old behavior.
A different line of thought would be to reset cancel_pressed before
starting the processing of any script file, so that you actually have to
press control-C *during* the processing of a file to cancel it. That's
just narrowing the race condition window some more, but it might be
worth doing.
In any case, I'm disinclined to mess with this in the back branches ...
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Marko Tiikkaja <marko@joh.to> writes:
> > On 6/28/14, 11:13 PM, marko@joh.to wrote:
> >> If I press ctrl-C while psql is prompting for a password but then log =
in
> >> (either because I used -W and the server isn't expecting a password or=
if I
> >> subsequently typed in the correct password), .psqlrc isn't processed. =
This
> >> seems quite dangerous if the user is e.g. assuming a specific value for
> >> AUTOCOMMIT.
>=20
> > (seeing that back branch releases are upcoming..) Ping?
>=20
> I can reproduce that in 9.3, but not in 9.4/HEAD: now, if you control-C at
> the password prompt, the program just exits instantly. That's evidently a
> result of commit 9099e4afe, which postponed the installation of the SIGINT
> handler till after the password prompt. What is happening in the earlier
> branches is that the SIGINT handler is setting cancel_pressed, which
> doesn't affect collection of the password, but which the main loop then
> takes as an indication that you wanted to abandon processing of the script
> file (ie, .psqlrc).
>=20
> There's a bit of a race condition here, in that if you press control-C
> just *after* connection setup, it will still allow psql to start up after
> having not processed the .psqlrc file. I think though that we'd be ill
> advised to try to prevent that by postponing control-C setup even further;
> what if the .psqlrc file is broken and initiates some long-running
> operation?
This I agree with.
> So I'm satisfied with the behavior of HEAD in this area. There's a case
> to be made for back-patching commit 9099e4afe into older branches, but
> I'm not sure if that'd be a good idea or not. People might be depending
> on the old behavior.
I've not looked at this closely, but I have to say that I've always felt
that ignoring ctrl-c at the password prompt is really bad form- it's
confusing to those who don't understand what's going on and really
annoying for those who do.
While we're at it, we should fix the complaint about .psql_history being
missing.. This isn't an error, imv:
could not save history to file "/home/sfrost/.psql_history": No such file o=
r directory
Not sure where we started doing that.
> A different line of thought would be to reset cancel_pressed before
> starting the processing of any script file, so that you actually have to
> press control-C *during* the processing of a file to cancel it. That's
> just narrowing the race condition window some more, but it might be
> worth doing.
Narrowing the window would probably be good to do, but I don't see a
need to back-patch that.
> In any case, I'm disinclined to mess with this in the back branches ...
If we have a simple/clean patch to fix the "Ctrl-C ignored at password
prompt", such that, instead, psql exits more-or-less immediately then
I'd be for back-patching just that as it certainly has always felt like
a wart or even a bug to me, but I agree that we don't want to introduce
other behavior changes beyond that in back-branches.
Thanks,
Stephen
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> In any case, I'm disinclined to mess with this in the back branches ...
> If we have a simple/clean patch to fix the "Ctrl-C ignored at password
> prompt", such that, instead, psql exits more-or-less immediately then
> I'd be for back-patching just that as it certainly has always felt like
> a wart or even a bug to me, but I agree that we don't want to introduce
> other behavior changes beyond that in back-branches.
That would be commit 9099e4afe. I won't vote against back-patching if
people think it's unlikely that anyone is expecting the old behavior.
> While we're at it, we should fix the complaint about .psql_history being
> missing.. This isn't an error, imv:
> could not save history to file "/home/sfrost/.psql_history": No such file or directory
Eh? I've not seen anything like that personally. I seem to recall having
heard of it happening with broken readline or libedit versions, but we
don't have very much ability to work around bugs in those.
regards, tom lane
(adding Peter to the CC, as the author of 9099e4afe)
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> In any case, I'm disinclined to mess with this in the back branches ...
>=20
> > If we have a simple/clean patch to fix the "Ctrl-C ignored at password
> > prompt", such that, instead, psql exits more-or-less immediately then
> > I'd be for back-patching just that as it certainly has always felt like
> > a wart or even a bug to me, but I agree that we don't want to introduce
> > other behavior changes beyond that in back-branches.
>=20
> That would be commit 9099e4afe. I won't vote against back-patching if
> people think it's unlikely that anyone is expecting the old behavior.
I'm having a hard time seeing how someone would be depending on the old
behavior. I'm for back-patching that change but given that it's in the
client, I won't press the issue if someone actually feels like there's a
chance it'd break some existing usage.
Peter- any particular reason you didn't backpatch 9099e4afe? Were you
concerned that it would break something for people, or just didn't as it
wasn't asked for? (If so- please consider it now asked for.. :)
> > While we're at it, we should fix the complaint about .psql_history being
> > missing.. This isn't an error, imv:
> > could not save history to file "/home/sfrost/.psql_history": No such fi=
le or directory
>=20
> Eh? I've not seen anything like that personally. I seem to recall having
> heard of it happening with broken readline or libedit versions, but we
> don't have very much ability to work around bugs in those.
Oh? I get it every time I run psql w/o a .psql_history file on a stock
Ubuntu system which certainly has a current libreadline installed..
Will have to investigate a bit more.
Thanks,
Stephen
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Stephen Frost <sfrost@snowman.net> writes:
>>> While we're at it, we should fix the complaint about .psql_history being
>>> missing.. This isn't an error, imv:
>>> could not save history to file "/home/sfrost/.psql_history": No such file or directory
>> Eh? I've not seen anything like that personally. I seem to recall having
>> heard of it happening with broken readline or libedit versions, but we
>> don't have very much ability to work around bugs in those.
> Oh? I get it every time I run psql w/o a .psql_history file on a stock
> Ubuntu system which certainly has a current libreadline installed..
> Will have to investigate a bit more.
FWIW, I just rechecked and verified I see no such misbehavior with RHEL6's
libreadline, nor with OS X 10.9.4's libedit.
regards, tom lane
On 7/16/14 3:51 AM, Tom Lane wrote: > So I'm satisfied with the behavior of HEAD in this area. There's a case > to be made for back-patching commit 9099e4afe into older branches, but > I'm not sure if that'd be a good idea or not. People might be depending > on the old behavior. I can't envision how anyone could be depending on the old behaviour, but then again, I'm not very imaginative. > A different line of thought would be to reset cancel_pressed before > starting the processing of any script file, so that you actually have to > press control-C *during* the processing of a file to cancel it. That's > just narrowing the race condition window some more, but it might be > worth doing. If you feel like backpatching is too much, I think we should at least narrow the window down like this. There's still a chance for a major duck-up, but you'd have to get very unlucky to pull that off. .marko
On Tue, 2014-07-15 at 22:36 -0400, Stephen Frost wrote: > Peter- any particular reason you didn't backpatch 9099e4afe? Were you > concerned that it would break something for people, or just didn't as > it wasn't asked for? At the time it wasn't considered a bug fix, and I generally don't want to back patch behavioral changes in psql. It's reasonably easy to just install a new major version of the client.