psql: Forbid use of COPY and \copy while in a pipeline

Running COPY within a pipeline can break protocol synchronization in
multiple ways.  psql is limited in terms of result processing if mixing
COPY commands with normal queries while controlling a pipeline with the
new meta-commands, as an effect of the following reasons:
- In COPY mode, the backend ignores additional Sync messages and will
not send a matching ReadyForQuery expected by the frontend.  Doing a
\syncpipeline just after COPY will leave the frontend waiting for a
ReadyForQuery message that won't be sent, leaving psql out-of-sync.
- libpq automatically sends a Sync with the Copy message which is not
tracked in the command queue, creating an unexpected synchronisation
point that psql cannot really know about.  While it is possible to track
such activity for a \copy, this cannot really be done sanely with plain
COPY queries.  Backend failures during a COPY would leave the pipeline
in an aborted state while the backend would be in a clean state, ready
to process commands.

At the end, fixing those issues would require modifications in how libpq
handles pipeline and COPY.  So, rather than implementing workarounds in
psql to shortcut the libpq internals (with command queue handling for
one), and because meta-commands for pipelines in psql are a new feature
with COPY in a pipeline having a limited impact compared to other
queries, this commit forbids the use of COPY within a pipeline to avoid
possible break of protocol synchronisation within psql.  If there is a
use-case for COPY support within pipelines in libpq, this could always
be added in the future, if necessary.

Most of the changes of this commit impacts the tests for psql pipelines,
removing the tests related to COPY.  Some TAP tests still exist for COPY
TO/FROM and \copy to/from, to check that that connections are aborted
when this operation is attempted.

Reported-by: Nikita Kalinin <n.kalinin@postgrespro.ru>
Author: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>
Discussion: https://postgr.es/m/AC468509-06E8-4E2A-A4B1-63046A4AC6AB@postgrespro.ru

5 files changed, 43 insertions, 325 deletions

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 8f7d8758ca0..1cb67ff806d 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3734,6 +3734,10 @@ testdb=&gt; <userinput>\setenv LESS -imx4F</userinput>
        </para>
 
        <para>
