Re: Confusing behavior of psql's \e

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

In response to

Responses

Browse pgsql-hackers by date

  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.