Обсуждение: psql: Allow editing query results with \gedit

Assuming a SELECT statement reading from a single table, it is quite an
effort to transform that statement to an UPDATE statement on that table,
perhaps to fix a typo that the user has spotted in the query result.

First, the general syntax is not the same with the order of syntax
elements changed. Then the row in question needs to be pinned down by
the primary key, requiring cut-and-paste of the PK columns. Furthermore,
the value to be updated needs to be put into the command, with proper
quoting. If the original value spans multiple line, copy-and-pasting it
for editing is especially tedious.

Suppose the following query where we spot a typo in the 2nd message:

=# select id, language, message from messages where language = 'en';
id | language | message
 1 | en       | Good morning
 2 | en       | Hello warld

The query needs to be transformed into this update:

=# update messages set message = 'Hello world' where id = 2;

This patch automates the tedious parts by opening the query result in a
editor in JSON format, where the user can edit the data. On closing the
editor, the JSON data is read back, and the differences are sent as
UPDATE commands. New rows are INSERTed, and deleted rows are DELETEd.

=# select id, language, message from messages where language = 'en' \gedit

An editor opens:
[
{ "id": 1, "language": "en", "message": "Good morning" },
{ "id": 2, "language": "en", "message": "Hello warld" }
]

Let's fix the typo and save the file:
[
{ "id": 1, "language": "en", "message": "Good morning" },
{ "id": 2, "language": "en", "message": "Hello world" }
]
UPDATE messages SET message = 'Hello world' WHERE id = '2';
UPDATE 1

In this example, typing "WHERE id = 2" would not be too hard, but the
primary key might be a composite key, with complex non-numeric values.
This is supported as well.

If expanded mode (\x) is enabled, \gedit will use the expanded JSON
format, best suitable for long values.


This patch requires the "psql JSON output format" patch.

Christoph

Pavel Stehule <pavel.stehule@gmail.com> writes:
> po 22. 1. 2024 v 16:06 odesílatel Christoph Berg <myon@debian.org> napsal:
>> This patch automates the tedious parts by opening the query result in a
>> editor in JSON format, where the user can edit the data. On closing the
>> editor, the JSON data is read back, and the differences are sent as
>> UPDATE commands. New rows are INSERTed, and deleted rows are DELETEd.

> Introduction of \gedit is interesting idea, but in this form it looks too
> magic

Yeah, I don't like it either --- it feels like something that belongs
in an ETL tool not psql.  The sheer size of the patch shows how far
afield it is from anything that psql already does, necessitating
writing tons of stuff that was not there before.  The bits that try
to parse the query to get necessary information seem particularly
half-baked.

> In the end I am not sure if I like it or dislike it. Looks dangerous. I can
> imagine possible damage when some people will see vi first time and will
> try to finish vi, but in this command, it will be transformed to executed
> UPDATEs.

Yup -- you'd certainly want some way of confirming that you actually
want the changes applied.  Our existing edit commands load the edited
string back into the query buffer, where you can \r it if you don't
want to run it.  But I fear that the results of this operation would
be too long for that to be workable.

> More generating UPDATEs without knowledge of table structure
> (knowledge of PK) can be issue (and possibly dangerous too), and you cannot
> to recognize PK from result of SELECT (Everywhere PK is not "id" and it is
> not one column).

It does look like it's trying to find out the pkey from the system
catalogs ... however, it's also accepting unique constraints without
a lot of thought about the implications of NULLs.  Even if you have
a pkey, it's not exactly clear to me what should happen if the user
changes the contents of a pkey field.  That could be interpreted as
either an INSERT or UPDATE, I think.

Also, while I didn't read too closely, it's not clear to me how the
code could reliably distinguish INSERT vs UPDATE vs DELETE cases ---
surely we're not going to try to put a "diff" engine into this, and
even if we did, diff is well known for producing somewhat surprising
decisions about exactly which old lines match which new ones.  That's
part of the reason why I really don't like the idea that the deduced
change commands will be summarily applied without the user even
seeing them.

            regards, tom lane

