Feedback-ID: ia2694551:Fastmail
Date: Fri, 19 May 2023 13:02:05 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Andrew Dunstan <andrew@dunslane.net>
Cc: Platon Pronko <platon7pronko@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: very long record lines in expanded psql output
Message-ID: <20230519110205.updpbjiuqgbox6gp@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="7dltxkhfrj6skipq"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <46f0313e-2097-2ad6-8965-45af62af3243@dunslane.net>
Archived-At: 
 <https://www.postgresql.org/message-id/20230519110205.updpbjiuqgbox6gp%40alvherre.pgsql>
Precedence: bulk


--7dltxkhfrj6skipq
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2022-Jul-25, Andrew Dunstan wrote:

> Committed. There were a couple of bits missing, which I filled in, and I
> changed the behaviour when the option is given without an argument, so
> that instead of resetting it the current value is shown, similarly to
> how most pset options work.

I was translating the new messages introduced by this commit, and
decided to unify them.  While doing so, I notice that the feature
misbehaves when you give it a string value that doesn't match any valid
value: it just resets the value to 0, which is entirely the wrong thing.
For other settings, we first verify that the given value is valid, and
only then we change the setting.

So I came up with the attached.  While at it, I noticed that we have
other uses of atoi() there, most notably pager_min_lines.  My first
impulse was to just to do the same as for xheader_width, namely to test
whether the atoid result is zero, and not change in that case.  But then
I noticed we're already using variables.c facilities, so I decided to do
likewise; this results in simpler code.  So we're now stricter, because
variables.c rejects trailing junk after the number.  (123asd was
previously parsed as 123, now it's an error.)


There remain atoi uses in this code, and aside from the atoi()-induced
behavior of making any non-number input set it to 0, it does stuff like

=# \pset border 65538asd
Border style is 2.

because border style is short int, so this value overflows it.  Same
happens with 'columns'.  However, this is all very old code and nobody
seems to have complained.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"¿Cómo puedes confiar en algo que pagas y que no ves,
y no confiar en algo que te dan y te lo muestran?" (Germán Poo)

--7dltxkhfrj6skipq
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-Tweak-xheader_width-input-parsing.patch"

From 5eb799984252cf231064d6d174e13d63880577df Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri, 19 May 2023 12:58:54 +0200
Subject: [PATCH] Tweak xheader_width input parsing

Don't throw away the previous value when an invalid value is proposed.
Also, change the error messages to not need separate translations.

While at it, change pager_min_lines to also avoid changing the setting
when an invalid value is given.
---
 src/bin/psql/command.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 607a57715a3..07c5f026b9b 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -4521,13 +4521,16 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 			popt->topt.expanded_header_width_type = PRINT_XHEADER_PAGE;
 		else
 		{
-			popt->topt.expanded_header_width_type = PRINT_XHEADER_EXACT_WIDTH;
-			popt->topt.expanded_header_exact_width = atoi(value);
-			if (popt->topt.expanded_header_exact_width == 0)
+			int		intval = atoi(value);
+
+			if (intval == 0)
 			{
 				pg_log_error("\\pset: allowed xheader_width values are full (default), column, page, or a number specifying the exact width.");
 				return false;
 			}
+
+			popt->topt.expanded_header_width_type = PRINT_XHEADER_EXACT_WIDTH;
+			popt->topt.expanded_header_exact_width = intval;
 		}
 	}
 
@@ -4660,8 +4663,9 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 	/* set minimum lines for pager use */
 	else if (strcmp(param, "pager_min_lines") == 0)
 	{
-		if (value)
-			popt->topt.pager_min_lines = atoi(value);
+		if (value &&
+			!ParseVariableNum(value, "pager_min_lines", &popt->topt.pager_min_lines))
+			return false;
 	}
 
 	/* disable "(x rows)" footer */
@@ -4727,11 +4731,11 @@ printPsetInfo(const char *param, printQueryOpt *popt)
 	else if (strcmp(param, "xheader_width") == 0)
 	{
 		if (popt->topt.expanded_header_width_type == PRINT_XHEADER_FULL)
-			printf(_("Expanded header width is 'full'.\n"));
+			printf(_("Expanded header width is '%s'.\n"), "full");
 		else if (popt->topt.expanded_header_width_type == PRINT_XHEADER_COLUMN)
-			printf(_("Expanded header width is 'column'.\n"));
+			printf(_("Expanded header width is '%s'.\n"), "column");
 		else if (popt->topt.expanded_header_width_type == PRINT_XHEADER_PAGE)
-			printf(_("Expanded header width is 'page'.\n"));
+			printf(_("Expanded header width is '%s'.\n"), "page");
 		else if (popt->topt.expanded_header_width_type == PRINT_XHEADER_EXACT_WIDTH)
 			printf(_("Expanded header width is %d.\n"), popt->topt.expanded_header_exact_width);
 	}

base-commit: 0b8ace8d773257fffeaceda196ed94877c2b74df
-- 
2.39.2


--7dltxkhfrj6skipq--