Horton

Thread

Commits

  1. Improve psql's behavior when the editor is exited without saving.

  2. Fix race condition in psql \e's detection of file modification.

  1. Confusing behavior of psql's \e

    Laurenz Albe <laurenz.albe@cybertec.at> — 2020-12-01T03:38:35Z

    If you quit the editor without saving, the current query buffer
or the last executed SQL statement get run.

This can be annoying and disruptive, and it requires you to
empty the file and save it if you don't want to execute anything.

But when editing a script, it is a clear POLA violation:

test=> \! cat q.sql
SELECT 99;

test=> SELECT 42;
 ?column? 
----------
       42
(1 row)

test=> \e q.sql
[quit the editor without saving]
 ?column? 
----------
       42
(1 row)

This is pretty bad: you either have to re-run the previous statement
or you have to empty your script file.  Both are unappealing.

I have been annoyed about this myself, and I have heard other prople
complain about it, so I propose to clear the query buffer if the
editor exits without modifying the file.

This behavior is much more intuitive for me.

Yours,
Laurenz Albe

  2. Re: Confusing behavior of psql's \e

    Chapman Flack <chap@anastigmatix.net> — 2020-12-01T16:03:35Z

    On 11/30/20 22:38, Laurenz Albe wrote:

> I have been annoyed about this myself, and I have heard other prople
> complain about it, so I propose to clear the query buffer if the
> editor exits without modifying the file.

Or how about keeping the query buffer content unchanged, but not
executing it? Then it's still there if you have another idea about
editing it. It's easy enough to \r if you really want it gone.

One downside I can foresee is that I could form a habit of doing
something that would be safe in psql version x but would bite me
one day if I'm in psql version x-- for some reason.

Regards,
-Chap

  3. Re: Confusing behavior of psql's \e

    Laurenz Albe <laurenz.albe@cybertec.at> — 2020-12-01T16:21:11Z

    On Tue, 2020-12-01 at 11:03 -0500, Chapman Flack wrote:
> On 11/30/20 22:38, Laurenz Albe wrote:
> > I have been annoyed about this myself, and I have heard other prople
> > complain about it, so I propose to clear the query buffer if the
> > editor exits without modifying the file.
> 
> Or how about keeping the query buffer content unchanged, but not
> executing it? Then it's still there if you have another idea about
> editing it. It's easy enough to \r if you really want it gone.

What if the buffer was empty?  Would you want to get the previous
query in the query buffer?  I'd assume not...

Yours,
Laurenz Albe

  4. Re: Confusing behavior of psql's \e

    Chapman Flack <chap@anastigmatix.net> — 2020-12-01T16:34:19Z

    On 12/01/20 11:21, Laurenz Albe wrote:
> On Tue, 2020-12-01 at 11:03 -0500, Chapman Flack wrote:
>>> complain about it, so I propose to clear the query buffer if the
>>> editor exits without modifying the file.
>>
>> Or how about keeping the query buffer content unchanged, but not
>> executing it? Then it's still there if you have another idea about
>> editing it. It's easy enough to \r if you really want it gone.
> 
> What if the buffer was empty?  Would you want to get the previous
> query in the query buffer?  I'd assume not...

I took your proposal to be specifically about what happens if the editor
is exited with no change to the buffer, and in that case, I would suggest
making no change to the buffer, but not re-executing it.

If the editor is exited after deliberately emptying the editor buffer,
I would expect that to be treated as emptying the query buffer.

I don't foresee any case that would entail bringing a /previous/ query
back into the query buffer.

Regards,
-Chap

  5. Re: Confusing behavior of psql's \e

    Laurenz Albe <laurenz.albe@cybertec.at> — 2020-12-16T09:45:44Z

    On Tue, 2020-12-01 at 11:34 -0500, Chapman Flack wrote:
> On 12/01/20 11:21, Laurenz Albe wrote:
> > On Tue, 2020-12-01 at 11:03 -0500, Chapman Flack wrote:
> > > > I propose to clear the query buffer if the
> > > > editor exits without modifying the file.
> > > 
> > > Or how about keeping the query buffer content unchanged, but not
> > > executing it? Then it's still there if you have another idea about
> > > editing it. It's easy enough to \r if you really want it gone.
> > 
> > What if the buffer was empty?  Would you want to get the previous
> > query in the query buffer?  I'd assume not...
> 
> I took your proposal to be specifically about what happens if the editor
> is exited with no change to the buffer, and in that case, I would suggest
> making no change to the buffer, but not re-executing it.
> 
> If the editor is exited after deliberately emptying the editor buffer,
> I would expect that to be treated as emptying the query buffer.
> 
> I don't foresee any case that would entail bringing a /previous/ query
> back into the query buffer.