Pavel Stehule <pavel.stehule@gmail.com> writes:
> I would have forms like FoxPro, I would have a grid like FoxPro, but not in
> psql, and I would not develop it :-)

Yeah, that's something that was also bothering me, but I failed to
put my finger on it.  "Here's some JSON, edit it, and don't forget
to keep the quoting correct" does not strike me as a user-friendly
way to adjust data content.  A spreadsheet-like display where you
can change data within cells seems like a far better API, although
I don't want to write that either.

This kind of API would not readily support INSERT or DELETE cases, but
TBH I think that's better anyway --- you're adding too much ambiguity
in pursuit of a very secondary use-case.  The stated complaint was
"it's too hard to build UPDATE commands", which I can sympathize with.

(BTW, I wonder how much of this already exists in pgAdmin.)

            regards, tom lane

Re: Pavel Stehule
> Introduction of \gedit is interesting idea, but in this form it looks too
> magic
> 
> a) why the data are in JSON format, that is not native for psql (minimally
> now)

Because we need something machine-readable. CSV would be an
alternative, but that is hardly human-readable.

> b) the implicit transformation to UPDATEs and the next evaluation can be
> pretty dangerous.

You can always run it in a transaction:

begin;
select * from tbl \gedit
commit;

Alternatively, we could try to make it not send the commands right away
- either by putting them into the query buffer (that currently work
only for single commands without any ';') or by opening another
editor. Doable, but perhaps the transaction Just Works?

> The concept of proposed feature is interesting, but the name \gedit is too
> generic, maybe too less descriptive for this purpose
> 
> Maybe \geditupdates can be better - but still it can be dangerous and slow
> (without limits)

An alternative I was considering was \edittable, but that would put
more emphasis on "table" when it's actually more powerful than that,
it works on a query result. (Similar in scope to updatable views.)

> In the end I am not sure if I like it or dislike it. Looks dangerous. I can
> imagine possible damage when some people will see vi first time and will
> try to finish vi, but in this command, it will be transformed to executed
> UPDATEs.

If you close the editor with touching the file, nothing will be sent.
And if you mess up the file, it will complain. I think it's unlikely
that people who end up in an editor they can't operate would be able
to modify JSON in a way that is still valid.

Also, \e has the same problem. Please don't let "there are users who
don't know what they are doing" spoil the idea.

> More generating UPDATEs without knowledge of table structure
> (knowledge of PK) can be issue (and possibly dangerous too), and you cannot
> to recognize PK from result of SELECT (Everywhere PK is not "id" and it is
> not one column).

It *does* retrieve the proper PK from the table. All updates are based
on the PK.


Re: Tom Lane
> > Introduction of \gedit is interesting idea, but in this form it looks too
> > magic
> 
> Yeah, I don't like it either --- it feels like something that belongs
> in an ETL tool not psql.

I tried to put it elsewhere first, starting with pspg:
https://github.com/okbob/pspg/issues/200
The problem is that external programs like the pager neither have
access to the query string/table name, nor the database connection.

ETL would also not quite fit, this is meant for interactive use.

> The sheer size of the patch shows how far
> afield it is from anything that psql already does, necessitating
> writing tons of stuff that was not there before.

I've been working on this for several months, so it's already larger
than the MVP would be. It does have features like (key=col1,col2) that
could be omitted.

> The bits that try
> to parse the query to get necessary information seem particularly
> half-baked.

Yes, that's not pretty, and I'd be open for suggestions on how to
improve that. I was considering:

1) this (dumb query parsing)
2) EXPLAIN the query to get the table
3) use libpq's PQftable

The problem with 2+3 is that on views and partitioned tables, this
would yield the base table name, not the table name used in the query.
1 turned out to be the most practical, and worked for me so far.

If the parse tree would be available, using that would be much better.
Should we perhaps add something like "explain (parse) select...", or
add pg_parsetree(query) function?

> Yup -- you'd certainly want some way of confirming that you actually
> want the changes applied.  Our existing edit commands load the edited
> string back into the query buffer, where you can \r it if you don't
> want to run it.  But I fear that the results of this operation would
> be too long for that to be workable.

