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
From 31257c2e697421bbcc08bdd546ccc8729654ccb3 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.a...@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.a...@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