I see I'll have to try harder.

The attached patch changes the behavior as follows:

- If the current query buffer is edited, and you quit the editor,
  the query buffer is retained.  This is as it used to be.

- If the query buffer is empty and you run \e, the previous query
  is edited (as it used to be), but quitting the editor will empty
  the query buffer and execute nothing.

- Similarly, if you "\e file" and quit the editor, nothing will
  be executed and the query buffer is emptied.

- The same behavior change applies to \ef and \ev.
  There is no need to retain the definition in the query buffer,
  you can always run the \ev or \ev again.

I consider this a bug fix, but one that shouldn't be backpatched.
Re-executing the previous query if the editor is quit is
annoying at least and dangerous at worst.

I'll add this patch to the next commitfest.

Yours,
Laurenz Albe

  6. Re: Confusing behavior of psql's \e

    Jacob Champion <pchampion@vmware.com> — 2021-03-03T00:07:22Z

    On Wed, 2020-12-16 at 10:45 +0100, Laurenz Albe wrote:
> I consider this a bug fix, but one that shouldn't be backpatched.
> Re-executing the previous query if the editor is quit is
> annoying at least and dangerous at worst.

I like that this patch also clears the query buffer in the error case.
(For example, if I save the file but then decide I want to cancel
execution, the only choice is to issue an abortive :cq from Vim. The
current master-branch behavior is to just dump me back onto a
continuation prompt, and I have to manually \r the buffer. With this
patch, I'm returned to an initial prompt with a clear buffer. Very
nice.)

Some unexpected behavior I saw when testing this patch: occasionally I
would perform a bare \e, save the temporary file, and quit, only to
find that nothing was executed. What's happening is, I'm saving the
file too quickly, and the timestamp check (which has only second
precision) is failing! This isn't a problem in practice for the
explicit-filename case, because you probably didn't create the file
within the last second, but the temporary files are always zero seconds
old by definition. I could see this tripping up some people.

--Jacob

  7. Re: Confusing behavior of psql's \e

    Laurenz Albe <laurenz.albe@cybertec.at> — 2021-03-03T16:08:22Z

    Thanks for testing!

On Wed, 2021-03-03 at 00:07 +0000, Jacob Champion wrote:
> On Wed, 2020-12-16 at 10:45 +0100, Laurenz Albe wrote:
> 
> > I consider this a bug fix, but one that shouldn't be backpatched.
> > Re-executing the previous query if the editor is quit is
> > annoying at least and dangerous at worst.
> 
> I like that this patch also clears the query buffer in the error case.
> (For example, if I save the file but then decide I want to cancel
> execution, the only choice is to issue an abortive :cq from Vim. The
> current master-branch behavior is to just dump me back onto a
> continuation prompt, and I have to manually \r the buffer. With this
> patch, I'm returned to an initial prompt with a clear buffer. Very
> nice.)
> 
> Some unexpected behavior I saw when testing this patch: occasionally I
> would perform a bare \e, save the temporary file, and quit, only to
> find that nothing was executed. What's happening is, I'm saving the
> file too quickly, and the timestamp check (which has only second
> precision) is failing! This isn't a problem in practice for the
> explicit-filename case, because you probably didn't create the file
> within the last second, but the temporary files are always zero seconds
> old by definition. I could see this tripping up some people.

That is because psql compares the file modification time before and
after the edit, and the "st_mtime" in "struct stat" has a precision of
seconds.  Some operating systems and file provide a finer granularity,
but for example PostgreSQL's _pgstat64 that is used on Windows doesn't.

This is no new behavior, and I think this is rare enough that we don't
have to bother.  I had to define a vim macro to :wq the file fast
enough to reproduce your observation...

Yours,
Laurenz Albe

  8. Re: Confusing behavior of psql's \e

    Jacob Champion <pchampion@vmware.com> — 2021-03-03T17:12:22Z

    On Wed, 2021-03-03 at 17:08 +0100, Laurenz Albe wrote:
> This is no new behavior, and I think this is rare enough that we don't
> have to bother.

I agree that it's not new behavior, but this patch exposes that
behavior for the temporary file case, because you've fixed the bug that
caused the timestamp check to not matter. As far as I can tell, you
can't run into that race on the master branch for temporary files, and
you won't run into it in practice for explicit filenames.

--Jacob

  9. Re: Confusing behavior of psql's \e

    Laurenz Albe <laurenz.albe@cybertec.at> — 2021-03-04T16:41:21Z

    On Wed, 2021-03-03 at 17:12 +0000, Jacob Champion wrote:
> On Wed, 2021-03-03 at 17:08 +0100, Laurenz Albe wrote:
> > This is no new behavior, and I think this is rare enough that we don't
> > have to bother.
> 
> I agree that it's not new behavior, but this patch exposes that
> behavior for the temporary file case, because you've fixed the bug that
> caused the timestamp check to not matter. As far as I can tell, you
> can't run into that race on the master branch for temporary files, and
> you won't run into it in practice for explicit filenames.

Actually, the timestamp check *did* matter before.
The code in "do_edit" has:

[after the editor has been called]
if (!error && before.st_mtime != after.st_mtime)
{
    [read file back into query_buf]
}

This is pre-existing code.  I just added an else branch:

else
{
    [discard query_buf if we were editing a script, function or view]
}

So if you do your "modify and save the file in less than a second"
trick with the old code, you would end up with the old, unmodified
data in the query buffer.

I would say that the old behavior is worse in that case, and
discarding the query buffer is better.

I am not against fixing or improving the behavior, but given the
fact that we can't portably get less than second precision, it
seems impossible.  For good measure, I have added a check if the
file size has changed.

I don't think we can or have to do better than that.
Few people are skilled enough to modify and save a file in less
than a second, and I don't think there have been complaints
about that behavior so far.

Attached is version 2 of the patch, with the added file size
check and a pgindent run.

Yours,
Laurenz Albe

  10. Re: Confusing behavior of psql's \e

    Jacob Champion <pchampion@vmware.com> — 2021-03-04T16:51:55Z

    On Thu, 2021-03-04 at 17:41 +0100, Laurenz Albe wrote:
> So if you do your "modify and save the file in less than a second"
> trick with the old code, you would end up with the old, unmodified
> data in the query buffer.

Sorry, I was unclear in my first post -- I'm not modifying the
temporary file. Just saving and quitting with :wq, which is much easier
to do in less than a second.

> I would say that the old behavior is worse in that case, and
> discarding the query buffer is better.

For the case where you've modified the buffer, I agree, and I'm not
arguing otherwise.

> I am not against fixing or improving the behavior, but given the
> fact that we can't portably get less than second precision, it
> seems impossible.

You could backdate the temporary file, so that any save is guaranteed
to move the timestamp forward. That should work even if the filesystem
has extremely poor precision.

--Jacob

  11. Re: Confusing behavior of psql's \e

    Laurenz Albe <laurenz.albe@cybertec.at> — 2021-03-05T12:38:32Z

    On Thu, 2021-03-04 at 16:51 +0000, Jacob Champion wrote:
> > I am not against fixing or improving the behavior, but given the
> > fact that we can't portably get less than second precision, it
> > seems impossible.
> 
> You could backdate the temporary file, so that any save is guaranteed
> to move the timestamp forward. That should work even if the filesystem
> has extremely poor precision.

Ah, of course, that is the way to go.
Attached is version 3 which does it like this.

Yours,
Laurenz Albe

  12. Re: Confusing behavior of psql's \e

    Jacob Champion <pchampion@vmware.com> — 2021-03-08T20:44:28Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            tested, passed

Very nice quality-of-life improvement. Thanks!

The new status of this patch is: Ready for Committer

  13. Re: Confusing behavior of psql's \e

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-10T05:13:58Z

    Laurenz Albe <laurenz.albe@cybertec.at> writes:
> On Thu, 2021-03-04 at 16:51 +0000, Jacob Champion wrote:
>> You could backdate the temporary file, so that any save is guaranteed
>> to move the timestamp forward. That should work even if the filesystem
>> has extremely poor precision.

> Ah, of course, that is the way to go.

I took a quick look at this.  I don't have an opinion yet about the
question of changing the when-to-discard-the-buffer rules, but I agree
that trying to get rid of the race condition inherent in the existing
file mtime test would be a good idea.  However, I've got some
portability-related gripes about how you are doing the latter:

