Re: Correct handling of blank/commented lines in PSQL interactive-mode history

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Greg Nancarrow <gregn4422(at)gmail(dot)com>
Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>, "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Correct handling of blank/commented lines in PSQL interactive-mode history
Date: 2021-11-27 23:30:30
Message-ID: 631830.1638055830@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> We could perhaps finesse that point by deciding that comment lines
> that are handled this way will never be sent to the server --- but
> I'm sure people will complain about that, too. I've definitely heard
> people complain because "--" comments are stripped from what's sent
> (so I'd look favorably on a patch to undo that).

In hopes of moving this thread forward, I experimented with doing that
bit, basically by simplifying the {whitespace} rule in psqlscan.l
to be just "ECHO;". That caused massive regression test failures,
of which this'll do as a sample:

--
-- This should fail
--
copy (select * from test1) (t,id) to stdout;
ERROR: syntax error at or near "("
-LINE 1: copy (select * from test1) (t,id) to stdout;
+LINE 4: copy (select * from test1) (t,id) to stdout;
^
--
-- Test JOIN

Of course, the problem is that since we're now including the three "--"
lines in what's sent to the server, it thinks the "copy" is on line 4.
I do not think we want such a behavior change: people don't tend to
think that such comments are part of the query.

I then experimented with combining the psqlscan.l change with mainloop.c
changes akin to what Greg had proposed, so as to discard leading comments
at the level of mainloop.c rather than inside the lexer. I didn't have
much luck getting to a behavior that I thought could be acceptable,
although maybe with more sweat it'd be possible.

One thing I noticed along the line is that because the history mechanism
records raw input lines while psqlscan.l discards dash-dash comments,
it's already the case that history entries don't match up too well with
what's sent to the server. So I'm not sure that my upthread complaint
about that holds water, and I'm less against Greg's original patch than
I was.

Trying to gather together the various issues mentioned on this thread,
I see:

* Initial input lines that are blank (whitespace, maybe including a
comment) are merged into the next command's history entry; but since
said lines don't give rise to any text sent to the server, there's
not really any reason why they couldn't be treated as text to be
emitted to the history file immediately. This is what Greg originally
set out to change. After my experiments mentioned above, I'm quite
doubtful that his patch is correct in detail (I'm afraid that it
probably emits stuff too soon in some cases), but it could likely be
fixed if we could just get agreement that a change of that sort is OK.

* It's not great that dash-dash comments aren't included in what we
send to the server. However, changing that is a lot trickier than
it looks. I think we want to continue suppressing comments that
precede the query proper. Including comments that are within the
query text (ahead of the trailing semi) is not so hard, but comments
following the semicolon look too much like comments-ahead-of-the-
next-query. Perhaps that issue should be left for another day ...
although it does feel like it interacts with the first point.

* Misbehavior of M-# was also mentioned. Does anyone object to
the draft patch I posted for that?

regards, tom lane

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bharath Rupireddy 2021-11-28 02:23:13 Re: Deduplicate code updating ControleFile's DBState.
Previous Message Tom Lane 2021-11-27 19:49:11 Re: rand48 replacement