From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Subject: Make psql ignore trailing semicolons in \sf, \ef, etc
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <2012221.1704746892.0@sss.pgh.pa.us>
Date: Mon, 08 Jan 2024 15:48:32 -0500
Message-ID: <2012251.1704746912@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2012251.1704746912%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2012221.1704746892.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

We had a complaint (see [1], but it's not the first IIRC) about how
psql doesn't behave very nicely if one ends \sf or allied commands
with a semicolon:

regression=3D# \sf sin(float8);
ERROR:  expected a right parenthesis

This is a bit of a usability gotcha, since many other backslash
commands are forgiving about trailing semicolons.  I looked at
the code and found that it's actually trying to ignore semicolons,
by passing semicolon =3D true to psql_scan_slash_option.  But that
fails to work because it's also passing type =3D OT_WHOLE_LINE,
and the whole-line code path ignores the semicolon flag.  Probably
that's just because nobody needed to use that combination back in
the day.  There's another user of OT_WHOLE_LINE, exec_command_help,
which also wants this behavior and has written its own stripping
code to get it.  That seems pretty silly, so here's a quick finger
exercise to move that logic into psql_scan_slash_option.

Is this enough of a bug to deserve back-patching?  I'm not sure.

			regards, tom lane

[1] https://www.postgresql.org/message-id/CAEs%3D6D%3DnwX2wm0hjkaw6C_LnqR%=
2BNFtnnzbSzeZq-xcfi_ooKSw%40mail.gmail.com


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="ignore-trailing-semi-in-sf-ef-sv-ev.patch";
	charset="us-ascii"
Content-ID: <2012221.1704746892.2@sss.pgh.pa.us>
Content-Description: ignore-trailing-semi-in-sf-ef-sv-ev.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 9103bc3465..2f74bfdc96 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1640,18 +1640,7 @@ exec_command_help(PsqlScanState scan_state, bool ac=
tive_branch)
 	if (active_branch)
 	{
 		char	   *opt =3D psql_scan_slash_option(scan_state,
-												 OT_WHOLE_LINE, NULL, false);
-		size_t		len;
-
-		/* strip any trailing spaces and semicolons */
-		if (opt)
-		{
-			len =3D strlen(opt);
-			while (len > 0 &&
-				   (isspace((unsigned char) opt[len - 1])
-					|| opt[len - 1] =3D=3D ';'))
-				opt[--len] =3D '\0';
-		}
+												 OT_WHOLE_LINE, NULL, true);
 =

 		helpSQL(opt, pset.popt.topt.pager);
 		free(opt);
@@ -3165,6 +3154,10 @@ ignore_slash_filepipe(PsqlScanState scan_state)
  * This *MUST* be used for inactive-branch processing of any slash comman=
d
  * that takes an OT_WHOLE_LINE option.  Otherwise we might consume a diff=
erent
  * amount of option text in active and inactive cases.
+ *
+ * Note: although callers might pass "semicolon" as either true or false,
+ * we need not duplicate that here, since it doesn't affect the amount of
+ * input text consumed.
  */
 static void
 ignore_slash_whole_line(PsqlScanState scan_state)
diff --git a/src/bin/psql/psqlscanslash.l b/src/bin/psql/psqlscanslash.l
index 514977e59d..10a35dffc4 100644
--- a/src/bin/psql/psqlscanslash.l
+++ b/src/bin/psql/psqlscanslash.l
@@ -18,6 +18,8 @@
  */
 #include "postgres_fe.h"
 =

+#include <ctype.h>
+
 #include "common.h"
 #include "psqlscanslash.h"
 =

@@ -640,7 +642,22 @@ psql_scan_slash_option(PsqlScanState state,
 			termPQExpBuffer(&mybuf);
 			return NULL;
 		case xslashwholeline:
-			/* always okay */
+			/*
+			 * In whole-line mode, we interpret semicolon =3D true as stripping
+			 * trailing whitespace as well as semi-colons; this gives the
+			 * nearest equivalent to what semicolon =3D true does in normal
+			 * mode.  Note there's no concept of quoting in this mode.
+			 */
+			if (semicolon)
+			{
+				while (mybuf.len > 0 &&
+					   (mybuf.data[mybuf.len - 1] =3D=3D ';' ||
+						(isascii((unsigned char) mybuf.data[mybuf.len - 1]) &&
+						 isspace((unsigned char) mybuf.data[mybuf.len - 1]))))
+				{
+					mybuf.data[--mybuf.len] =3D '\0';
+				}
+			}
 			break;
 		default:
 			/* can't get here */
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expecte=
d/psql.out
index 5d61e4c7bb..4f3fd46420 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -5323,7 +5323,7 @@ END
          LANGUAGE sql
          IMMUTABLE PARALLEL SAFE STRICT COST 1
 1       RETURN ($2 + $1)
-\sf ts_debug(text)
+\sf ts_debug(text);
 CREATE OR REPLACE FUNCTION pg_catalog.ts_debug(document text, OUT alias t=
ext, OUT description text, OUT token text, OUT dictionaries regdictionary[=
], OUT dictionary regdictionary, OUT lexemes text[])
  RETURNS SETOF record
  LANGUAGE sql
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index f199d624d3..c997106b9f 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1315,7 +1315,7 @@ drop role regress_psql_user;
 \sf information_schema._pg_index_position
 \sf+ information_schema._pg_index_position
 \sf+ interval_pl_time
-\sf ts_debug(text)
+\sf ts_debug(text);
 \sf+ ts_debug(text)
 =

 -- AUTOCOMMIT

------- =_aaaaaaaaaa0--