Date: Fri, 03 Jun 2022 17:56:03 +0900 (JST)
Message-Id: <20220603.175603.682874096867945240.horikyota.ntt@gmail.com>
To: michael@paquier.xyz
Cc: christoph.berg@credativ.de, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17504: psql --single-transaction -vON_ERROR_STOP=1 still
 commits after client-side error
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <YphNHbOkg22H7d52@paquier.xyz>
References: <YpTgRGknCLBopP8C@msg.df7cb.de>
	<20220531.113927.640026319524742851.horikyota.ntt@gmail.com>
	<YphNHbOkg22H7d52@paquier.xyz>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Multipart/Mixed;
 boundary="--Next_Part(Fri_Jun__3_17_56_03_2022_900)--"
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20220603.175603.682874096867945240.horikyota.ntt%40gmail.com>
Precedence: bulk

----Next_Part(Fri_Jun__3_17_56_03_2022_900)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Thanks for the suggestions!

At Thu, 2 Jun 2022 14:39:41 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> The change that you are sending would enforce this policy as Christoph
> would like.  Some tests would be nice to check such behaviors, say in

Added a test.

> 001_basic.pl, but we also need to be careful when sending down queries
> with psql expected to fail because of SIGPIPE (c757a3d, 6d41dd0).  The

This does not need to send a byte after the client-side failure. The
connection is rather living after failure in this case.

> docs need a refresh, they mention now that COMMIT is sent after the
> last command but that would not be the case anymore with this patch if
> there is a client-side error.

Agreed. Does the following work?

 > It causes psql to issue a BEGIN command before the first such option
 > and a COMMIT command after the last one, thereby wrapping all the
 > commands into a single transaction.
+> If any of the commands fails, a ROLLBACK command is sent instead.
 > This ensures that either all the commands complete successfully, or
 > no changes are applied.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

----Next_Part(Fri_Jun__3_17_56_03_2022_900)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="do_not_commit_after_client-side_failure.patch"

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 47bf3342a5..6f5476d99e 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -584,8 +584,10 @@ EOF
         <application>psql</application> to issue a <command>BEGIN</command> command
         before the first such option and a <command>COMMIT</command> command after
         the last one, thereby wrapping all the commands into a single
-        transaction.  This ensures that either all the commands complete
-        successfully, or no changes are applied.
+        transaction.  If any of the commands fails,
+        a <command>ROLLBACK</command> command is sent instead. This ensures
+        that either all the commands complete successfully, or no changes are
+        applied.
        </para>
 
        <para>
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index ddff903915..2261f78f81 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -426,7 +426,9 @@ main(int argc, char *argv[])
 
 		if (options.single_txn)
 		{
-			if ((res = PSQLexec("COMMIT")) == NULL)
+			res = PSQLexec(successResult == EXIT_SUCCESS ?
+						   "COMMIT" : "ROLLBACK");
+			if (res == NULL)
 			{
 				if (pset.on_error_stop)
 				{
diff --git a/src/bin/psql/t/001_basic.pl b/src/bin/psql/t/001_basic.pl
index c3ed18e84d..0cae05ca4b 100644
--- a/src/bin/psql/t/001_basic.pl
+++ b/src/bin/psql/t/001_basic.pl
@@ -198,4 +198,14 @@ like(
 ^LOCATION: .*$/m,
 	'\errverbose after \gdesc with error');
 
+# Check if client-side error causes the whole transaction rolled-back
+$node->safe_psql('postgres', "CREATE TABLE t AS SELECT 1 AS a");
+
+$node->command_fails(['psql', '--single-transaction', '-v', 'ON_ERROR_STOP=1',
+					  '-c', 'DELETE FROM t',
+					  '-c', "\\copy t FROM '/nonexistent'"]); 
+
+is($node->safe_psql('postgres', 'SELECT count(*) FROM t'), 1,
+   'Non-server error when ON_ERROR_STOP rolls back the transaction');
+
 done_testing();

----Next_Part(Fri_Jun__3_17_56_03_2022_900)----