Обсуждение: Correct handling of blank/commented lines in PSQL interactive-mode history

Hi,

Single lines entered in PSQL interactive-mode, containing just
whitespace or an SQL comment ("--..."), don't seem to be stored
correctly in the history.
For example, such lines are currently prepended to the history of the
next command entered, rather than having their own history entry.
Also, if HISTCONTROL=ignorespace is in effect, if a line is entered
that starts with a space and the rest of the line is whitespace or an
SQL comment, then it prevents the next command entered from being
saved in the history.

I've attached a patch that corrects the behaviour.
For the type of lines mentioned, the patch makes the history behave
more like Bash history.

[I noticed this problem in PSQL interactive-mode history when typing
in a long SQL command which I then decided to just comment, using a
"--" prefix, and enter it, to store it in the history, so I could
later recall it from the history after first executing some other
commands.]


Below are some examples of problem scenarios, and results BEFORE/AFTER
the patch is applied:

(1)

<space><ENTER>
SELECT 1;<ENTER>

BEFORE PATCH:
Results in a single history entry, with <space> on the 1st line and
"SELECT 1;" on the 2nd line.
AFTER PATCH:
Results in two history entries, 1st contains <space> and the 2nd
contains "SELECT 1;".


(2)

-- my comment<ENTER>
SELECT 1;<ENTER>

BEFORE PATCH:
Results in a single history entry, containing "-- my comment" on the
1st line and "SELECT 1;" on the 2nd line.
AFTER PATCH:
Results in two history entries, 1st contains "-- my comment" and the
2nd contains "SELECT 1;".


(3)
{--variable=HISTCONTROL=ignorespace}

<space><ENTER>
SELECT 1;<ENTER>

BEFORE PATCH:
No history entry is saved.
AFTER PATCH:
Results in one history entry, containing "SELECT 1;".


(4)
{--variable=HISTCONTROL=ignorespace}

<space>-- my comment<ENTER>
SELECT 1;<ENTER>

BEFORE PATCH:
No history entry is saved.
AFTER PATCH:
Results in one history entry, containing "SELECT 1;".


Regards,
Greg Nancarrow
Fujitsu Australia

On Mon, 2021-09-06 at 07:50 -0700, David G. Johnston wrote:
> On Mon, Sep 6, 2021 at 7:13 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
> > I've attached a patch that corrects the behaviour.
> > For the type of lines mentioned, the patch makes the history behave
> > more like Bash history.
> 
> I have my doubts that you've really fixed anything here since Bash is a
> line-oriented shell while psql is a statement-oriented one.  This is a feature.
> What you are observing is, I think, a side-effect of that fact that
> comments cannot terminate statements.  That seems reasonable.
> In short, your BEFORE results make sense and don't require fixing.

I think that psql's behavior should be governed more by usefulness than
by consideratoins like "comments cannot terminate statements".

I agree with Greg that the current behavior is annoying and would
welcome the change.  This has bothered me before.

That multi-line statements that contain a line with a space are omitted
from the history when HISTCONTROL is set to "ignorespace" seems like
a bug to me.

So +1 on the idea of the patch, although I didn't scrutinize the
implementation.

Yours,
Laurenz Albe

On Mon, Sep 06, 2021 at 07:50:15AM -0700, David G. Johnston wrote:
> On Mon, Sep 6, 2021 at 7:13 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
> > I've attached a patch that corrects the behaviour.
> > For the type of lines mentioned, the patch makes the history behave
> > more like Bash history.

The behavior of bash is configurable here:

|HISTCONTROL
|      A  colon-separated  list  of  values controlling how commands are saved on the history list.  If the list of
valuesincludes ignorespace, lines which begin with a space character are not saved in the history
 
|      list....

> I have my doubts that you've really fixed anything here since Bash is a
> line-oriented shell while psql is a statement-oriented one.  This is a
> feature.

