Confusing behavior of psql's \e
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
Attachments:
0001-Discard-query-buffer-if-editor-is-quit-in-e.patchtext/x-patch; charset=UTF-8; name=0001-Discard-query-buffer-if-editor-is-quit-in-e.patchDownload
From 209e4a0ab51f88a82e1fc0d4e4efd24d38a7d26c Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Tue, 1 Dec 2020 04:28:50 +0100
Subject: [PATCH] Discard query buffer if editor is quit in \e
Before, the current query buffer or the previous query was executed
when you quit the editor without saving.
This was frequently annoying, but downright confusing if \e was used
to edit a script file, because it would then execute the previous
command rather than the script file.
---
doc/src/sgml/ref/psql-ref.sgml | 4 +++-
src/bin/psql/command.c | 2 ++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 221a967bfe..002ed38fe7 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1949,7 +1949,9 @@ testdb=>
</para>
<para>
- The new contents of the query buffer are then re-parsed according to
+ If the editor is quit without modifying the file, the query buffer
+ is cleared.
+ Otherwise, the new contents of the query buffer are re-parsed according to
the normal rules of <application>psql</application>, treating the
whole buffer as a single line. Any complete queries are immediately
executed; that is, if the query buffer contains or ends with a
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index c7a83d5dfc..ffc5d209bc 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3838,6 +3838,8 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
fclose(stream);
}
}
+ else
+ resetPQExpBuffer(query_buf);
/* remove temp file */
if (!filename_arg)
--
2.26.2
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
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
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
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
Attachments:
0001-Improve-e-ef-and-ev-if-the-editor-is-quit.patchtext/x-patch; charset=UTF-8; name=0001-Improve-e-ef-and-ev-if-the-editor-is-quit.patchDownload
From ecbf6cb81282ea5646b087290ca60a50daabe22c Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Wed, 16 Dec 2020 10:34:14 +0100
Subject: [PATCH] Improve \e, \ef and \ev if the editor is quit
Before, quitting the editor without saving would cause
the *previous query* to be executed, which is certainly not
what anyone would expect.
It is better to execute nothing and clear the query buffer
in this case.
The exception is if the current query buffer is editied:
in this case, the behavior is unclanged, and the query buffer
is retained.
Reviewed-by: Chapman Flack
Discussion: https://postgr.es/m/0ba3f2a658bac6546d9934ab6ba63a805d46a49b.camel%40cybertec.at
---
doc/src/sgml/ref/psql-ref.sgml | 10 +++++---
src/bin/psql/command.c | 43 +++++++++++++++++++++++-----------
2 files changed, 36 insertions(+), 17 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 221a967bfe..44eebbf141 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1949,7 +1949,9 @@ testdb=>
</para>
<para>
- The new contents of the query buffer are then re-parsed according to
+ If you edit a file or the previous query, and you quit the editor without
+ modifying the file, the query buffer is cleared.
+ Otherwise, the new contents of the query buffer are re-parsed according to
the normal rules of <application>psql</application>, treating the
whole buffer as a single line. Any complete queries are immediately
executed; that is, if the query buffer contains or ends with a
@@ -2018,7 +2020,8 @@ Tue Oct 26 21:40:57 CEST 1999
in the form of a <command>CREATE OR REPLACE FUNCTION</command> or
<command>CREATE OR REPLACE PROCEDURE</command> command.
Editing is done in the same way as for <literal>\edit</literal>.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
if you added a semicolon to it. Otherwise it is redisplayed;
type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal>
to cancel.
@@ -2094,7 +2097,8 @@ Tue Oct 26 21:40:57 CEST 1999
This command fetches and edits the definition of the named view,
in the form of a <command>CREATE OR REPLACE VIEW</command> command.
Editing is done in the same way as for <literal>\edit</literal>.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
if you added a semicolon to it. Otherwise it is redisplayed;
type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal>
to cancel.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 38b588882d..15d7c343e9 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -151,7 +151,7 @@ static void copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf)
static bool do_connect(enum trivalue reuse_previous_specification,
char *dbname, char *user, char *host, char *port);
static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
- int lineno, bool *edited);
+ int lineno, bool *edited, bool discard_on_quit);
static bool do_shell(const char *command);
static bool do_watch(PQExpBuffer query_buf, double sleep);
static bool lookup_object_oid(EditableObjectType obj_type, const char *desc,
@@ -1000,6 +1000,13 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
}
if (status != PSQL_CMD_ERROR)
{
+ /*
+ * If the query buffer is empty, we'll edit the previous query.
+ * But in that case, we don't want to keep that if the editor
+ * is quit.
+ */
+ bool discard_on_quit = (query_buf->len == 0);
+
expand_tilde(&fname);
if (fname)
canonicalize_path(fname);
@@ -1007,7 +1014,7 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
/* If query_buf is empty, recall previous query for editing */
copy_previous_query(query_buf, previous_buf);
- if (do_edit(fname, query_buf, lineno, NULL))
+ if (do_edit(fname, query_buf, lineno, NULL, discard_on_quit))
status = PSQL_CMD_NEWEDIT;
else
status = PSQL_CMD_ERROR;
@@ -1130,11 +1137,9 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch,
{
bool edited = false;
- if (!do_edit(NULL, query_buf, lineno, &edited))
+ if (!do_edit(NULL, query_buf, lineno, &edited, true))
status = PSQL_CMD_ERROR;
- else if (!edited)
- puts(_("No changes"));
- else
+ else if (edited)
status = PSQL_CMD_NEWEDIT;
}
@@ -3632,12 +3637,7 @@ UnsyncVariables(void)
}
-/*
- * do_edit -- handler for \e
- *
- * If you do not specify a filename, the current query buffer will be copied
- * into a temporary one.
- */
+/* helper for do_edit() */
static bool
editFile(const char *fname, int lineno)
{
@@ -3705,10 +3705,18 @@ editFile(const char *fname, int lineno)
}
-/* call this one */
+/*
+ * do_edit -- handler for \e
+ *
+ * If you do not specify a filename, the current query buffer will be copied
+ * into a temporary one.
+ * "edited" will be set to true if the file is modified.
+ * If "discard_on_quit" is true, discard the query buffer if the file was
+ * not modified.
+ */
static bool
do_edit(const char *filename_arg, PQExpBuffer query_buf,
- int lineno, bool *edited)
+ int lineno, bool *edited, bool discard_on_quit)
{
char fnametmp[MAXPGPATH];
FILE *stream = NULL;
@@ -3845,6 +3853,13 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
fclose(stream);
}
}
+ else
+ {
+ if (discard_on_quit)
+ resetPQExpBuffer(query_buf);
+ else
+ puts(_("No changes"));
+ }
/* remove temp file */
if (!filename_arg)
--
2.26.2
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
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
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
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
Attachments:
0001-Improve-e-ef-and-ev-if-the-editor-is-quit.v2.patchtext/x-patch; charset=UTF-8; name=0001-Improve-e-ef-and-ev-if-the-editor-is-quit.v2.patchDownload
From 8dc37cc9e775032f8f95cb837760dfe5463fc343 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Thu, 4 Mar 2021 17:33:59 +0100
Subject: [PATCH] Improve \e, \ef and \ev if the editor is quit
Before, quitting the editor without saving would cause
the *previous query* to be executed, which is certainly not
what anyone would expect.
It is better to execute nothing and clear the query buffer
in this case.
The exception is if the current query buffer is editied:
in this case, the behavior is unclanged, and the query buffer
is retained.
Author: Laurenz Albe <laurenz.albe@cybertec.at>
Reviewed-by: Chapman Flack, Jacob Champion
Discussion: https://postgr.es/m/0ba3f2a658bac6546d9934ab6ba63a805d46a49b.camel%40cybertec.at
---
doc/src/sgml/ref/psql-ref.sgml | 10 ++++---
src/bin/psql/command.c | 49 +++++++++++++++++++++++-----------
2 files changed, 41 insertions(+), 18 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 13c1edfa4d..0bf69174ff 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1970,7 +1970,9 @@ testdb=>
</para>
<para>
- The new contents of the query buffer are then re-parsed according to
+ If you edit a file or the previous query, and you quit the editor without
+ modifying the file, the query buffer is cleared.
+ Otherwise, the new contents of the query buffer are re-parsed according to
the normal rules of <application>psql</application>, treating the
whole buffer as a single line. Any complete queries are immediately
executed; that is, if the query buffer contains or ends with a
@@ -2039,7 +2041,8 @@ Tue Oct 26 21:40:57 CEST 1999
in the form of a <command>CREATE OR REPLACE FUNCTION</command> or
<command>CREATE OR REPLACE PROCEDURE</command> command.
Editing is done in the same way as for <literal>\edit</literal>.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
if you added a semicolon to it. Otherwise it is redisplayed;
type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal>
to cancel.
@@ -2115,7 +2118,8 @@ Tue Oct 26 21:40:57 CEST 1999
This command fetches and edits the definition of the named view,
in the form of a <command>CREATE OR REPLACE VIEW</command> command.
Editing is done in the same way as for <literal>\edit</literal>.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
if you added a semicolon to it. Otherwise it is redisplayed;
type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal>
to cancel.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index c98e3d31d0..2264ed5bab 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -151,7 +151,7 @@ static void copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf)
static bool do_connect(enum trivalue reuse_previous_specification,
char *dbname, char *user, char *host, char *port);
static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
- int lineno, bool *edited);
+ int lineno, bool *edited, bool discard_on_quit);
static bool do_shell(const char *command);
static bool do_watch(PQExpBuffer query_buf, double sleep);
static bool lookup_object_oid(EditableObjectType obj_type, const char *desc,
@@ -1003,6 +1003,13 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
}
if (status != PSQL_CMD_ERROR)
{
+ /*
+ * If the query buffer is empty, we'll edit the previous
+ * query. But in that case, we don't want to keep that if the
+ * editor is quit.
+ */
+ bool discard_on_quit = (query_buf->len == 0);
+
expand_tilde(&fname);
if (fname)
canonicalize_path(fname);
@@ -1010,7 +1017,7 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
/* If query_buf is empty, recall previous query for editing */
copy_previous_query(query_buf, previous_buf);
- if (do_edit(fname, query_buf, lineno, NULL))
+ if (do_edit(fname, query_buf, lineno, NULL, discard_on_quit))
status = PSQL_CMD_NEWEDIT;
else
status = PSQL_CMD_ERROR;
@@ -1133,11 +1140,9 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch,
{
bool edited = false;
- if (!do_edit(NULL, query_buf, lineno, &edited))
+ if (!do_edit(NULL, query_buf, lineno, &edited, true))
status = PSQL_CMD_ERROR;
- else if (!edited)
- puts(_("No changes"));
- else
+ else if (edited)
status = PSQL_CMD_NEWEDIT;
}
@@ -3626,12 +3631,7 @@ UnsyncVariables(void)
}
-/*
- * do_edit -- handler for \e
- *
- * If you do not specify a filename, the current query buffer will be copied
- * into a temporary one.
- */
+/* helper for do_edit() */
static bool
editFile(const char *fname, int lineno)
{
@@ -3699,10 +3699,18 @@ editFile(const char *fname, int lineno)
}
-/* call this one */
+/*
+ * do_edit -- handler for \e
+ *
+ * If you do not specify a filename, the current query buffer will be copied
+ * into a temporary one.
+ * "edited" will be set to true if the file is modified.
+ * If "discard_on_quit" is true, discard the query buffer if the file was
+ * not modified.
+ */
static bool
do_edit(const char *filename_arg, PQExpBuffer query_buf,
- int lineno, bool *edited)
+ int lineno, bool *edited, bool discard_on_quit)
{
char fnametmp[MAXPGPATH];
FILE *stream = NULL;
@@ -3809,7 +3817,10 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
error = true;
}
- if (!error && before.st_mtime != after.st_mtime)
+ /* consider the file edited if the size or modification time differ */
+ if (!error &&
+ (before.st_size != after.st_size ||
+ before.st_mtime != after.st_mtime))
{
stream = fopen(fname, PG_BINARY_R);
if (!stream)
@@ -3839,6 +3850,14 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
fclose(stream);
}
}
+ else
+ {
+ /* if we were editing anything else than the query buffer, discard */
+ if (discard_on_quit)
+ resetPQExpBuffer(query_buf);
+ else
+ puts(_("No changes"));
+ }
/* remove temp file */
if (!filename_arg)
--
2.26.2
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
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
Attachments:
0001-Improve-e-ef-and-ev-if-the-editor-is-quit.v3.patchtext/x-patch; charset=UTF-8; name=0001-Improve-e-ef-and-ev-if-the-editor-is-quit.v3.patchDownload
From 7ff01271cb8ea5c9011a57ed613026ec7511d785 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Fri, 5 Mar 2021 13:34:12 +0100
Subject: [PATCH] Improve \e, \ef and \ev if the editor is quit
Before, if you edited a function or view definition or a
script file, quitting the editor without saving would cause
the *previous query* to be executed, which is certainly not
what anyone would expect.
It is better to execute nothing and clear the query buffer
in this case.
The behavior when editing the query buffer is unchanged:
quitting without saving will retain the query buffer.
Author: Laurenz Albe <laurenz.albe@cybertec.at>
Reviewed-by: Chapman Flack, Jacob Champion
Discussion: https://postgr.es/m/0ba3f2a658bac6546d9934ab6ba63a805d46a49b.camel%40cybertec.at
---
doc/src/sgml/ref/psql-ref.sgml | 10 ++++--
src/bin/psql/command.c | 56 +++++++++++++++++++++++++---------
2 files changed, 49 insertions(+), 17 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 13c1edfa4d..0bf69174ff 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1970,7 +1970,9 @@ testdb=>
</para>
<para>
- The new contents of the query buffer are then re-parsed according to
+ If you edit a file or the previous query, and you quit the editor without
+ modifying the file, the query buffer is cleared.
+ Otherwise, the new contents of the query buffer are re-parsed according to
the normal rules of <application>psql</application>, treating the
whole buffer as a single line. Any complete queries are immediately
executed; that is, if the query buffer contains or ends with a
@@ -2039,7 +2041,8 @@ Tue Oct 26 21:40:57 CEST 1999
in the form of a <command>CREATE OR REPLACE FUNCTION</command> or
<command>CREATE OR REPLACE PROCEDURE</command> command.
Editing is done in the same way as for <literal>\edit</literal>.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
if you added a semicolon to it. Otherwise it is redisplayed;
type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal>
to cancel.
@@ -2115,7 +2118,8 @@ Tue Oct 26 21:40:57 CEST 1999
This command fetches and edits the definition of the named view,
in the form of a <command>CREATE OR REPLACE VIEW</command> command.
Editing is done in the same way as for <literal>\edit</literal>.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
if you added a semicolon to it. Otherwise it is redisplayed;
type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal>
to cancel.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index c98e3d31d0..bd01d59d96 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -10,6 +10,7 @@
#include <ctype.h>
#include <time.h>
#include <pwd.h>
+#include <utime.h>
#ifndef WIN32
#include <sys/stat.h> /* for stat() */
#include <fcntl.h> /* open() flags */
@@ -151,7 +152,7 @@ static void copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf)
static bool do_connect(enum trivalue reuse_previous_specification,
char *dbname, char *user, char *host, char *port);
static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
- int lineno, bool *edited);
+ int lineno, bool *edited, bool discard_on_quit);
static bool do_shell(const char *command);
static bool do_watch(PQExpBuffer query_buf, double sleep);
static bool lookup_object_oid(EditableObjectType obj_type, const char *desc,
@@ -1003,6 +1004,13 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
}
if (status != PSQL_CMD_ERROR)
{
+ /*
+ * If the query buffer is empty, we'll edit the previous
+ * query. But in that case, we don't want to keep that if the
+ * editor is quit.
+ */
+ bool discard_on_quit = (query_buf->len == 0);
+
expand_tilde(&fname);
if (fname)
canonicalize_path(fname);
@@ -1010,7 +1018,7 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
/* If query_buf is empty, recall previous query for editing */
copy_previous_query(query_buf, previous_buf);
- if (do_edit(fname, query_buf, lineno, NULL))
+ if (do_edit(fname, query_buf, lineno, NULL, discard_on_quit))
status = PSQL_CMD_NEWEDIT;
else
status = PSQL_CMD_ERROR;
@@ -1133,11 +1141,9 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch,
{
bool edited = false;
- if (!do_edit(NULL, query_buf, lineno, &edited))
+ if (!do_edit(NULL, query_buf, lineno, &edited, true))
status = PSQL_CMD_ERROR;
- else if (!edited)
- puts(_("No changes"));
- else
+ else if (edited)
status = PSQL_CMD_NEWEDIT;
}
@@ -3626,12 +3632,7 @@ UnsyncVariables(void)
}
-/*
- * do_edit -- handler for \e
- *
- * If you do not specify a filename, the current query buffer will be copied
- * into a temporary one.
- */
+/* helper for do_edit() */
static bool
editFile(const char *fname, int lineno)
{
@@ -3699,10 +3700,18 @@ editFile(const char *fname, int lineno)
}
-/* call this one */
+/*
+ * do_edit -- handler for \e
+ *
+ * If you do not specify a filename, the current query buffer will be copied
+ * into a temporary one.
+ * "edited" will be set to true if the file is modified.
+ * If "discard_on_quit" is true, discard the query buffer if the file was
+ * not modified.
+ */
static bool
do_edit(const char *filename_arg, PQExpBuffer query_buf,
- int lineno, bool *edited)
+ int lineno, bool *edited, bool discard_on_quit)
{
char fnametmp[MAXPGPATH];
FILE *stream = NULL;
@@ -3763,6 +3772,8 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
else
{
unsigned int ql = query_buf->len;
+ /* old modification timestamp for the temporary file */
+ struct utimbuf epoch = { 0, 0 };
/* force newline-termination of what we send to editor */
if (ql > 0 && query_buf->data[ql - 1] != '\n')
@@ -3790,6 +3801,14 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
pg_log_error("%s: %m", fname);
error = true;
}
+ /* set the modification timestamp to the epoch */
+ else if (utime(fname, &epoch) != 0)
+ {
+ pg_log_error("%s: %m", fname);
+ if (remove(fname) != 0)
+ pg_log_error("%s: %m", fname);
+ error = true;
+ }
}
}
@@ -3809,6 +3828,7 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
error = true;
}
+ /* file was edited if the modification time is changed */
if (!error && before.st_mtime != after.st_mtime)
{
stream = fopen(fname, PG_BINARY_R);
@@ -3839,6 +3859,14 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
fclose(stream);
}
}
+ else
+ {
+ /* if we were editing anything else than the query buffer, discard */
+ if (discard_on_quit)
+ resetPQExpBuffer(query_buf);
+ else
+ puts(_("No changes"));
+ }
/* remove temp file */
if (!filename_arg)
--
2.26.2
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
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"?
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
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
Attachments:
0001-Improve-e-ef-and-ev-if-the-editor-is-quit.v4.patchtext/x-patch; charset=UTF-8; name=0001-Improve-e-ef-and-ev-if-the-editor-is-quit.v4.patchDownload
From 31257c2e697421bbcc08bdd546ccc8729654ccb3 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Wed, 10 Mar 2021 11:28:20 +0100
Subject: [PATCH] Improve \e, \ef and \ev if the editor is quit
Before, if you edited a function or view definition or a
script file, quitting the editor without saving would cause
the *previous query* to be executed, which is certainly not
what anyone would expect.
It is better to execute nothing and clear the query buffer
in this case.
The behavior when editing the query buffer is unchanged:
quitting without saving will retain the query buffer.
This patch also fixes an old race condition: due to the low
granularity that stat(2) can measure for the modification
time, an edit that took less than a second might go
unnoticed.
In passing, fix the misplaced function comment for do_edit().
Author: Laurenz Albe <laurenz.albe@cybertec.at>
Reviewed-by: Chapman Flack, Jacob Champion
Discussion: https://postgr.es/m/0ba3f2a658bac6546d9934ab6ba63a805d46a49b.camel%40cybertec.at
---
doc/src/sgml/ref/psql-ref.sgml | 10 ++++--
src/bin/psql/command.c | 64 ++++++++++++++++++++++++++--------
2 files changed, 57 insertions(+), 17 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 13c1edfa4d..0bf69174ff 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1970,7 +1970,9 @@ testdb=>
</para>
<para>
- The new contents of the query buffer are then re-parsed according to
+ If you edit a file or the previous query, and you quit the editor without
+ modifying the file, the query buffer is cleared.
+ Otherwise, the new contents of the query buffer are re-parsed according to
the normal rules of <application>psql</application>, treating the
whole buffer as a single line. Any complete queries are immediately
executed; that is, if the query buffer contains or ends with a
@@ -2039,7 +2041,8 @@ Tue Oct 26 21:40:57 CEST 1999
in the form of a <command>CREATE OR REPLACE FUNCTION</command> or
<command>CREATE OR REPLACE PROCEDURE</command> command.
Editing is done in the same way as for <literal>\edit</literal>.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
if you added a semicolon to it. Otherwise it is redisplayed;
type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal>
to cancel.
@@ -2115,7 +2118,8 @@ Tue Oct 26 21:40:57 CEST 1999
This command fetches and edits the definition of the named view,
in the form of a <command>CREATE OR REPLACE VIEW</command> command.
Editing is done in the same way as for <literal>\edit</literal>.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
if you added a semicolon to it. Otherwise it is redisplayed;
type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal>
to cancel.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index c98e3d31d0..d1e091b8cf 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -10,6 +10,7 @@
#include <ctype.h>
#include <time.h>
#include <pwd.h>
+#include <utime.h>
#ifndef WIN32
#include <sys/stat.h> /* for stat() */
#include <fcntl.h> /* open() flags */
@@ -151,7 +152,7 @@ static void copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf)
static bool do_connect(enum trivalue reuse_previous_specification,
char *dbname, char *user, char *host, char *port);
static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
- int lineno, bool *edited);
+ int lineno, bool *edited, bool discard_on_quit);
static bool do_shell(const char *command);
static bool do_watch(PQExpBuffer query_buf, double sleep);
static bool lookup_object_oid(EditableObjectType obj_type, const char *desc,
@@ -1003,6 +1004,13 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
}
if (status != PSQL_CMD_ERROR)
{
+ /*
+ * If the query buffer is empty, we'll edit the previous
+ * query. But in that case, we don't want to keep that if the
+ * editor is quit.
+ */
+ bool discard_on_quit = (query_buf->len == 0);
+
expand_tilde(&fname);
if (fname)
canonicalize_path(fname);
@@ -1010,7 +1018,7 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
/* If query_buf is empty, recall previous query for editing */
copy_previous_query(query_buf, previous_buf);
- if (do_edit(fname, query_buf, lineno, NULL))
+ if (do_edit(fname, query_buf, lineno, NULL, discard_on_quit))
status = PSQL_CMD_NEWEDIT;
else
status = PSQL_CMD_ERROR;
@@ -1133,11 +1141,9 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch,
{
bool edited = false;
- if (!do_edit(NULL, query_buf, lineno, &edited))
+ if (!do_edit(NULL, query_buf, lineno, &edited, true))
status = PSQL_CMD_ERROR;
- else if (!edited)
- puts(_("No changes"));
- else
+ else if (edited)
status = PSQL_CMD_NEWEDIT;
}
@@ -3626,12 +3632,7 @@ UnsyncVariables(void)
}
-/*
- * do_edit -- handler for \e
- *
- * If you do not specify a filename, the current query buffer will be copied
- * into a temporary one.
- */
+/* helper for do_edit() */
static bool
editFile(const char *fname, int lineno)
{
@@ -3699,10 +3700,18 @@ editFile(const char *fname, int lineno)
}
-/* call this one */
+/*
+ * do_edit -- handler for \e
+ *
+ * If you do not specify a filename, the current query buffer will be copied
+ * into a temporary one.
+ * "edited" will be set to true if the file is modified.
+ * If "discard_on_quit" is true, discard the query buffer if the file was
+ * not modified.
+ */
static bool
do_edit(const char *filename_arg, PQExpBuffer query_buf,
- int lineno, bool *edited)
+ int lineno, bool *edited, bool discard_on_quit)
{
char fnametmp[MAXPGPATH];
FILE *stream = NULL;
@@ -3790,6 +3799,24 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
pg_log_error("%s: %m", fname);
error = true;
}
+ else
+ {
+ struct utimbuf ut;
+
+ /*
+ * Try to set the file modification time of the temporary file
+ * a few seconds into the past. Otherwise, the low
+ * granularity (one second) that we can portably measure with
+ * stat(2) could lead us to not recognize a modification, if
+ * the file was edited in less than a second.
+ *
+ * This is a rather narrow race condition, so don't error out
+ * if the utime(2) call fails --- that would make the cure
+ * worse than the disease.
+ */
+ ut.modtime = ut.actime = time(NULL) - 1;
+ (void) utime(fname, &ut);
+ }
}
}
@@ -3809,6 +3836,7 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
error = true;
}
+ /* file was edited if the modification time is changed */
if (!error && before.st_mtime != after.st_mtime)
{
stream = fopen(fname, PG_BINARY_R);
@@ -3839,6 +3867,14 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
fclose(stream);
}
}
+ else
+ {
+ /* if we were editing anything else than the query buffer, discard */
+ if (discard_on_quit)
+ resetPQExpBuffer(query_buf);
+ else
+ puts(_("No changes"));
+ }
/* remove temp file */
if (!filename_arg)
--
2.26.2
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
Attachments:
0001-Improve-e-ef-and-ev-if-the-editor-is-quit.v5.patchtext/x-diff; charset=us-ascii; name=0001-Improve-e-ef-and-ev-if-the-editor-is-quit.v5.patchDownload
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 13c1edfa4d..0bf69174ff 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1970,7 +1970,9 @@ testdb=>
</para>
<para>
- The new contents of the query buffer are then re-parsed according to
+ If you edit a file or the previous query, and you quit the editor without
+ modifying the file, the query buffer is cleared.
+ Otherwise, the new contents of the query buffer are re-parsed according to
the normal rules of <application>psql</application>, treating the
whole buffer as a single line. Any complete queries are immediately
executed; that is, if the query buffer contains or ends with a
@@ -2039,7 +2041,8 @@ Tue Oct 26 21:40:57 CEST 1999
in the form of a <command>CREATE OR REPLACE FUNCTION</command> or
<command>CREATE OR REPLACE PROCEDURE</command> command.
Editing is done in the same way as for <literal>\edit</literal>.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
if you added a semicolon to it. Otherwise it is redisplayed;
type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal>
to cancel.
@@ -2115,7 +2118,8 @@ Tue Oct 26 21:40:57 CEST 1999
This command fetches and edits the definition of the named view,
in the form of a <command>CREATE OR REPLACE VIEW</command> command.
Editing is done in the same way as for <literal>\edit</literal>.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
if you added a semicolon to it. Otherwise it is redisplayed;
type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal>
to cancel.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 838f74bbbb..3b97cd51dc 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -152,7 +152,7 @@ static void copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf)
static bool do_connect(enum trivalue reuse_previous_specification,
char *dbname, char *user, char *host, char *port);
static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
- int lineno, bool *edited);
+ int lineno, bool *edited, bool discard_on_quit);
static bool do_shell(const char *command);
static bool do_watch(PQExpBuffer query_buf, double sleep);
static bool lookup_object_oid(EditableObjectType obj_type, const char *desc,
@@ -1004,6 +1004,13 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
}
if (status != PSQL_CMD_ERROR)
{
+ /*
+ * If the query buffer is empty, we'll edit the previous
+ * query. But in that case, we don't want to keep that if the
+ * editor is quit.
+ */
+ bool discard_on_quit = (query_buf->len == 0);
+
expand_tilde(&fname);
if (fname)
canonicalize_path(fname);
@@ -1011,7 +1018,7 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
/* If query_buf is empty, recall previous query for editing */
copy_previous_query(query_buf, previous_buf);
- if (do_edit(fname, query_buf, lineno, NULL))
+ if (do_edit(fname, query_buf, lineno, NULL, discard_on_quit))
status = PSQL_CMD_NEWEDIT;
else
status = PSQL_CMD_ERROR;
@@ -1134,11 +1141,9 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch,
{
bool edited = false;
- if (!do_edit(NULL, query_buf, lineno, &edited))
+ if (!do_edit(NULL, query_buf, lineno, &edited, true))
status = PSQL_CMD_ERROR;
- else if (!edited)
- puts(_("No changes"));
- else
+ else if (edited)
status = PSQL_CMD_NEWEDIT;
}
@@ -3627,12 +3632,7 @@ UnsyncVariables(void)
}
-/*
- * do_edit -- handler for \e
- *
- * If you do not specify a filename, the current query buffer will be copied
- * into a temporary one.
- */
+/* helper for do_edit() */
static bool
editFile(const char *fname, int lineno)
{
@@ -3700,10 +3700,18 @@ editFile(const char *fname, int lineno)
}
-/* call this one */
+/*
+ * do_edit -- handler for \e
+ *
+ * If you do not specify a filename, the current query buffer will be copied
+ * into a temporary one.
+ * "edited" will be set to true if the file is modified.
+ * If "discard_on_quit" is true, discard the query buffer if the file was
+ * not modified.
+ */
static bool
do_edit(const char *filename_arg, PQExpBuffer query_buf,
- int lineno, bool *edited)
+ int lineno, bool *edited, bool discard_on_quit)
{
char fnametmp[MAXPGPATH];
FILE *stream = NULL;
@@ -3860,6 +3868,14 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
fclose(stream);
}
}
+ else
+ {
+ /* if we were editing anything else than the query buffer, discard */
+ if (discard_on_quit)
+ resetPQExpBuffer(query_buf);
+ else
+ puts(_("No changes"));
+ }
/* remove temp file */
if (!filename_arg)
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 becomesdiscard_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
Attachments:
0001-Improve-e-ef-and-ev-if-the-editor-is-quit.v6.patchtext/x-patch; charset=UTF-8; name=0001-Improve-e-ef-and-ev-if-the-editor-is-quit.v6.patchDownload
From 436f48e21adcf207dc267db834a2e80f846fb666 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Mon, 15 Mar 2021 17:09:22 +0100
Subject: [PATCH] Improve \e, \ef and \ev if the editor is quit
Before, if you edited a function or view definition or a
script file, quitting the editor without saving would cause
the *previous query* to be executed, which is certainly not
what anyone would expect.
It is better to execute nothing and clear the query buffer
in this case.
The behavior when editing the current query buffer is
unchanged: quitting without saving will retain it.
In passing, fix the misplaced function comment for do_edit().
Author: Laurenz Albe <laurenz.albe@cybertec.at>
Reviewed-by: Chapman Flack, Jacob Champion
Discussion: https://postgr.es/m/0ba3f2a658bac6546d9934ab6ba63a805d46a49b.camel%40cybertec.at
---
doc/src/sgml/ref/psql-ref.sgml | 10 ++++--
src/bin/psql/command.c | 63 +++++++++++++++++++++++++---------
2 files changed, 53 insertions(+), 20 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 13c1edfa4d..0bf69174ff 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1970,7 +1970,9 @@ testdb=>
</para>
<para>
- The new contents of the query buffer are then re-parsed according to
+ If you edit a file or the previous query, and you quit the editor without
+ modifying the file, the query buffer is cleared.
+ Otherwise, the new contents of the query buffer are re-parsed according to
the normal rules of <application>psql</application>, treating the
whole buffer as a single line. Any complete queries are immediately
executed; that is, if the query buffer contains or ends with a
@@ -2039,7 +2041,8 @@ Tue Oct 26 21:40:57 CEST 1999
in the form of a <command>CREATE OR REPLACE FUNCTION</command> or
<command>CREATE OR REPLACE PROCEDURE</command> command.
Editing is done in the same way as for <literal>\edit</literal>.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
if you added a semicolon to it. Otherwise it is redisplayed;
type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal>
to cancel.
@@ -2115,7 +2118,8 @@ Tue Oct 26 21:40:57 CEST 1999
This command fetches and edits the definition of the named view,
in the form of a <command>CREATE OR REPLACE VIEW</command> command.
Editing is done in the same way as for <literal>\edit</literal>.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
if you added a semicolon to it. Otherwise it is redisplayed;
type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal>
to cancel.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 838f74bbbb..e2af0f56bd 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -148,11 +148,11 @@ static void save_query_text_state(PsqlScanState scan_state, ConditionalStack cst
PQExpBuffer query_buf);
static void discard_query_text(PsqlScanState scan_state, ConditionalStack cstack,
PQExpBuffer query_buf);
-static void copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf);
+static bool copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf);
static bool do_connect(enum trivalue reuse_previous_specification,
char *dbname, char *user, char *host, char *port);
static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
- int lineno, bool *edited);
+ int lineno, bool *edited, bool discard_on_quit);
static bool do_shell(const char *command);
static bool do_watch(PQExpBuffer query_buf, double sleep);
static bool lookup_object_oid(EditableObjectType obj_type, const char *desc,
@@ -418,7 +418,7 @@ exec_command(const char *cmd,
* the individual command subroutines.
*/
if (status == PSQL_CMD_SEND)
- copy_previous_query(query_buf, previous_buf);
+ (void) copy_previous_query(query_buf, previous_buf);
return status;
}
@@ -1004,14 +1004,20 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
}
if (status != PSQL_CMD_ERROR)
{
+ bool discard_on_quit;
+
expand_tilde(&fname);
if (fname)
canonicalize_path(fname);
- /* If query_buf is empty, recall previous query for editing */
- copy_previous_query(query_buf, previous_buf);
+ /*
+ * If query_buf is empty, recall previous query for editing.
+ * But in that case, the query buffer should be emptied if
+ * editing doesn't modify the file.
+ */
+ discard_on_quit = copy_previous_query(query_buf, previous_buf);
- if (do_edit(fname, query_buf, lineno, NULL))
+ if (do_edit(fname, query_buf, lineno, NULL, discard_on_quit))
status = PSQL_CMD_NEWEDIT;
else
status = PSQL_CMD_ERROR;
@@ -1134,7 +1140,7 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch,
{
bool edited = false;
- if (!do_edit(NULL, query_buf, lineno, &edited))
+ if (!do_edit(NULL, query_buf, lineno, &edited, true))
status = PSQL_CMD_ERROR;
else if (!edited)
puts(_("No changes"));
@@ -2637,7 +2643,7 @@ exec_command_watch(PsqlScanState scan_state, bool active_branch,
}
/* If query_buf is empty, recall and execute previous query */
- copy_previous_query(query_buf, previous_buf);
+ (void) copy_previous_query(query_buf, previous_buf);
success = do_watch(query_buf, sleep);
@@ -2961,12 +2967,19 @@ discard_query_text(PsqlScanState scan_state, ConditionalStack cstack,
* This is used by various slash commands for which re-execution of a
* previous query is a common usage. For convenience, we allow the
* case of query_buf == NULL (and do nothing).
+ *
+ * Returns "true" if the previous query was copied into the query
+ * buffer, else "false".
*/
-static void
+static bool
copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf)
{
if (query_buf && query_buf->len == 0)
+ {
appendPQExpBufferStr(query_buf, previous_buf->data);
+ return true;
+ }
+ return false;
}
/*
@@ -3627,12 +3640,7 @@ UnsyncVariables(void)
}
-/*
- * do_edit -- handler for \e
- *
- * If you do not specify a filename, the current query buffer will be copied
- * into a temporary one.
- */
+/* helper for do_edit() */
static bool
editFile(const char *fname, int lineno)
{
@@ -3700,10 +3708,20 @@ editFile(const char *fname, int lineno)
}
-/* call this one */
+/*
+ * do_edit -- handler for \e
+ *
+ * If you do not specify a filename, the current query buffer will be copied
+ * into a temporary one.
+ * "edited" will be set to true if the file is modified.
+ *
+ * After this function is done, the resulting file will be copied back into the
+ * query buffer. As an exception to this, the query buffer will be emptied
+ * if the file was not modified and the caller set "discard_on_quit" to "true".
+ */
static bool
do_edit(const char *filename_arg, PQExpBuffer query_buf,
- int lineno, bool *edited)
+ int lineno, bool *edited, bool discard_on_quit)
{
char fnametmp[MAXPGPATH];
FILE *stream = NULL;
@@ -3860,6 +3878,17 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
fclose(stream);
}
}
+ else
+ {
+ /*
+ * If the file was not modified, and the caller requested it, discard
+ * the query buffer.
+ */
+ if (discard_on_quit)
+ resetPQExpBuffer(query_buf);
+ else
+ puts(_("No changes"));
+ }
/* remove temp file */
if (!filename_arg)
--
2.26.2
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
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