+        <command>COPY</command> is not supported while in pipeline mode.
+       </para>
+
+       <para>
         Example:
 <programlisting>
 \startpipeline
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 47352b7faed..b53cd8ab698 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1867,18 +1867,30 @@ ExecQueryAndProcessResults(const char *query,
 		{
 			FILE	   *copy_stream = NULL;
 
-			if (pset.piped_syncs > 1)
+			if (PQpipelineStatus(pset.db) != PQ_PIPELINE_OFF)
 			{
 				/*
-				 * When reading COPY data, the backend ignores sync messages
-				 * and will not send a matching ReadyForQuery response.  Even
-				 * if we adjust piped_syncs and requested_results, it is not
-				 * possible to salvage this as the sync message would still be
-				 * in libpq's command queue and we would be stuck in a busy
-				 * pipeline state.  Thus, we abort the connection to avoid
-				 * this state.
+				 * Running COPY within a pipeline can break the protocol
+				 * synchronisation in multiple ways, and psql shows its limits
+				 * when it comes to tracking this information.
+				 *
+				 * While in COPY mode, the backend process ignores additional
+				 * Sync messages and will not send the matching ReadyForQuery
+				 * expected by the frontend.
+				 *
+				 * Additionally, libpq automatically sends a Sync with the
+				 * Copy message, creating an unexpected synchronisation point.
+				 * A failure during COPY would leave the pipeline in an
+				 * aborted state while the backend would be in a clean state,
+				 * ready to process commands.
+				 *
+				 * Improving those issues would require modifications in how
+				 * libpq handles pipelines and COPY.  Hence, for the time
+				 * being, we forbid the use of COPY within a pipeline,
+				 * aborting the connection to avoid an inconsistent state on
+				 * psql side if trying to use a COPY command.
 				 */
-				pg_log_info("\\syncpipeline after COPY is not supported, aborting connection");
+				pg_log_info("COPY in a pipeline is not supported, aborting connection");
 				exit(EXIT_BADCONN);
 			}
 
diff --git a/src/bin/psql/t/001_basic.pl b/src/bin/psql/t/001_basic.pl
index ae5c1d66405..f42c3961e09 100644
--- a/src/bin/psql/t/001_basic.pl
+++ b/src/bin/psql/t/001_basic.pl
@@ -483,8 +483,8 @@ psql_like($node, "copy (values ('foo'),('bar')) to stdout \\g | $pipe_cmd",
 my $c4 = slurp_file($g_file);
 like($c4, qr/foo.*bar/s);
 
-# Tests with pipelines.  These trigger FATAL failures in the backend,
-# so they cannot be tested via SQL.
+# Test COPY within pipelines.  These abort the connection from
+# the frontend so they cannot be tested via SQL.
 $node->safe_psql('postgres', 'CREATE TABLE psql_pipeline()');
 my $log_location = -s $node->logfile;
 psql_fails_like(
@@ -493,53 +493,41 @@ psql_fails_like(
 COPY psql_pipeline FROM STDIN;
 SELECT 'val1';
 \\syncpipeline
-\\getresults
 \\endpipeline},
-	qr/server closed the connection unexpectedly/,
-	'protocol sync loss in pipeline: direct COPY, SELECT, sync and getresult'
-);
+	qr/COPY in a pipeline is not supported, aborting connection/,
+	'COPY FROM in pipeline: fails');
 $node->wait_for_log(
 	qr/FATAL: .*terminating connection because protocol synchronization was lost/,
 	$log_location);
 
+# Remove \syncpipeline here.
 psql_fails_like(
 	$node,
 	qq{\\startpipeline
-COPY psql_pipeline FROM STDIN \\bind \\sendpipeline
-SELECT 'val1' \\bind \\sendpipeline
-\\syncpipeline
-\\getresults
-\\endpipeline},
-	qr/server closed the connection unexpectedly/,
-	'protocol sync loss in pipeline: bind COPY, SELECT, sync and getresult');
-
-# This time, test without the \getresults and \syncpipeline.
-psql_fails_like(
-	$node,
-	qq{\\startpipeline
-COPY psql_pipeline FROM STDIN;
+COPY psql_pipeline TO STDOUT;
 SELECT 'val1';
 \\endpipeline},
-	qr/server closed the connection unexpectedly/,
-	'protocol sync loss in pipeline: COPY, SELECT and sync');
+	qr/COPY in a pipeline is not supported, aborting connection/,
+	'COPY TO in pipeline: fails');
 
-# Tests sending a sync after a COPY TO/FROM.  These abort the connection
-# from the frontend.
 psql_fails_like(
 	$node,
 	qq{\\startpipeline
-COPY psql_pipeline FROM STDIN;
+\\copy psql_pipeline from stdin;
+SELECT 'val1';
 \\syncpipeline
 \\endpipeline},
-	qr/\\syncpipeline after COPY is not supported, aborting connection/,
-	'sending sync after COPY FROM');
+	qr/COPY in a pipeline is not supported, aborting connection/,
+	'\copy from in pipeline: fails');
+
+# Sync attempt after a COPY TO/FROM.
 psql_fails_like(
 	$node,
 	qq{\\startpipeline
-COPY psql_pipeline TO STDOUT;
+\\copy psql_pipeline to stdout;
 \\syncpipeline
 \\endpipeline},
-	qr/\\syncpipeline after COPY is not supported, aborting connection/,
-	'sending sync after COPY TO');
+	qr/COPY in a pipeline is not supported, aborting connection/,
+	'\copy to in pipeline: fails');
 
 done_testing();
diff --git a/src/test/regress/expected/psql_pipeline.out b/src/test/regress/expected/psql_pipeline.out
index a30dec088b9..e78e6bfa0ad 100644
--- a/src/test/regress/expected/psql_pipeline.out
+++ b/src/test/regress/expected/psql_pipeline.out
@@ -228,192 +228,6 @@ BEGIN \bind \sendpipeline
 INSERT INTO psql_pipeline VALUES ($1) \bind 1 \sendpipeline
 COMMIT \bind \sendpipeline
 \endpipeline