The results are as long as you like. The intended use case would be to
change just a few rows.

As said above, I was already thinking of making it user-confirmable,
just the current version doesn't have it.

> It does look like it's trying to find out the pkey from the system
> catalogs ... however, it's also accepting unique constraints without
> a lot of thought about the implications of NULLs.

Right, the UNIQUE part doesn't take care of NULLs yet. Will fix that.
(Probably by erroring out if any key column is NULL.)

> Even if you have
> a pkey, it's not exactly clear to me what should happen if the user
> changes the contents of a pkey field.  That could be interpreted as
> either an INSERT or UPDATE, I think.

A changed PK will be interpreted as DELETE + INSERT. (I shall make
that more clear in the documentation.)

> Also, while I didn't read too closely, it's not clear to me how the
> code could reliably distinguish INSERT vs UPDATE vs DELETE cases ---
> surely we're not going to try to put a "diff" engine into this, and
> even if we did, diff is well known for producing somewhat surprising
> decisions about exactly which old lines match which new ones.  That's
> part of the reason why I really don't like the idea that the deduced
> change commands will be summarily applied without the user even
> seeing them.

The "diff" is purely based on "after editing, is there still a row
with this key identity". If the PK columns are not changed, the UPDATE
will hook on the PK value.

If you changed the PK columns, that will be a DELETE and an INSERT.

During development, I was considering to forbid (error out) changing
the PK columns. But then, simply deleting rows (= DELETE) and adding
new rows (= INSERT) seemed like a nice feature by itself, so I left
that in. Perhaps that should be reconsidered?

Christoph

Re: David G. Johnston
> Building off the other comments, I'd suggest trying to get rid of the
> intermediate JSOn format and also just focus on a single row at any given
> time.

We need *some* machine-readable format. It doesn't have to be JSON,
but JSON is actually pretty nice to read - and if values are too long,
or there are too many values, switch to extended mode:

select * from messages \gedit (expanded)

[{
  "id": "1",
  "language": "en",
  "message": "This is a very long test text with little actual meaning."
},{
  "id": "2",
  "language": "en",
  "message": "Another one, a bit shorter."
}]

I tweaked the indentation in the psql JSON output patch specifically
to make it readable.

Restricting to a single line might make sense if it helps editing, but
I don't think it does.

> For an update the first argument to the metacommand could be the unique key
> value present in the previous result.  The resultant UPDATE would just put
> that into the where clause and every other column in the result would be a
> SET clause column with the thing being set the current value, ready to be
> edited.

Hmm, then you would still have to cut-and-paste the PK value. If that
that's a multi-column non-numeric key, you are basically back to the
original problem.


Re: Tom Lane
> Yeah, that's something that was also bothering me, but I failed to
> put my finger on it.  "Here's some JSON, edit it, and don't forget
> to keep the quoting correct" does not strike me as a user-friendly
> way to adjust data content.  A spreadsheet-like display where you
> can change data within cells seems like a far better API, although
> I don't want to write that either.

Right. I wouldn't want a form editing system in there either. But
perhaps this middle ground of using a well-established format that is
easy to generate and to parse (it's using the JSON parser from
pgcommon) makes it fit into psql.

If parsing the editor result fails, the user is asked if they want to
re-edit with a parser error message, and if they go to the editor
again, the cursor is placed in the line where the error is. (Also,
what's wrong with having to strictly adhere to some syntax, we are
talking about SQL here.)

It's admittedly larger than the average \backslash command, but it
does fit into psql's interactive usage. \crosstabview is perhaps a
similar thing - it doesn't really fit into a simple "send query and
display result" client, but since it solves an actual problem, it
makes well sense to spend the extra code on it.

> This kind of API would not readily support INSERT or DELETE cases, but
> TBH I think that's better anyway --- you're adding too much ambiguity
> in pursuit of a very secondary use-case.  The stated complaint was
> "it's too hard to build UPDATE commands", which I can sympathize with.

I've been using the feature already for some time, and it's a real
relief. In my actual use case here, I use it on my ham radio logbook:

=# select start, call, qrg, name from log where cty = 'CE9' order by start;
         start          │  call  │     qrg     │ name
────────────────────────┼────────┼─────────────┼───────
 2019-03-12 20:34:00+00 │ RI1ANL │    7.076253 │ ∅
 2021-03-16 21:24:00+00 │ DP0GVN │    2400.395 │ Felix
 2022-01-15 17:19:00+00 │ DP0GVN │     2400.01 │ Felix
 2022-10-23 19:17:15+00 │ DP0GVN │ 2400.041597 │ ∅
 2023-10-01 14:05:00+00 │ 8J1RL  │   28.182575 │ ∅
 2024-01-22 21:15:15+00 │ DP1POL │   10.138821 │ ∅
(6 Zeilen)

The primary key is (start, call).

If I now want to note that the last contact with Antarctica there was
also with Felix, I'd have to transform that into

update log set name = 'Felix' where start = '2024-01-22 21:15:15+00' and call = 'DP1POL';

\gedit is just so much easier.

UPDATE is the core feature. If we want to say INSERT and DELETE aren't
supported, but UPDATE support can go in, that'd be fine with me.

> (BTW, I wonder how much of this already exists in pgAdmin.)

pgadmin seems to support it. (Most other clients don't.)

Obviously, I would want to do the updating using the client I also use
for querying.

Christoph

Re: Pavel Stehule
> It looks great for simple queries, but if somebody uses it like SELECT *
> FROM pg_proc \gedit

What's wrong with that? If the pager can handle the amount of data,
the editor can do that as well. (If not, the fix is to just not run
the command, and not blame the feature.)

> I almost sure so \gedit is wrong name for this feature.

I'm open for suggestions.

> Can be nice if we are able:
> 
> a) export data set in some readable format
> 
> b) be possible to use more command in pipes
> 
> some like
> 
> select start, call, qrg, name from log where cty = 'CE9' order by start
> \gpipexec(tsv) mypipe | bash update_pattern.sh > tmpfile; vi tmpfile; cat
> tmpfile > mypipe

Well yeah, that's still a lot of typing.

> I understand your motivation well, but I don't like your proposal because
> too many different things are pushed to one feature, and it is designed for
> a single purpose.

It's one feature for one purpose. And the patch isn't *that* huge. Did
I make the mistake of adding documentation, extra command options, and
tab completion in v1?

Christoph

On Mon, Jan 22, 2024 at 11:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Introduction of \gedit is interesting idea, but in this form it looks too
> > magic
>
> Yeah, I don't like it either --- it feels like something that belongs
> in an ETL tool not psql.  The sheer size of the patch shows how far
> afield it is from anything that psql already does, necessitating
> writing tons of stuff that was not there before.  The bits that try
> to parse the query to get necessary information seem particularly
> half-baked.

Based on these comments and the one from David Johnston, I think there
is a consensus that we do not want this patch, so I'm marking it as
Rejected in the CommitFest application. If I've misunderstood the
situation, then please feel free to change the status accordingly.