Hm, I don't think bash is "line oriented" ?  You can type anything into it that
you'd put in a shell script.  For example:

$ for a in `seq 1 3`
> do
> echo $a
> done
1
2
3

-- 
Justin

On 2021-Sep-06, Laurenz Albe wrote:

> I agree with Greg that the current behavior is annoying and would
> welcome the change.  This has bothered me before.

It has bothered me too.  I am particularly bothered by the uselessness
that M-# results in -- namely, inserting a # at the start of the buffer.
This is quite useless: in bash, the # starts a comment, so M-# makes the
entry a comment; but in psql it doesn't have that effect.  If somebody
were to make M-# make the buffer a /* ... */ comment, it would become
quite handy.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Sep-06, Laurenz Albe wrote:
>> I agree with Greg that the current behavior is annoying and would
>> welcome the change.  This has bothered me before.

> It has bothered me too.

I'm not here to claim that the current behavior is perfect.  However,
AFAICT the patch as-presented breaks the principle that text goes into
the history at the moment it's sent to the server.  In particular, we
might make an entry for text that *never* got to the server because you
cleared the buffer instead.  I don't find that to be an improvement.
It breaks one of the primary use-cases for history, ie keeping a record
of what you did.

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).

I think the questions around empty-line handling are largely
orthogonal to this, and we'll just confuse ourselves if we
discuss that at the same time.  Likewise for M-#.

            regards, tom lane

[ this is a digression from the main point of the thread, but ... ]

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I am particularly bothered by the uselessness
> that M-# results in -- namely, inserting a # at the start of the buffer.

Fixing that might be as simple as the attached.  I've not beat on
it hard though.

            regards, tom lane

diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c
index f926bc98dc..1dcd95a7b9 100644
--- a/src/bin/psql/input.c
+++ b/src/bin/psql/input.c
@@ -353,8 +353,13 @@ initializeInput(int flags)
 
         useReadline = true;
 
-        /* these two things must be done in this order: */
+        /* set appropriate values for Readline's global variables */
         initialize_readline();
+
+        /* set comment-begin to a useful value for SQL */
+        (void) rl_variable_bind("comment-begin", "-- ");
+
+        /* this reads ~/.inputrc, so do it after rl_variable_bind */
         rl_initialize();
 
         useHistory = true;

On 07.09.21 21:16, Tom Lane wrote:
> [ this is a digression from the main point of the thread, but ... ]
> 
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>> I am particularly bothered by the uselessness
>> that M-# results in -- namely, inserting a # at the start of the buffer.
> 
> Fixing that might be as simple as the attached.  I've not beat on
> it hard though.

I see this in my .inputrc, although I don't remember when/how I put it 
there:

$if psql
set comment-begin --
set expand-tilde on
$endif

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

On Sat, 2021-11-27 at 18:30 -0500, Tom Lane wrote:
> 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.

For me, it is just a mild annoyance to have unrelated comments
preceding the query be part of the query's history file entry.
If that is difficult to improve, I can live with it the way it is.

> * 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.

If we treat double-dash comments differently from /* */ ones,
that is indeed odd.  I personally haven't been bothered by it, though.

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

No, I think that is a clear improvement.

There was one other problem mentioned in the original mail, and that
seems to be the most serious one to me:

> psql
psql (14.1)
Type "help" for help.

test=> \set HISTCONTROL ignorespace                       
test=>   -- line that starts with space
test=> SELECT 42;
 ?column? 
══════════
       42
(1 row)

Now that query is not added to the history file at all.

Yours,
Laurenz Albe

Laurenz Albe <laurenz.albe@cybertec.at> writes:
> There was one other problem mentioned in the original mail, and that
> seems to be the most serious one to me:
> [ HISTCONTROL behavior ]

The actual behavior of that option (which perhaps isn't adequately
documented) is that it suppresses a history entry if the first
character of the possibly-multi-line entry is a space.  It certainly
can't operate on a per-line basis, or you'd be likely to lose chunks
of a single SQL command, so I think that definition is fine as
it is (ignoring the whole question of whether the feature is sane
at all ... but if you don't think so, why would you use it?)

Greg's patch would fix this specifically by ensuring that the line
with the space and comment is treated as a separate history entry.
So I don't really see that as a separate bug.  Or, if you will,
the fact that people see it as a bug confirms that such a line
should be treated as a separate history entry.

            regards, tom lane

On 2021-Nov-29, Tom Lane wrote:

> Greg's patch would fix this specifically by ensuring that the line
> with the space and comment is treated as a separate history entry.
> So I don't really see that as a separate bug.  Or, if you will,
> the fact that people see it as a bug confirms that such a line
> should be treated as a separate history entry.

I wonder if these things would be easier to deal with or more convenient
if we thought of -- as starting a line-scoped comment, and /* */ as
starting a query-scoped comment, and treat both types differently.  That
is, a -- comment would not be part of the subsequent command (and they
would become two separate history entries), but a /* */ comment would be
part of the command, and so both the comment and the query would be
saved as a single history entry.

So with ignorespace, then you can get either behavior:

 /* don't put neither comment nor command in history */
select 1;
and then nothing gets put in history; but if you do

 -- don't put this *comment* in history, but do put command
select 1;

then the comment is ignored, but the query does end up in history.


Perhaps one problem is how to behave with intra-query -- comments.
Surely they should not split the command in three parts, but I'm not
sure to what extent that is possible to implement.

This doesn't actually fix the issue that Greg was complaining about,
because his problem is precisely the -- comments.  But would Greg and
other users be satisfied if we made that distinction?

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Mon, 2021-11-29 at 09:43 -0500, Tom Lane wrote:
> Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > There was one other problem mentioned in the original mail, and that
> > seems to be the most serious one to me:
> > [ HISTCONTROL behavior ]
> 
> The actual behavior of that option (which perhaps isn't adequately
> documented) is that it suppresses a history entry if the first
> character of the possibly-multi-line entry is a space.  It certainly
> can't operate on a per-line basis, or you'd be likely to lose chunks
> of a single SQL command, so I think that definition is fine as
> it is (ignoring the whole question of whether the feature is sane
> at all ... but if you don't think so, why would you use it?)
> 
> Greg's patch would fix this specifically by ensuring that the line
> with the space and comment is treated as a separate history entry.
> So I don't really see that as a separate bug.  Or, if you will,
> the fact that people see it as a bug confirms that such a line
> should be treated as a separate history entry.

Ah, yes.  You are right with both the explanation for the behavior
and stating that it points towards treating leading comments as
being seperate from the query.

And, thinking about HISTCONTROL, it does not seem sane in the
context of SQL, and I would never use it.

Yours,
Laurenz Albe

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I wonder if these things would be easier to deal with or more convenient
> if we thought of -- as starting a line-scoped comment, and /* */ as
> starting a query-scoped comment, and treat both types differently.  That
> is, a -- comment would not be part of the subsequent command (and they
> would become two separate history entries), but a /* */ comment would be
> part of the command, and so both the comment and the query would be
> saved as a single history entry.

The hack I was fooling with yesterday would have had that effect,
although it was a consequence of the fact that I was too lazy
to parse slash-star comments ;-).  But basically what I was
trying to do was to force a line that was only whitespace
(possibly plus dash-dash comment) to be treated as a separate
history entry, while not suppressing dash-dash comments
altogether as the current code does.

            regards, tom lane

After some further hackery, here's a set of patches that I think
might be acceptable.  They're actually fairly independent, although
they touch different aspects of the same behavior.

0001 gets rid of psqlscan.l's habit of suppressing dash-dash comments,
but only once we have collected some non-whitespace query input.
The upshot of this is that dash-dash comments will get sent to the
server as long as they are within the query proper, that is after the
first non-whitespace token and before the ending semicolon.  Comments
that are between queries are still suppressed, because not doing that
seems to result in far too much behavioral change.  As it stands,
though, there are just a few regression test result changes.

0002 is a simplified version of Greg's patch.  I think we only need
to look at the state of the query_buf to see if any input has been
collected in order to determine if we are within or between queries.
I'd originally thought this'd need to be a lot more complicated,
but as long as psqlscan.l continues to drop pre-query comments,
this seems to be enough.

0003 is the same patch I posted before to adjust M-# behavior.

            regards, tom lane

diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out
b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index b52d187722..e0abe34bb6 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -101,7 +101,7 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
 ------------------------------------------------------------------------------+-------+------
  PREPARE pgss_test (int) AS SELECT $1, $2 LIMIT $3                            |     1 |    1
  SELECT $1                                                                   +|     4 |    4
-                                                                             +|       |
+   -- multiline                                                              +|       |
    AS "text"                                                                  |       |
  SELECT $1 + $2                                                               |     2 |    2
  SELECT $1 + $2 + $3 AS "add"                                                 |     3 |    3
diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l
index 0fab48a382..325b440a12 100644
--- a/src/fe_utils/psqlscan.l
+++ b/src/fe_utils/psqlscan.l
@@ -376,12 +376,11 @@ other            .
                     /*
                      * Note that the whitespace rule includes both true
                      * whitespace and single-line ("--" style) comments.
-                     * We suppress whitespace at the start of the query
-                     * buffer.  We also suppress all single-line comments,
-                     * which is pretty dubious but is the historical
-                     * behavior.
+                     * We suppress whitespace until we have collected some
+                     * non-whitespace data.  (This interacts with some
+                     * decisions in MainLoop(); see there for details.)
                      */
-                    if (!(output_buf->len == 0 || yytext[0] == '-'))
+                    if (output_buf->len > 0)
                         ECHO;
                 }

diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 5949996ebc..be5fa5727d 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1905,14 +1905,14 @@ from generate_series(1,5) x,
      (values (0::float8),(0.1),(0.25),(0.4),(0.5),(0.6),(0.75),(0.9),(1)) v(p)
 group by p order by p;
 ERROR:  sum is not an ordered-set aggregate, so it cannot have WITHIN GROUP
-LINE 1: select p, sum() within group (order by x::float8)
+LINE 1: select p, sum() within group (order by x::float8)  -- error
                   ^
 select p, percentile_cont(p,p)  -- error
 from generate_series(1,5) x,
      (values (0::float8),(0.1),(0.25),(0.4),(0.5),(0.6),(0.75),(0.9),(1)) v(p)
 group by p order by p;
 ERROR:  WITHIN GROUP is required for ordered-set aggregate percentile_cont
-LINE 1: select p, percentile_cont(p,p)
+LINE 1: select p, percentile_cont(p,p)  -- error
                   ^
 select percentile_cont(0.5) within group (order by b) from aggtest;
  percentile_cont
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index c2e5676196..cb9373227d 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -928,7 +928,7 @@ CREATE TRIGGER gtest2a BEFORE INSERT OR UPDATE ON gtest26
   WHEN (NEW.b < 0)  -- error
   EXECUTE PROCEDURE gtest_trigger_func();
 ERROR:  BEFORE trigger's WHEN condition cannot reference NEW generated columns
-LINE 3:   WHEN (NEW.b < 0)
+LINE 3:   WHEN (NEW.b < 0)  -- error
                 ^
 DETAIL:  Column "b" is a generated column.
 CREATE TRIGGER gtest2b BEFORE INSERT OR UPDATE ON gtest26
@@ -936,7 +936,7 @@ CREATE TRIGGER gtest2b BEFORE INSERT OR UPDATE ON gtest26
   WHEN (NEW.* IS NOT NULL)  -- error
   EXECUTE PROCEDURE gtest_trigger_func();
 ERROR:  BEFORE trigger's WHEN condition cannot reference NEW generated columns
-LINE 3:   WHEN (NEW.* IS NOT NULL)
+LINE 3:   WHEN (NEW.* IS NOT NULL)  -- error
                 ^
 DETAIL:  A whole-row reference is used and the table contains generated columns.
 CREATE TRIGGER gtest2 BEFORE INSERT ON gtest26
diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out
index a3a2e383e3..75e61460d9 100644
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -2100,7 +2100,7 @@ WITH outermost(x) AS (
 )
 SELECT * FROM outermost ORDER BY 1;
 ERROR:  relation "outermost" does not exist
-LINE 4:          SELECT * FROM outermost
+LINE 4:          SELECT * FROM outermost  -- fail
                                ^
 DETAIL:  There is a WITH item named "outermost", but it cannot be referenced from this part of the query.
 HINT:  Use WITH RECURSIVE, or re-order the WITH items to remove forward references.
@@ -2124,7 +2124,7 @@ WITH RECURSIVE outermost(x) AS (
 )
 SELECT * FROM outermost ORDER BY 1;
 ERROR:  recursive reference to query "outermost" must not appear within a subquery
-LINE 2:   WITH innermost as (SELECT 2 FROM outermost)
+LINE 2:   WITH innermost as (SELECT 2 FROM outermost) -- fail
                                            ^
 --
 -- This test will fail with the old implementation of PARAM_EXEC parameter
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
index e49ed02293..3b7b4ad4a2 100644
--- a/src/bin/psql/mainloop.c
+++ b/src/bin/psql/mainloop.c
@@ -564,9 +564,20 @@ MainLoop(FILE *source)
                 break;
         }
 
-        /* Add line to pending history if we didn't execute anything yet */
-        if (pset.cur_cmd_interactive && !line_saved_in_history)
-            pg_append_history(line, history_buf);
+        /*
+         * Add line to pending history if we didn't do so already.  Then, if
+         * the query buffer is still empty, flush out any unsent history
+         * entry.  This means that empty lines (containing only whitespace and
+         * perhaps a dash-dash comment) that precede a query will be recorded
+         * as separate history entries, not as part of that query.
+         */
+        if (pset.cur_cmd_interactive)
+        {
+            if (!line_saved_in_history)
+                pg_append_history(line, history_buf);
+            if (query_buf->len == 0)
+                pg_send_history(history_buf);
+        }
 
         psql_scan_finish(scan_state);
         free(line);
diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c
index f926bc98dc..1dcd95a7b9 100644
--- a/src/bin/psql/input.c
+++ b/src/bin/psql/input.c
@@ -353,8 +353,13 @@ initializeInput(int flags)
 
         useReadline = true;
 
-        /* these two things must be done in this order: */
+        /* set appropriate values for Readline's global variables */
         initialize_readline();
+
+        /* set comment-begin to a useful value for SQL */
+        (void) rl_variable_bind("comment-begin", "-- ");
+
+        /* this reads ~/.inputrc, so do it after rl_variable_bind */
         rl_initialize();
 
         useHistory = true;

On Tue, Nov 30, 2021 at 7:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> After some further hackery, here's a set of patches that I think
> might be acceptable.  They're actually fairly independent, although
> they touch different aspects of the same behavior.
>
> 0001 gets rid of psqlscan.l's habit of suppressing dash-dash comments,
> but only once we have collected some non-whitespace query input.
> The upshot of this is that dash-dash comments will get sent to the
> server as long as they are within the query proper, that is after the
> first non-whitespace token and before the ending semicolon.  Comments
> that are between queries are still suppressed, because not doing that
> seems to result in far too much behavioral change.  As it stands,
> though, there are just a few regression test result changes.
>
> 0002 is a simplified version of Greg's patch.  I think we only need
> to look at the state of the query_buf to see if any input has been
> collected in order to determine if we are within or between queries.
> I'd originally thought this'd need to be a lot more complicated,
> but as long as psqlscan.l continues to drop pre-query comments,
> this seems to be enough.
>
> 0003 is the same patch I posted before to adjust M-# behavior.
>