--- COPY FROM STDIN
--- with \sendpipeline and \bind
-\startpipeline
-SELECT $1 \bind 'val1' \sendpipeline
-COPY psql_pipeline FROM STDIN \bind \sendpipeline
-\endpipeline
- ?column? 
-----------
- val1
-(1 row)
-
--- with semicolon
-\startpipeline
-SELECT 'val1';
-COPY psql_pipeline FROM STDIN;
-\endpipeline
- ?column? 
-----------
- val1
-(1 row)
-
--- COPY FROM STDIN with \flushrequest + \getresults
--- with \sendpipeline and \bind
-\startpipeline
-SELECT $1 \bind 'val1' \sendpipeline
-COPY psql_pipeline FROM STDIN \bind \sendpipeline
-\flushrequest
-\getresults
- ?column? 
-----------
- val1
-(1 row)
-
-message type 0x5a arrived from server while idle
-\endpipeline
--- with semicolon
-\startpipeline
-SELECT 'val1';
-COPY psql_pipeline FROM STDIN;
-\flushrequest
-\getresults
- ?column? 
-----------
- val1
-(1 row)
-
-message type 0x5a arrived from server while idle
-\endpipeline
--- COPY FROM STDIN with \syncpipeline + \getresults
--- with \bind and \sendpipeline
-\startpipeline
-SELECT $1 \bind 'val1' \sendpipeline
-COPY psql_pipeline FROM STDIN \bind \sendpipeline
-\syncpipeline
-\getresults
- ?column? 
-----------
- val1
-(1 row)
-
-\endpipeline
--- with semicolon
-\startpipeline
-SELECT 'val1';
-COPY psql_pipeline FROM STDIN;
-\syncpipeline
-\getresults
- ?column? 
-----------
- val1
-(1 row)
-
-\endpipeline
--- COPY TO STDOUT
--- with \bind and \sendpipeline
-\startpipeline
-SELECT $1 \bind 'val1' \sendpipeline
-copy psql_pipeline TO STDOUT \bind \sendpipeline
-\endpipeline
- ?column? 
-----------
- val1
-(1 row)
-
-1	\N
-2	test2
-20	test2
-3	test3
-30	test3
-4	test4
-40	test4
--- with semicolon
-\startpipeline
-SELECT 'val1';
-copy psql_pipeline TO STDOUT;
-\endpipeline
- ?column? 
-----------
- val1
-(1 row)
-
-1	\N
-2	test2
-20	test2
-3	test3
-30	test3
-4	test4
-40	test4
--- COPY TO STDOUT with \flushrequest + \getresults
--- with \bind and \sendpipeline
-\startpipeline
-SELECT $1 \bind 'val1' \sendpipeline
-copy psql_pipeline TO STDOUT \bind \sendpipeline
-\flushrequest
-\getresults
- ?column? 
-----------
- val1
-(1 row)
-
-1	\N
-2	test2
-20	test2
-3	test3
-30	test3
-4	test4
-40	test4
-\endpipeline
--- with semicolon
-\startpipeline
-SELECT 'val1';
-copy psql_pipeline TO STDOUT;
-\flushrequest
-\getresults
- ?column? 
-----------
- val1
-(1 row)
-
-1	\N
-2	test2
-20	test2
-3	test3
-30	test3
-4	test4
-40	test4
-\endpipeline
--- COPY TO STDOUT with \syncpipeline + \getresults
--- with \bind and \sendpipeline
-\startpipeline
-SELECT $1 \bind 'val1' \sendpipeline
-copy psql_pipeline TO STDOUT \bind \sendpipeline
-\syncpipeline
-\getresults
- ?column? 
-----------
- val1
-(1 row)
-
-1	\N
-2	test2
-20	test2
-3	test3
-30	test3
-4	test4
-40	test4
-\endpipeline
--- with semicolon
-\startpipeline
-SELECT 'val1';
-copy psql_pipeline TO STDOUT;
-\syncpipeline
-\getresults
- ?column? 
-----------
- val1
-(1 row)
-
-1	\N
-2	test2
-20	test2
-3	test3
-30	test3
-4	test4
-40	test4
-\endpipeline
 -- Use \parse and \bind_named
 \startpipeline
 SELECT $1 \parse ''