1. There is no principled reason to assume that the epoch date is in the
past.  IIRC, Postgres' timestamp epoch of 2000-01-01 was in the future
at the time we set it.  More relevant to the immediate issue, I clearly
recall a discussion at Red Hat in which one of the principal glibc
maintainers (likely Ulrich Drepper, though I'm not quite sure) argued
that 32-bit time_t could be used indefinitely by redefining the epoch
forward by 2^32 seconds every often; which would require intervals of
circa 68 years in which time_t was seen as a negative offset from a
future epoch date, rather than an unsigned offset from a past date.
Now, I thought he was nuts then and I still think that 32-bit hardware
will be ancient history by 2038 ... but there may be systems that do it
like that.  glibc hates ABI breakage.

2. Putting an enormously old date on a file that was just created will
greatly confuse onlookers, some of whom (such as backup or antivirus
daemons) might not react pleasantly.

Between #1 and #2, it's clearly worth the extra one or two lines of
code to set the file dates to, say, "time(NULL) - 1", rather than
assuming that zero is good enough.

3. I wonder about the portability of utime(2).  I see that we are using
it to cause updates of socket and lock file times, but our expectations
for it there are rock-bottom low.  I think that failing the edit command
if utime() fails is an overreaction even with optimistic assumptions about
its reliability.  Doing that makes things strictly worse than simply not
doing anything, because 99% of the time this refinement is unnecessary.

In short, I think the relevant code ought to be more like

	else
	{
	    struct utimbuf ut;

	    ut.modtime = ut.actime = time(NULL) - 1;
	    (void) utime(fname, &ut);
	}

(plus some comments of course)

			regards, tom lane

PS: I seem to recall that some Microsoft filesystems have 2-second
resolution on file mod times, so maybe it needs to be "time(NULL) - 2"?

  14. Re: Confusing behavior of psql's \e

    Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> — 2021-03-10T10:16:18Z

    On Wed, Mar 10, 2021 at 6:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

>
> PS: I seem to recall that some Microsoft filesystems have 2-second
> resolution on file mod times, so maybe it needs to be "time(NULL) - 2"?
>

You are thinking about FAT:

https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfiletime#remarks

Regards,

Juan José Santamaría Flecha

  15. Re: Confusing behavior of psql's \e

    Laurenz Albe <laurenz.albe@cybertec.at> — 2021-03-10T10:32:02Z

    On Wed, 2021-03-10 at 00:13 -0500, Tom Lane wrote:
> I agree
> that trying to get rid of the race condition inherent in the existing
> file mtime test would be a good idea.  However, I've got some
> portability-related gripes about how you are doing the latter:
> 
> 1. There is no principled reason to assume that the epoch date is in the
> past.  IIRC, Postgres' timestamp epoch of 2000-01-01 was in the future
> at the time we set it.  More relevant to the immediate issue, I clearly
> recall a discussion at Red Hat in which one of the principal glibc
> maintainers (likely Ulrich Drepper, though I'm not quite sure) argued
> that 32-bit time_t could be used indefinitely by redefining the epoch
> forward by 2^32 seconds every often; which would require intervals of
> circa 68 years in which time_t was seen as a negative offset from a
> future epoch date, rather than an unsigned offset from a past date.
> Now, I thought he was nuts then and I still think that 32-bit hardware
> will be ancient history by 2038 ... but there may be systems that do it
> like that.  glibc hates ABI breakage.
> 
> 2. Putting an enormously old date on a file that was just created will
> greatly confuse onlookers, some of whom (such as backup or antivirus
> daemons) might not react pleasantly.
> 
> Between #1 and #2, it's clearly worth the extra one or two lines of
> code to set the file dates to, say, "time(NULL) - 1", rather than
> assuming that zero is good enough.
> 
> 3. I wonder about the portability of utime(2).  I see that we are using
> it to cause updates of socket and lock file times, but our expectations
> for it there are rock-bottom low.  I think that failing the edit command
> if utime() fails is an overreaction even with optimistic assumptions about
> its reliability.  Doing that makes things strictly worse than simply not
> doing anything, because 99% of the time this refinement is unnecessary.
> 
> In short, I think the relevant code ought to be more like
> 
> 	else
> 	{
> 	    struct utimbuf ut;
> 
> 	    ut.modtime = ut.actime = time(NULL) - 1;
> 	    (void) utime(fname, &ut);
> 	}
> 
> (plus some comments of course)
> 
> 			regards, tom lane
> 
> PS: I seem to recall that some Microsoft filesystems have 2-second
> resolution on file mod times, so maybe it needs to be "time(NULL) - 2"?

Thanks for taking a look!

Taken together, these arguments are convincing.

Done like that in the attached patch version 4.

Yours,
Laurenz Albe

  16. Re: Confusing behavior of psql's \e

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-12T18:12:46Z

    Laurenz Albe <laurenz.albe@cybertec.at> writes:
> Done like that in the attached patch version 4.

I pushed the race-condition-fixing part of this, since that's an
unarguable bug fix and hence seems OK to back-patch.  (I added a
check on change of file size, because why not.)

Attached is the rest, just to keep the cfbot happy.

I don't think this is quite committable yet.  The division of
labor between do_edit() and its callers seems to need more
thought: in particular, I see that \ef now fails to print
"No changes" when I would expect it to.  But the real question
is whether there is any non-error condition in which do_edit
should not set the query_buffer, either to the edit result
or empty.  If we're going to improve the header comment for
do_edit, I would expect it to specify what happens to the
query_buf, and the fact that I can't write a concise spec
leads me to think that a bit more design effort is needed.

Also, somewhat cosmetic, but: I feel like the hunk that is
setting discard_on_quit in exec_command_edit is assuming
more than it ought to about what copy_previous_query will do.
Perhaps it's worth making copy_previous_query return a bool
indicating whether it copied previous_buf, and then
exec_command_edit becomes

	discard_on_quit = copy_previous_query(query_buf, previous_buf);

			regards, tom lane

  17. Re: Confusing behavior of psql's \e

    Laurenz Albe <laurenz.albe@cybertec.at> — 2021-03-15T16:26:41Z

    On Fri, 2021-03-12 at 13:12 -0500, Tom Lane wrote:
> I pushed the race-condition-fixing part of this, since that's an
> unarguable bug fix and hence seems OK to back-patch.  (I added a
> check on change of file size, because why not.)

Thank you!

> Attached is the rest, just to keep the cfbot happy.

Thanks for that as well.

> I don't think this is quite committable yet.  The division of
> labor between do_edit() and its callers seems to need more
> thought: in particular, I see that \ef now fails to print
> "No changes" when I would expect it to.

Hm.  My idea was that "No changes" is short for "No changes to
the query buffer" and is printed only if you quit the editor,
but the query buffer is retained.

But it also makes sense to interpret it as "No changes to the
function or view definition", so I have put it back according
to your expectations.

> But the real question
> is whether there is any non-error condition in which do_edit
> should not set the query_buffer, either to the edit result
> or empty.

I don't follow.  That's exactly what happens...
But I guess the confusion is due to my inadequate comments.

> If we're going to improve the header comment for
> do_edit, I would expect it to specify what happens to the
> query_buf, and the fact that I can't write a concise spec
> leads me to think that a bit more design effort is needed.

I have described the fate of the query buffer in the function
comment.  I hope it is clearer now.

> Also, somewhat cosmetic, but: I feel like the hunk that is
> setting discard_on_quit in exec_command_edit is assuming
> more than it ought to about what copy_previous_query will do.
> Perhaps it's worth making copy_previous_query return a bool
> indicating whether it copied previous_buf, and then
> exec_command_edit becomes
> 
>         discard_on_quit = copy_previous_query(query_buf, previous_buf);

That is a clear improvement, and I have done it like that.


Perhaps I should restate the problem the patch is trying to solve:

test=> INSERT INTO dummy VALUES (DEFAULT);
INSERT 0 1
test=> -- quit the editor during the following
test=> \e script.sql
INSERT 0 1

Ouch.  A second line was inserted into "dummy".

The same happens with plain \e: if I edit the previous query
and quit, I don't expect it to be executed again.
Currently, the only way to achieve that is to empty the temporary
file and then save it, which is annoying.


Attached is version 6.

Yours,
Laurenz Albe

  18. Re: Confusing behavior of psql's \e

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-03T21:43:57Z

    Laurenz Albe <laurenz.albe@cybertec.at> writes:
> Attached is version 6.

Pushed with some mostly-cosmetic fiddling.

One thing I changed that wasn't cosmetic is that as you had it,
the behavior of "\e file" varied depending on whether the query
buffer had been empty, which surely seems like a bad idea.
I made it do discard_on_quit always in that case.  I think there
might be a case for discard_on_quit = false always, ie maybe
the user wanted to load the file into the query buffer as-is.
But it seems like a pretty weak case --- you'd be more likely
to just use \i for that situation.

			regards, tom lane

  19. Re: Confusing behavior of psql's \e

    Laurenz Albe <laurenz.albe@cybertec.at> — 2021-04-06T09:17:58Z

    On Sat, 2021-04-03 at 17:43 -0400, Tom Lane wrote:
> Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > Attached is version 6.
> 
> Pushed with some mostly-cosmetic fiddling.
> 
> One thing I changed that wasn't cosmetic is that as you had it,
> the behavior of "\e file" varied depending on whether the query
> buffer had been empty, which surely seems like a bad idea.
> I made it do discard_on_quit always in that case.  I think there
> might be a case for discard_on_quit = false always, ie maybe
> the user wanted to load the file into the query buffer as-is.
> But it seems like a pretty weak case --- you'd be more likely
> to just use \i for that situation.

Thanks for that!

Yours,
Laurenz Albe