I did some testing of the patches against the 4 problems that I
originally reported, and they fixed all of them.
0002 is definitely simpler than my original effort.
The patches LGTM.
Thanks for working on this.
(BTW, the patches are in Windows CRLF format, so on Linux at least I
needed to convert them using dos2unix so they'd apply using Git)

Regards,
Greg Nancarrow
Fujitsu Australia

On 2021-Nov-29, Tom Lane wrote:

> After some further hackery, here's a set of patches that I think
> might be acceptable.  They're actually fairly independent, although
> they touch different aspects of the same behavior.

I tried the collection and I find the overall behavior quite convenient.
I think it works just as I wish it would.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"The eagle never lost so much time, as
when he submitted to learn of the crow." (William Blake)

Greg Nancarrow <gregn4422@gmail.com> writes:
> (BTW, the patches are in Windows CRLF format, so on Linux at least I
> needed to convert them using dos2unix so they'd apply using Git)

Hmm.  Applying "od -c" to the copy of that message that's in my
PG list folder shows clearly that there's no \r in it, nor do
I see any when I save off the attachment.  I suppose this must
be an artifact of the way that your MUA treats text attachments;
or maybe the mail got mangled on its way to you.

            regards, tom lane

On Tue, Nov 30, 2021 at 11:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Greg Nancarrow <gregn4422@gmail.com> writes:
> > (BTW, the patches are in Windows CRLF format, so on Linux at least I
> > needed to convert them using dos2unix so they'd apply using Git)
>
> Hmm.  Applying "od -c" to the copy of that message that's in my
> PG list folder shows clearly that there's no \r in it, nor do
> I see any when I save off the attachment.  I suppose this must
> be an artifact of the way that your MUA treats text attachments;
> or maybe the mail got mangled on its way to you.
>

Yeah, sorry, looks like it could be a Gmail issue for me.
When I alternatively downloaded your patches from the pgsql-hackers
archive, they're in Unix format, as you say.
After a bit of investigation, it seems that patch attachments (like
yours) with a Context-Type of "text/x-diff" download through Gmail in
CRLF format for me (I'm running a browser on Windows, but my Postgres
development environment is in a Linux VM). So those must get converted
from Unix to CRLF format if downloaded using a browser running on
Windows.
The majority of patch attachments (?) seem to have a Context-Type of
"application/octet-stream" or "text/x-patch", and these seem to
download raw (in their original Unix format).
I guess the attachment context-type is varying according to the mail
client used for posting.

Sorry for the noise.

Regards,
Greg Nancarrow
Fujitsu Australia

On Tue, Nov 30, 2021 at 12:15 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> Yeah, sorry, looks like it could be a Gmail issue for me.
> When I alternatively downloaded your patches from the pgsql-hackers
> archive, they're in Unix format, as you say.
> After a bit of investigation, it seems that patch attachments (like
> yours) with a Context-Type of "text/x-diff" download through Gmail in
> CRLF format for me (I'm running a browser on Windows, but my Postgres
> development environment is in a Linux VM). So those must get converted
> from Unix to CRLF format if downloaded using a browser running on
> Windows.
> The majority of patch attachments (?) seem to have a Context-Type of
> "application/octet-stream" or "text/x-patch", and these seem to
> download raw (in their original Unix format).
> I guess the attachment context-type is varying according to the mail
> client used for posting.
>

Oops, typos, I meant to say "Content-Type".

Regards,
Greg Nancarrow
Fujitsu Australia

Greg Nancarrow <gregn4422@gmail.com> writes:
> After a bit of investigation, it seems that patch attachments (like
> yours) with a Context-Type of "text/x-diff" download through Gmail in
> CRLF format for me (I'm running a browser on Windows, but my Postgres
> development environment is in a Linux VM). So those must get converted
> from Unix to CRLF format if downloaded using a browser running on
> Windows.
> The majority of patch attachments (?) seem to have a Context-Type of
> "application/octet-stream" or "text/x-patch", and these seem to
> download raw (in their original Unix format).

Interesting.  I can probably adjust my MUA to send "text/x-patch",
but I'll have to look around to see where that's determined.
(I dislike using "application/octet-stream" for this, because
the archives won't show that as text, they only let you download
the attachment.  Maybe that's more Safari's fault than the
archives per se, not sure.)

            regards, tom lane

On Mon, Nov 29, 2021 at 11:12:35PM -0500, Tom Lane wrote:
> Greg Nancarrow <gregn4422@gmail.com> writes:
> > After a bit of investigation, it seems that patch attachments (like
> > yours) with a Context-Type of "text/x-diff" download through Gmail in
> > CRLF format for me (I'm running a browser on Windows, but my Postgres
> > development environment is in a Linux VM). So those must get converted
> > from Unix to CRLF format if downloaded using a browser running on
> > Windows.
> > The majority of patch attachments (?) seem to have a Context-Type of
> > "application/octet-stream" or "text/x-patch", and these seem to
> > download raw (in their original Unix format).
> 
> Interesting.  I can probably adjust my MUA to send "text/x-patch",
> but I'll have to look around to see where that's determined.
> (I dislike using "application/octet-stream" for this, because
> the archives won't show that as text, they only let you download
> the attachment.  Maybe that's more Safari's fault than the
> archives per se, not sure.)

I would be interesting to know if "text/x-patch" is better than
"text/x-diff" --- I currently use the later.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Bruce Momjian <bruce@momjian.us> writes:
> On Mon, Nov 29, 2021 at 11:12:35PM -0500, Tom Lane wrote:
>> Interesting.  I can probably adjust my MUA to send "text/x-patch",
>> but I'll have to look around to see where that's determined.

> I would be interesting to know if "text/x-patch" is better than
> "text/x-diff" --- I currently use the later.

I found out that where that is coming from is "file -i", so I'm a
bit loath to modify it.  Is there any hard documentation as to why
"text/x-patch" should be preferred?

            regards, tom lane

On Tue, Nov 30, 2021 at 04:35:13PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > On Mon, Nov 29, 2021 at 11:12:35PM -0500, Tom Lane wrote:
> >> Interesting.  I can probably adjust my MUA to send "text/x-patch",
> >> but I'll have to look around to see where that's determined.
> 
> > I would be interesting to know if "text/x-patch" is better than
> > "text/x-diff" --- I currently use the later.
> 
> I found out that where that is coming from is "file -i", so I'm a
> bit loath to modify it.  Is there any hard documentation as to why
> "text/x-patch" should be preferred?

I thought this was happening from /etc/mime.types:

    text/x-diff                                     diff patch

The file extensions 'diff' and 'patch' trigger mime to use text/x-diff
for its attachments, at least on Debian.  Based on that, I assumed
"text/x-diff" was more standardized than "text/x-patch".

However, it seems file -i also looks at the contents since a file with a
single word in it is not recognized as a diff:

    $ git diff > /rtmp/x.diff
    $ file -i /rtmp/x.diff
    /rtmp/x.diff: text/x-diff; charset=us-ascii
                  -----------
    $ echo test > /rtmp/x.diff
    $ file -i /rtmp/x.diff
    /rtmp/x.diff: text/plain; charset=us-ascii
                  ----------

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Nov-29, Tom Lane wrote:
>> After some further hackery, here's a set of patches that I think
>> might be acceptable.  They're actually fairly independent, although
>> they touch different aspects of the same behavior.

> I tried the collection and I find the overall behavior quite convenient.
> I think it works just as I wish it would.

Hearing no further comments, pushed.

            regards, tom lane