@@ -740,7 +554,7 @@ SELECT COUNT(*) FROM psql_pipeline \bind \sendpipeline
 
  count 
 -------
-     7
+     1
 (1 row)
 
 -- After an error, pipeline is aborted and requires \syncpipeline to be
diff --git a/src/test/regress/sql/psql_pipeline.sql b/src/test/regress/sql/psql_pipeline.sql
index 16e1e1e84cd..5945eca1ef7 100644
--- a/src/test/regress/sql/psql_pipeline.sql
+++ b/src/test/regress/sql/psql_pipeline.sql
@@ -105,106 +105,6 @@ INSERT INTO psql_pipeline VALUES ($1) \bind 1 \sendpipeline
 COMMIT \bind \sendpipeline
 \endpipeline
 
--- COPY FROM STDIN
--- with \sendpipeline and \bind
-\startpipeline
-SELECT $1 \bind 'val1' \sendpipeline
-COPY psql_pipeline FROM STDIN \bind \sendpipeline
-\endpipeline
-2	test2
-\.
--- with semicolon
-\startpipeline
-SELECT 'val1';
-COPY psql_pipeline FROM STDIN;
-\endpipeline
-20	test2
-\.
-
--- COPY FROM STDIN with \flushrequest + \getresults
--- with \sendpipeline and \bind
-\startpipeline
-SELECT $1 \bind 'val1' \sendpipeline
-COPY psql_pipeline FROM STDIN \bind \sendpipeline
-\flushrequest
-\getresults
-3	test3
-\.
-\endpipeline
--- with semicolon
-\startpipeline
-SELECT 'val1';
-COPY psql_pipeline FROM STDIN;
-\flushrequest
-\getresults
-30	test3
-\.
-\endpipeline
-
--- COPY FROM STDIN with \syncpipeline + \getresults
--- with \bind and \sendpipeline
-\startpipeline
-SELECT $1 \bind 'val1' \sendpipeline
-COPY psql_pipeline FROM STDIN \bind \sendpipeline
-\syncpipeline
-\getresults
-4	test4
-\.
-\endpipeline
--- with semicolon
-\startpipeline
-SELECT 'val1';
-COPY psql_pipeline FROM STDIN;
-\syncpipeline
-\getresults
-40	test4
-\.
-\endpipeline
-
--- COPY TO STDOUT
--- with \bind and \sendpipeline
-\startpipeline
-SELECT $1 \bind 'val1' \sendpipeline
-copy psql_pipeline TO STDOUT \bind \sendpipeline
-\endpipeline
--- with semicolon
-\startpipeline
-SELECT 'val1';
-copy psql_pipeline TO STDOUT;
-\endpipeline
-
--- COPY TO STDOUT with \flushrequest + \getresults
--- with \bind and \sendpipeline
-\startpipeline
-SELECT $1 \bind 'val1' \sendpipeline
-copy psql_pipeline TO STDOUT \bind \sendpipeline
-\flushrequest
-\getresults
-\endpipeline
--- with semicolon
-\startpipeline
-SELECT 'val1';
-copy psql_pipeline TO STDOUT;
-\flushrequest
-\getresults
-\endpipeline
-
--- COPY TO STDOUT with \syncpipeline + \getresults
--- with \bind and \sendpipeline
-\startpipeline
-SELECT $1 \bind 'val1' \sendpipeline
-copy psql_pipeline TO STDOUT \bind \sendpipeline
-\syncpipeline
-\getresults
-\endpipeline
--- with semicolon
-\startpipeline
-SELECT 'val1';
-copy psql_pipeline TO STDOUT;
-\syncpipeline
-\getresults
-\endpipeline
-
 -- Use \parse and \bind_named
 \startpipeline
 SELECT $1 \parse ''