Subject: Re: psql - add SHOW_ALL_RESULTS option
To: Fabien COELHO <coelho@cri.ensmp.fr>
Cc: "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
References: <69C0B369-570C-4524-8EE4-BCCACECB6BEE@amazon.com>
 <20210412193301.GA11204@alvherre.pgsql> <YHeVHH0wVcRwdogL@paquier.xyz>
 <3738387.1618451507@sss.pgh.pa.us>
 <alpine.DEB.2.22.394.2104151336060.191178@pseudo>
 <451a2111-3757-c511-9413-8f8af531a167@enterprisedb.com>
 <alpine.DEB.2.22.394.2106121130400.4105466@pseudo>
 <CALDaNm0+pzGgp-M1mAoUVYRtjadbWYuB96J8RaV9iH3+HA8FTA@mail.gmail.com>
 <alpine.DEB.2.22.394.2107151742300.1705285@pseudo>
 <18c78a01-4a34-9dd4-f78b-6860f1420c8e@enterprisedb.com>
 <alpine.DEB.2.22.394.2107221655020.25741@pseudo>
 <6954328d-96f2-77f7-735f-7ce493a40949@enterprisedb.com>
 <alpine.DEB.2.22.394.2109251058000.3216312@pseudo>
 <alpine.DEB.2.22.394.2110021622530.2817726@pseudo>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Message-ID: <2570e2ae-fa0f-aac9-f72f-bb59a9983a20@enterprisedb.com>
Date: Fri, 8 Oct 2021 14:15:31 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.22.394.2110021622530.2817726@pseudo>
Content-Type: multipart/mixed;
 boundary="------------26D2DAE88A07B42478BD193F"
Content-Language: en-US
Archived-At: 
 <https://www.postgresql.org/message-id/2570e2ae-fa0f-aac9-f72f-bb59a9983a20%40enterprisedb.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------26D2DAE88A07B42478BD193F
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit

On 02.10.21 16:31, Fabien COELHO wrote:
>> Attached v9 integrates your tests and makes them work.
> 
> Attached v11 is a rebase.

This patch still has a few of the problems reported earlier this year.

In [0], it was reported that certain replication commands result in 
infinite loops because of faulty error handling.  This still happens.  I 
wrote a test for it, attached here.  (I threw in a few more basic tests, 
just to have some more coverage that was lacking, and to have a file to 
put the new test in.)

In [1], it was reported that server crashes produce duplicate error 
messages.  This also still happens.  I didn't write a test for it, maybe 
you have an idea.  (Obviously, we could check whether the error message 
is literally there twice in the output, but that doesn't seem very 
general.)  But it's easy to test manually: just have psql connect, shut 
down the server, then run a query.

Additionally, I looked into the Coverity issue reported in [2].  That 
one is fixed, but I figured it would be good to be able to check your 
patches with a static analyzer in a similar way.  I don't have the 
ability to run Coverity locally, so I looked at scan-build and fixed a 
few minor warnings, also attached as a patch.  Your current patch 
appears to be okay in that regard.


[0]: 
https://www.postgresql.org/message-id/69C0B369-570C-4524-8EE4-BCCACECB6BEE@amazon.com

[1]: https://www.postgresql.org/message-id/2902362.1618244606@sss.pgh.pa.us

[2]: https://www.postgresql.org/message-id/2680034.1618157764@sss.pgh.pa.us

--------------26D2DAE88A07B42478BD193F
Content-Type: text/plain; charset=UTF-8; x-mac-type="0"; x-mac-creator="0";
 name="0001-psql-More-tests.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="0001-psql-More-tests.patch"

From e8dbe137737f94a2eaff86dc1676f9df39c60d00 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 7 Oct 2021 22:12:40 +0200
Subject: [PATCH] psql: More tests

---
 src/bin/psql/t/001_basic.pl | 42 +++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 src/bin/psql/t/001_basic.pl

diff --git a/src/bin/psql/t/001_basic.pl b/src/bin/psql/t/001_basic.pl
new file mode 100644
index 0000000000..cd899e851e
--- /dev/null
+++ b/src/bin/psql/t/001_basic.pl
@@ -0,0 +1,42 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More tests => 25;
+
+program_help_ok('psql');
+program_version_ok('psql');
+program_options_handling_ok('psql');
+
+my ($stdout, $stderr);
+my $result;
+
+# test --help=foo, analogous to program_help_ok()
+foreach my $arg (qw(commands variables))
+{
+	$result = IPC::Run::run [ 'psql', "--help=$arg" ], '>', \$stdout, '2>', \$stderr;
+	ok($result, "psql --help=$arg exit code 0");
+	isnt($stdout, '', "psql --help=$arg goes to stdout");
+	is($stderr, '', "psql --help=$arg nothing to stderr");
+}
+
+my $node = PostgresNode->new('main');
+$node->init;
+$node->append_conf(
+	'postgresql.conf', q{
+wal_level = 'logical'
+max_replication_slots = 4
+max_wal_senders = 4
+});
+$node->start;
+
+$node->command_like([ 'psql', '-c', '\copyright' ], qr/Copyright/, '\copyright');
+$node->command_like([ 'psql', '-c', '\help' ], qr/ALTER/, '\help without arguments');
+$node->command_like([ 'psql', '-c', '\help SELECT' ], qr/SELECT/, '\help');
+
+$node->command_fails_like([ 'psql', 'replication=database', '-c', 'START_REPLICATION 123/456' ],
+	qr/^unexpected PQresultStatus: 8$/, 'handling of unexpected PQresultStatus');
-- 
2.33.0


--------------26D2DAE88A07B42478BD193F
Content-Type: text/plain; charset=UTF-8; x-mac-type="0"; x-mac-creator="0";
 name="0001-psql-Fix-scan-build-warnings.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="0001-psql-Fix-scan-build-warnings.patch"

From 96634e3dc5f775b73a4142e9a5d83190bd9aecbb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 8 Oct 2021 13:30:15 +0200
Subject: [PATCH] psql: Fix scan-build warnings

---
 src/bin/psql/common.c   | 32 ++++++++++++++++++--------------
 src/bin/psql/copy.c     |  1 -
 src/bin/psql/describe.c |  1 -
 3 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 5640786678..1b224bf9e4 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -594,6 +594,7 @@ PSQLexec(const char *query)
 int
 PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE *printQueryFout)
 {
+	bool		timing = pset.timing;
 	PGresult   *res;
 	double		elapsed_msec = 0;
 	instr_time	before;
@@ -608,7 +609,7 @@ PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE *printQueryFout)
 
 	SetCancelConn(pset.db);
 
-	if (pset.timing)
+	if (timing)
 		INSTR_TIME_SET_CURRENT(before);
 
 	res = PQexec(pset.db, query);
@@ -621,7 +622,7 @@ PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE *printQueryFout)
 		return 0;
 	}
 
-	if (pset.timing)
+	if (timing)
 	{
 		INSTR_TIME_SET_CURRENT(after);
 		INSTR_TIME_SUBTRACT(after, before);
@@ -674,7 +675,7 @@ PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE *printQueryFout)
 	fflush(fout);
 
 	/* Possible microtiming output */
-	if (pset.timing)
+	if (timing)
 		PrintTiming(elapsed_msec);
 
 	return 1;
@@ -1192,6 +1193,7 @@ PrintQueryResults(PGresult *results)
 bool
 SendQuery(const char *query)
 {
+	bool		timing = pset.timing;
 	PGresult   *results;
 	PGTransactionStatusType transaction_status;
 	double		elapsed_msec = 0;
@@ -1300,7 +1302,7 @@ SendQuery(const char *query)
 		instr_time	before,
 					after;
 
-		if (pset.timing)
+		if (timing)
 			INSTR_TIME_SET_CURRENT(before);
 
 		results = PQexec(pset.db, query);
@@ -1309,7 +1311,7 @@ SendQuery(const char *query)
 		ResetCancelConn();
 		OK = ProcessResult(&results);
 
-		if (pset.timing)
+		if (timing)
 		{
 			INSTR_TIME_SET_CURRENT(after);
 			INSTR_TIME_SUBTRACT(after, before);
@@ -1400,7 +1402,7 @@ SendQuery(const char *query)
 	ClearOrSaveResult(results);
 
 	/* Possible microtiming output */
-	if (pset.timing)
+	if (timing)
 		PrintTiming(elapsed_msec);
 
 	/* check for events that may occur during query execution */
@@ -1471,6 +1473,7 @@ SendQuery(const char *query)
 static bool
 DescribeQuery(const char *query, double *elapsed_msec)
 {
+	bool		timing = pset.timing;
 	PGresult   *results;
 	bool		OK;
 	instr_time	before,
@@ -1478,7 +1481,7 @@ DescribeQuery(const char *query, double *elapsed_msec)
 
 	*elapsed_msec = 0;
 
-	if (pset.timing)
+	if (timing)
 		INSTR_TIME_SET_CURRENT(before);
 
 	/*
@@ -1550,7 +1553,7 @@ DescribeQuery(const char *query, double *elapsed_msec)
 			results = PQexec(pset.db, buf.data);
 			OK = AcceptResult(results);
 
-			if (pset.timing)
+			if (timing)
 			{
 				INSTR_TIME_SET_CURRENT(after);
 				INSTR_TIME_SUBTRACT(after, before);
@@ -1591,6 +1594,7 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
 	PGresult   *results;
 	PQExpBufferData buf;
 	printQueryOpt my_popt = pset.popt;
+	bool		timing = pset.timing;
 	FILE	   *fout;
 	bool		is_pipe;
 	bool		is_pager = false;
@@ -1610,7 +1614,7 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
 	my_popt.topt.stop_table = false;
 	my_popt.topt.prior_records = 0;
 
-	if (pset.timing)
+	if (timing)
 		INSTR_TIME_SET_CURRENT(before);
 
 	/* if we're not in a transaction, start one */
@@ -1640,7 +1644,7 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
 	if (!OK)
 		goto cleanup;
 
-	if (pset.timing)
+	if (timing)
 	{
 		INSTR_TIME_SET_CURRENT(after);
 		INSTR_TIME_SUBTRACT(after, before);
@@ -1682,13 +1686,13 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
 
 	for (;;)
 	{
-		if (pset.timing)
+		if (timing)
 			INSTR_TIME_SET_CURRENT(before);
 
 		/* get fetch_count tuples at a time */
 		results = PQexec(pset.db, fetch_cmd);
 
-		if (pset.timing)
+		if (timing)
 		{
 			INSTR_TIME_SET_CURRENT(after);
 			INSTR_TIME_SUBTRACT(after, before);
@@ -1802,7 +1806,7 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
 	}
 
 cleanup:
-	if (pset.timing)
+	if (timing)
 		INSTR_TIME_SET_CURRENT(before);
 
 	/*
@@ -1828,7 +1832,7 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
 		ClearOrSaveResult(results);
 	}
 
-	if (pset.timing)
+	if (timing)
 	{
 		INSTR_TIME_SET_CURRENT(after);
 		INSTR_TIME_SUBTRACT(after, before);
diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c
index 64ab40c4f7..3c4d862bdf 100644
--- a/src/bin/psql/copy.c
+++ b/src/bin/psql/copy.c
@@ -660,7 +660,6 @@ handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)
 				if (PQputCopyData(conn, buf, buflen) <= 0)
 				{
 					OK = false;
-					copydone = true;
 					break;
 				}
 
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index a33d77c0ef..ea4ca5c05c 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -635,7 +635,6 @@ describeFunctions(const char *functypes, const char *func_pattern,
 				appendPQExpBufferStr(&buf, "p.prokind = 'w'\n");
 			else
 				appendPQExpBufferStr(&buf, "p.proiswindow\n");
-			needs_or = true;
 		}
 		appendPQExpBufferStr(&buf, "      )\n");
 	}
-- 
2.33.0


--------------26D2DAE88A07B42478BD193F--