[patch] \g with multiple result sets and \watch with copy queries

Started by Daniel Veriteover 3 years ago9 messages
#1Daniel Verite
daniel@manitou-mail.org
2 attachment(s)

Hi,

The psql improvement in v15 to output multiple result sets does not
behave as one might expect with \g: the output file or program
to pipe into is opened/closed on each result set, overwriting the
previous ones in the case of \g file.

Example:

psql -At <<EOF
-- good (two results output)
select 1\; select 2;

-- bad: ends up with only "2" in the file
select 1\; select 2 \g file

EOF

That problem with \g is due to PrintQueryTuples() an HandleCopyResult()
still having the responsibility to open/close the output stream.
I think this code should be moved upper in the call stack, in
ExecQueryAndProcessResults().

The first attached patch implements a fix that way.

When testing this I've stumbled on another issue nearby: COPY TO
STDOUT followed by \watch should normally produce the error message
"\watch cannot be used with COPY", but the execution goes into a
infinite busy loop instead.
This is because ClearOrSaveAllResults() loops over PQgetResult() until
it returns NULL, but as it turns out, that never happens: it seems
stuck on a PGRES_COPY_OUT result.

While looking to fix that, it occurred to me that it would be
simpler to allow \watch to deal with COPY results rather than
continuing to disallow it. ISTM that before v15, the reason
why PSQLexecWatch() did not want to deal with COPY was to not
bother with a niche use case, rather than because of some
specific impossibility with it.
Now that it calls the generic ExecQueryAndProcessResults() code
that can handle COPY transfers, \watch on a COPY query seems to work
fine if not disallowed.
Besides, v15 adds the possibility to feed \watch output into
a program through PSQL_WATCH_PAGER, and since the copy format is
the best format to be consumed by programs, this seems like
a good reason to allow COPY TO STDOUT with it.
\watch on a COPY FROM STDIN query doesn't make much sense,
but it can be interrupted with ^C if run by mistake, so I don't see a
need to disallow it specifically.

So the second patch fixes the infinite loop problem like that, on top of
the first patch.

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

Attachments:

01-psql-backslash-g-fix.patchtext/plainDownload
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index e611e3266d..88f4b159f9 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -669,42 +669,13 @@ PrintQueryTuples(const PGresult *result, const printQueryOpt *opt, FILE *printQu
 {
 	bool		ok = true;
 
-	/* write output to \g argument, if any */
-	if (pset.gfname)
-	{
-		FILE	   *fout;
-		bool		is_pipe;
-
-		if (!openQueryOutputFile(pset.gfname, &fout, &is_pipe))
-			return false;
-		if (is_pipe)
-			disable_sigpipe_trap();
-
-		printQuery(result, &pset.popt, fout, false, pset.logfile);
-		if (ferror(fout))
-		{
-			pg_log_error("could not print result table: %m");
-			ok = false;
-		}
+	FILE	   *fout = printQueryFout ? printQueryFout : pset.queryFout;
 
-		if (is_pipe)
-		{
-			pclose(fout);
-			restore_sigpipe_trap();
-		}
-		else
-			fclose(fout);
-	}
-	else
+	printQuery(result, opt ? opt : &pset.popt, fout, false, pset.logfile);
+	if (ferror(fout))
 	{
-		FILE	   *fout = printQueryFout ? printQueryFout : pset.queryFout;
-
-		printQuery(result, opt ? opt : &pset.popt, fout, false, pset.logfile);
-		if (ferror(fout))
-		{
-			pg_log_error("could not print result table: %m");
-			ok = false;
-		}
+		pg_log_error("could not print result table: %m");
+		ok = false;
 	}
 
 	return ok;
@@ -861,10 +832,9 @@ loop_exit:
  * want if the status line doesn't get taken as part of the COPY data.
  */
 static bool
-HandleCopyResult(PGresult **resultp)
+HandleCopyResult(PGresult **resultp, FILE *copystream)
 {
 	bool		success;
-	FILE	   *copystream;
 	PGresult   *copy_result;
 	ExecStatusType result_status = PQresultStatus(*resultp);
 
@@ -875,33 +845,6 @@ HandleCopyResult(PGresult **resultp)
 
 	if (result_status == PGRES_COPY_OUT)
 	{
-		bool		need_close = false;
-		bool		is_pipe = false;
-
-		if (pset.copyStream)
-		{
-			/* invoked by \copy */
-			copystream = pset.copyStream;
-		}
-		else if (pset.gfname)
-		{
-			/* invoked by \g */
-			if (openQueryOutputFile(pset.gfname,
-									&copystream, &is_pipe))
-			{
-				need_close = true;
-				if (is_pipe)
-					disable_sigpipe_trap();
-			}
-			else
-				copystream = NULL;	/* discard COPY data entirely */
-		}
-		else
-		{
-			/* fall back to the generic query output stream */
-			copystream = pset.queryFout;
-		}
-
 		success = handleCopyOut(pset.db,
 								copystream,
 								&copy_result)
@@ -917,24 +860,11 @@ HandleCopyResult(PGresult **resultp)
 			PQclear(copy_result);
 			copy_result = NULL;
 		}
-
-		if (need_close)
-		{
-			/* close \g argument file/pipe */
-			if (is_pipe)
-			{
-				pclose(copystream);
-				restore_sigpipe_trap();
-			}
-			else
-			{
-				fclose(copystream);
-			}
-		}
 	}
 	else
 	{
 		/* COPY IN */
+		/* Ignore the copystream argument passed to the function */
 		copystream = pset.copyStream ? pset.copyStream : pset.cur_cmd_source;
 		success = handleCopyIn(pset.db,
 							   copystream,
@@ -1445,6 +1375,8 @@ ExecQueryAndProcessResults(const char *query, double *elapsed_msec, bool *svpt_g
 	instr_time	before,
 				after;
 	PGresult   *result;
+	bool		is_pipe = false;
+	FILE		*results_fout = NULL;
 
 	if (timing)
 		INSTR_TIME_SET_CURRENT(before);
@@ -1555,6 +1487,8 @@ ExecQueryAndProcessResults(const char *query, double *elapsed_msec, bool *svpt_g
 		if (result_status == PGRES_COPY_IN ||
 			result_status == PGRES_COPY_OUT)
 		{
+			FILE *copy_stream = NULL;
+
 			if (is_watch)
 			{
 				ClearOrSaveAllResults();
@@ -1562,7 +1496,36 @@ ExecQueryAndProcessResults(const char *query, double *elapsed_msec, bool *svpt_g
 				return -1;
 			}
 
-			success &= HandleCopyResult(&result);
+			if (pset.copyStream) /* invoked by \copy */
+			{
+				copy_stream = pset.copyStream;
+			}
+			else if (pset.gfname)
+			{
+				if (results_fout==NULL)
+				{
+					if (openQueryOutputFile(pset.gfname, &results_fout, &is_pipe))
+					{
+						if (is_pipe)
+							disable_sigpipe_trap();
+						copy_stream = results_fout;
+					}
+					else
+						success = false;
+				}
+				else
+					copy_stream = results_fout;
+			}
+			else
+			{
+				/* fall back to the generic query output stream */
+				copy_stream = pset.queryFout;
+			}
+
+			/* Even if the output stream could not be opened, call
+			   HandleCopyResult() with a NULL output stream to collect the
+			   COPY data. */
+			success &= HandleCopyResult(&result, copy_stream);
 		}
 
 		/*
@@ -1594,7 +1557,24 @@ ExecQueryAndProcessResults(const char *query, double *elapsed_msec, bool *svpt_g
 
 		/* this may or may not print something depending on settings */
 		if (result != NULL)
-			success &= PrintQueryResult(result, last, false, opt, printQueryFout);
+		{
+			/* if results need to be printed into the file specified by
+			   \g, open it */
+			if (PQresultStatus(result) == PGRES_TUPLES_OK &&
+				pset.gfname && results_fout==NULL)
+			{
+				if (openQueryOutputFile(pset.gfname, &results_fout, &is_pipe))
+				{
+					if (is_pipe)
+						disable_sigpipe_trap();
+					printQueryFout = results_fout;
+				}
+				else
+					success = false;
+			}
+			if (success)
+				success &= PrintQueryResult(result, last, false, opt, printQueryFout);
+		}
 
 		/* set variables on last result if all went well */
 		if (!is_watch && last && success)
@@ -1610,6 +1590,17 @@ ExecQueryAndProcessResults(const char *query, double *elapsed_msec, bool *svpt_g
 		}
 	}
 
+	if (results_fout)
+	{
+		if (is_pipe)
+		{
+			pclose(results_fout);
+			restore_sigpipe_trap();
+		}
+		else
+			fclose(results_fout);
+	}
+
 	/* may need this to recover from conn loss during COPY */
 	if (!CheckConnection())
 		return -1;
02-psql-copy-watch-fix.patchtext/plainDownload
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 88f4b159f9..5864a9833f 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1351,10 +1351,9 @@ DescribeQuery(const char *query, double *elapsed_msec)
  *
  * Sends query and cycles through PGresult objects.
  *
- * When not under \watch and if our command string contained a COPY FROM STDIN
- * or COPY TO STDOUT, the PGresult associated with these commands must be
- * processed by providing an input or output stream.  In that event, we'll
- * marshal data for the COPY.
+ * If our command string contained a COPY FROM STDIN or COPY TO STDOUT, the
+ * PGresult associated with these commands must be processed by providing an
+ * input or output stream.  In that event, we'll marshal data for the COPY.
  *
  * For other commands, the results are processed normally, depending on their
  * status.
@@ -1491,12 +1490,9 @@ ExecQueryAndProcessResults(const char *query, double *elapsed_msec, bool *svpt_g
 
 			if (is_watch)
 			{
-				ClearOrSaveAllResults();
-				pg_log_error("\\watch cannot be used with COPY");
-				return -1;
+				copy_stream = printQueryFout ? printQueryFout : pset.queryFout;
 			}
-
-			if (pset.copyStream) /* invoked by \copy */
+			else if (pset.copyStream) /* invoked by \copy */
 			{
 				copy_stream = pset.copyStream;
 			}
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Daniel Verite (#1)
Re: [patch] \g with multiple result sets and \watch with copy queries

"Daniel Verite" <daniel@manitou-mail.org> writes:

The psql improvement in v15 to output multiple result sets does not
behave as one might expect with \g: the output file or program
to pipe into is opened/closed on each result set, overwriting the
previous ones in the case of \g file.

Ugh. I think we'd better fix that before 15.0, else somebody may
think this is the new intended behavior and raise compatibility
concerns when we fix it. I will see if I can squeeze it in before
this afternoon's 15rc2 wrap.

regards, tom lane

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#2)
Re: [patch] \g with multiple result sets and \watch with copy queries

I wrote:

Ugh. I think we'd better fix that before 15.0, else somebody may
think this is the new intended behavior and raise compatibility
concerns when we fix it. I will see if I can squeeze it in before
this afternoon's 15rc2 wrap.

Pushed after making some corrections.

Given the time pressure, I did not worry about installing regression
test coverage for this stuff, but I wonder if we shouldn't add some.

regards, tom lane

#4Daniel Verite
daniel@manitou-mail.org
In reply to: Tom Lane (#3)
Re: [patch] \g with multiple result sets and \watch with copy queries

Tom Lane wrote:

Pushed after making some corrections.

Thanks!

Given the time pressure, I did not worry about installing regression
test coverage for this stuff, but I wonder if we shouldn't add some.

Currently, test/regress/sql/psql.sql doesn't AFAICS write anything
outside of stdout, but \g, \o, \copy need to write to external
files to be tested properly.

Looking at nearby tests, I see that commit d1029bb5a26 brings
interesting additions in test/regress/sql/misc.sql that could be used
as a model to handle output files. psql.sql could write
into PG_ABS_BUILDDIR, then read the files back with \copy I guess,
then output that again to stdout for comparison. I'll see if I can get
that to work.

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Daniel Verite (#4)
Re: [patch] \g with multiple result sets and \watch with copy queries

"Daniel Verite" <daniel@manitou-mail.org> writes:

Tom Lane wrote:

Given the time pressure, I did not worry about installing regression
test coverage for this stuff, but I wonder if we shouldn't add some.

Currently, test/regress/sql/psql.sql doesn't AFAICS write anything
outside of stdout, but \g, \o, \copy need to write to external
files to be tested properly.

Yeah, I don't think we can usefully test these in psql.sql, because
file-system side effects are bad in that context. But maybe a TAP
test could cope?

regards, tom lane

#6Daniel Verite
daniel@manitou-mail.org
In reply to: Tom Lane (#5)
1 attachment(s)
Re: [patch] \g with multiple result sets and \watch with copy queries

Tom Lane wrote:

Currently, test/regress/sql/psql.sql doesn't AFAICS write anything
outside of stdout, but \g, \o, \copy need to write to external
files to be tested properly.

Yeah, I don't think we can usefully test these in psql.sql, because
file-system side effects are bad in that context. But maybe a TAP
test could cope?

I've came up with the attached using psql.sql only, at least for
\g and \o writing to files.
This is a bit more complicated than the usual tests, but not
that much.
Any opinions on this?

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

Attachments:

tests-psql-g-o-file.patchtext/plainDownload
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index a7f5700edc..90beedde58 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -5402,6 +5402,136 @@ CONTEXT:  PL/pgSQL function warn(text) line 2 at RAISE
 \set SHOW_ALL_RESULTS on
 DROP FUNCTION warn(TEXT);
 --
+-- \g file
+--
+\getenv abs_builddir PG_ABS_BUILDDIR
+\set outfile1 :abs_builddir '/results/psql-output1'
+-- this table is used to load back the output data from files
+CREATE TEMPORARY TABLE reload_output(
+ lineno int not null generated always as identity,
+ line text
+);
+SELECT 1 AS a \g :outfile1
+COPY reload_output(line) FROM :'outfile1';
+SELECT 2 AS b\; SELECT 3 AS c\; SELECT 4 AS d \g :outfile1
+COPY reload_output(line) FROM :'outfile1';
+COPY (select 'foo') to stdout \; COPY (select 'bar') to stdout \g :outfile1
+COPY reload_output(line) FROM :'outfile1';
+SELECT line FROM reload_output ORDER BY lineno;
+  line   
+---------
+  a 
+ ---
+  1
+ (1 row)
+ 
+  b 
+ ---
+  2
+ (1 row)
+ 
+  c 
+ ---
+  3
+ (1 row)
+ 
+  d 
+ ---
+  4
+ (1 row)
+ 
+ foo
+ bar
+(22 rows)
+
+TRUNCATE TABLE reload_output;
+--
+-- \o file
+--
+\set outfile2 :abs_builddir '/results/psql-output2'
+\o :outfile2
+select max(unique1) from onek;
+SELECT 1 AS a\; SELECT 2 AS b\; SELECT 3 AS c;
+-- COPY TO file
+-- the data goes into :outfile1 and the command status into :outfile2
+\set QUIET false
+COPY (select unique1 from onek order by unique1 limit 10) TO :'outfile1';
+-- DML command status
+UPDATE onek SET unique1=unique1 WHERE false;
+\set QUIET true
+\o
+COPY reload_output(line) FROM :'outfile1';
+SELECT line FROM reload_output ORDER BY lineno;
+ line 
+------
+ 0
+ 1
+ 2
+ 3
+ 4
+ 5
+ 6
+ 7
+ 8
+ 9
+(10 rows)
+
+TRUNCATE TABLE reload_output;
+COPY reload_output(line) FROM :'outfile2';
+SELECT line FROM reload_output ORDER BY lineno;
+   line   
+----------
+  max 
+ -----
+  999
+ (1 row)
+ 
+  a 
+ ---
+  1
+ (1 row)
+ 
+  b 
+ ---
+  2
+ (1 row)
+ 
+  c 
+ ---
+  3
+ (1 row)
+ 
+ COPY 10
+ UPDATE 0
+(22 rows)
+
+TRUNCATE TABLE reload_output;
+\o :outfile2
+-- multiple COPY TO stdout
+-- the data go into :outfile2 and the status is not output
+COPY (select 'foo1') to stdout \; COPY (select 'bar1') to stdout;
+-- combine \o and \g file with multiple COPY queries
+COPY (select 'foo2') to stdout \; COPY (select 'bar2') to stdout \g :outfile1
+\o
+COPY reload_output(line) FROM :'outfile1';
+SELECT line FROM reload_output ORDER BY lineno;
+ line 
+------
+ foo2
+ bar2
+(2 rows)
+
+TRUNCATE TABLE reload_output;
+COPY reload_output(line) FROM :'outfile2';
+SELECT line FROM reload_output ORDER BY lineno;
+ line 
+------
+ foo1
+ bar1
+(2 rows)
+
+TRUNCATE TABLE reload_output;
+--
 -- AUTOCOMMIT and combined queries
 --
 \set AUTOCOMMIT off
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 1149c6a839..c3ae21633d 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1365,6 +1365,72 @@ SELECT 1 AS one \; SELECT warn('1.5') \; SELECT 2 AS two ;
 \set SHOW_ALL_RESULTS on
 DROP FUNCTION warn(TEXT);
 
+--
+-- \g file
+--
+\getenv abs_builddir PG_ABS_BUILDDIR
+\set outfile1 :abs_builddir '/results/psql-output1'
+
+-- this table is used to load back the output data from files
+CREATE TEMPORARY TABLE reload_output(
+ lineno int not null generated always as identity,
+ line text
+);
+
+SELECT 1 AS a \g :outfile1
+COPY reload_output(line) FROM :'outfile1';
+
+SELECT 2 AS b\; SELECT 3 AS c\; SELECT 4 AS d \g :outfile1
+COPY reload_output(line) FROM :'outfile1';
+
+COPY (select 'foo') to stdout \; COPY (select 'bar') to stdout \g :outfile1
+COPY reload_output(line) FROM :'outfile1';
+
+SELECT line FROM reload_output ORDER BY lineno;
+TRUNCATE TABLE reload_output;
+
+--
+-- \o file
+--
+\set outfile2 :abs_builddir '/results/psql-output2'
+
+\o :outfile2
+select max(unique1) from onek;
+SELECT 1 AS a\; SELECT 2 AS b\; SELECT 3 AS c;
+
+-- COPY TO file
+-- the data goes into :outfile1 and the command status into :outfile2
+\set QUIET false
+COPY (select unique1 from onek order by unique1 limit 10) TO :'outfile1';
+-- DML command status
+UPDATE onek SET unique1=unique1 WHERE false;
+\set QUIET true
+\o
+
+COPY reload_output(line) FROM :'outfile1';
+SELECT line FROM reload_output ORDER BY lineno;
+TRUNCATE TABLE reload_output;
+
+COPY reload_output(line) FROM :'outfile2';
+SELECT line FROM reload_output ORDER BY lineno;
+TRUNCATE TABLE reload_output;
+
+\o :outfile2
+-- multiple COPY TO stdout
+-- the data go into :outfile2 and the status is not output
+COPY (select 'foo1') to stdout \; COPY (select 'bar1') to stdout;
+-- combine \o and \g file with multiple COPY queries
+COPY (select 'foo2') to stdout \; COPY (select 'bar2') to stdout \g :outfile1
+\o
+
+COPY reload_output(line) FROM :'outfile1';
+SELECT line FROM reload_output ORDER BY lineno;
+TRUNCATE TABLE reload_output;
+
+COPY reload_output(line) FROM :'outfile2';
+SELECT line FROM reload_output ORDER BY lineno;
+TRUNCATE TABLE reload_output;
+
 --
 -- AUTOCOMMIT and combined queries
 --
#7Corey Huinker
corey.huinker@gmail.com
In reply to: Daniel Verite (#6)
Re: [patch] \g with multiple result sets and \watch with copy queries

This is a bit more complicated than the usual tests, but not
that much.
Any opinions on this?

+1

I think that because it is more complicated than usual psql, we may want to
comment on the intention of the tests and some of the less-than-common psql
elements (\set concatenation, resetting \o, etc). If you see value in that
I can amend the patch.

Are there any options on COPY (header, formats) that we think we should
test as well?

#8Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Daniel Verite (#1)
1 attachment(s)
Re: [patch] \g with multiple result sets and \watch with copy queries

Bonjour Daniel,

Good catch! Thanks for the quick fix!

As usual, what is not tested does not:-(

Attached a tap test to check for the expected behavior with multiple
command \g.

--
Fabien.

Attachments:

psql-test-g-1.patchtext/x-diff; name=psql-test-g-1.patchDownload
diff --git a/src/bin/psql/t/001_basic.pl b/src/bin/psql/t/001_basic.pl
index f447845717..c81feadd4e 100644
--- a/src/bin/psql/t/001_basic.pl
+++ b/src/bin/psql/t/001_basic.pl
@@ -325,4 +325,22 @@ is($row_count, '10',
 	'client-side error commits transaction, no ON_ERROR_STOP and multiple -c switches'
 );
 
+# test \g
+psql_like($node, "SELECT 'un' \\g g_file_1.out", qr//, "one command \\g");
+my $c1 = slurp_file("g_file_1.out");
+like($c1, qr/un/);
+unlink "g_file_1.out" or die $!;
+
+psql_like($node, "SELECT 'deux' \\; SELECT 'trois' \\g g_file_2.out", qr//, "two commands \\g");
+my $c2 = slurp_file("g_file_2.out");
+like($c2, qr/deux.*trois/s);
+unlink "g_file_2.out" or die $!;
+
+psql_like($node, "\\set SHOW_ALL_RESULTS 0\nSELECT 'quatre' \\; SELECT 'cinq' \\g g_file_3.out", qr//,
+  "two commands \\g with only last result");
+my $c3 = slurp_file("g_file_3.out");
+like($c3, qr/cinq/);
+unlike($c3, qr/quatre/);
+unlink "g_file_3.out" or die $!;
+
 done_testing();
#9Daniel Verite
daniel@manitou-mail.org
In reply to: Corey Huinker (#7)
Re: [patch] \g with multiple result sets and \watch with copy queries

Corey Huinker wrote:

I think that because it is more complicated than usual psql, we may want to
comment on the intention of the tests and some of the less-than-common psql
elements (\set concatenation, resetting \o, etc). If you see value in that
I can amend the patch.

If the intentions of some tests appear to be unclear, then yes, sure.
I don't feel the need to explain the "how" though. The other
comments in these files say why we're testing such or such case, but
don't go beyond that.

Are there any options on COPY (header, formats) that we think we should
test as well?

There are COPY tests already in src/test/regress/sql/copy*.sql, which
hopefully cover the many combination of options.

For \g and \o the intention behind the tests is to check that the
query output goes where it should in all cases. The options that can't
affect where the results go are not really in scope.

FTR I started a followup thread on this at [1]/messages/by-id/25c2bb5b-9012-40f8-8088-774cb764046d@manitou-mail.org, to be associated to a
new CF entry [2]https://commitfest.postgresql.org/40/4000/

[1]: /messages/by-id/25c2bb5b-9012-40f8-8088-774cb764046d@manitou-mail.org
/messages/by-id/25c2bb5b-9012-40f8-8088-774cb764046d@manitou-mail.org

[2]: https://commitfest.postgresql.org/40/4000/

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite