From: | Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Jacob Champion <pchampion(at)vmware(dot)com>, "chap(at)anastigmatix(dot)net" <chap(at)anastigmatix(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Confusing behavior of psql's \e |
Date: | 2021-03-15 16:26:41 |
Message-ID: | cd66669eaed24aadf294c2b552104bc630125217.camel@cybertec.at |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
0001-Improve-e-ef-and-ev-if-the-editor-is-quit.v6.patch | text/x-patch | 7.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Mark Dilger | 2021-03-15 16:30:43 | Re: REINDEX backend filtering |
Previous Message | Peter Geoghegan | 2021-03-15 16:18:21 | Re: Postgres crashes at memcopy() after upgrade to PG 13. |