I feel slightly bad about rejecting this not only because rejecting
patches that people have put work into sucks but also because (1) I do
understand why it could be useful to have something like this and (2)
I think in many ways the patch is quite well-considered, e.g. it has
options like table and key to work around cases where the naive logic
doesn't get the right answer. But I also do understand why the
reactions thus far have been skeptical: there's a lot of pretty
magical stuff in this patch. That's a reliability concern: when you
type \gedit and it works, that's cool, but sometimes it isn't going to
work, and you're not always going to understand why, and you can
probably fix a lot of those cases by using the "table" or "key"
options, but you have to know they exist, and you have to realize that
they're needed, and the whole thing is suddenly a lot less convenient.
I think if we add this feature, a bunch of people won't notice, but
among those who do, I bet there will be a pretty good chance of people
complaining about cases that don't work, and perhaps not understanding
why they don't work, and I suspect fixing some of those complaints may
require something pretty close to solving the halting problem. :-(

Now maybe that's all wrong and we should adopt this patch with
enthusiasm, but if so, we need the patch to have significantly more +1
votes than -1 votes, and right now the reverse seems to be the case.

--
Robert Haas
EDB: http://www.enterprisedb.com

Re: Robert Haas
> Based on these comments and the one from David Johnston, I think there
> is a consensus that we do not want this patch, so I'm marking it as
> Rejected in the CommitFest application. If I've misunderstood the
> situation, then please feel free to change the status accordingly.

Hi Robert,

thanks for looking at the patch.

> I feel slightly bad about rejecting this not only because rejecting
> patches that people have put work into sucks but also because (1) I do
> understand why it could be useful to have something like this and (2)
> I think in many ways the patch is quite well-considered, e.g. it has
> options like table and key to work around cases where the naive logic
> doesn't get the right answer. But I also do understand why the
> reactions thus far have been skeptical: there's a lot of pretty
> magical stuff in this patch. That's a reliability concern: when you
> type \gedit and it works, that's cool, but sometimes it isn't going to
> work, and you're not always going to understand why, and you can
> probably fix a lot of those cases by using the "table" or "key"
> options, but you have to know they exist, and you have to realize that
> they're needed, and the whole thing is suddenly a lot less convenient.
> I think if we add this feature, a bunch of people won't notice, but
> among those who do, I bet there will be a pretty good chance of people
> complaining about cases that don't work, and perhaps not understanding
> why they don't work, and I suspect fixing some of those complaints may
> require something pretty close to solving the halting problem. :-(

That's a good summary of the situation, thanks.

I still think the feature would be cool to have, but admittedly, in
the meantime I've had cases myself where the automatism went into the
wrong direction (updating key columns results in DELETE-INSERT cycles
that aren't doing the right thing if you didn't select all columns
originally), so I now understand the objections and agree people were
right about that. This could be fixed by feeding the resulting
commands through another editor round, but that just adds complexity
instead of removing confusion.

I think there is a pretty straightforward way to address the problems,
though: instead of letting the user edit JSON, format the query result
in the form of UPDATE commands and let the user edit them. As Tom said
upthread:

Tom> The stated complaint was "it's too hard to build UPDATE commands",
Tom> which I can sympathize with.

... which this would perfectly address - it's building the commands.

The editor will have a bit more clutter (the UPDATE SET WHERE
boilerplate), and the fields are somewhat out of order (the key at the
end), but SQL commands are what people understand, and there is
absolutely no ambiguity on what is going to be executed since the
commands are exactly what is leaving the editor.

> Now maybe that's all wrong and we should adopt this patch with
> enthusiasm, but if so, we need the patch to have significantly more +1
> votes than -1 votes, and right now the reverse seems to be the case.

I'll update the patch and ask here. Thanks!

Christoph

On Fri, May 17, 2024 at 9:24 AM Christoph Berg <myon@debian.org> wrote:
> Tom> The stated complaint was "it's too hard to build UPDATE commands",
> Tom> which I can sympathize with.
>
> ... which this would perfectly address - it's building the commands.
>
> The editor will have a bit more clutter (the UPDATE SET WHERE
> boilerplate), and the fields are somewhat out of order (the key at the
> end), but SQL commands are what people understand, and there is
> absolutely no ambiguity on what is going to be executed since the
> commands are exactly what is leaving the editor.

A point to consider is that the user can also do this in the query
itself, if desired. It'd just be a matter of assembling the query
string with appropriate calls to quote_literal() and quote_ident(),
which is not actually all that hard and I suspect many of us have done
that at one point or another. And then you know that you'll get the
right set of key columns and update the right table (as opposed to,
say, a view over the right table, or the wrong one out of several
tables that you joined).

Now you might say, well, that's not terribly convenient, which is
probably true, but this might be a case of -- convenient, reliable,
works with arbitrary queries -- pick two. I don't know. I wouldn't be
all that surprised if there's something clever and useful we could do
in this area, but I sure don't know what it is.

--
Robert Haas
EDB: http://www.enterprisedb.com