cli,sql: do not exit interactive shells with Ctrl+C

This change is a preface to making the interactive shell support Ctrl+C to cancel a currently executing query. When that happens, we need to ensure that Ctrl+C *only* cancels the current query, and not the entire shell. This is because there is no deterministic boundary in time for a user to decide whether the key press will cancel only the query, or stop the shell. If they care about keeping their shell alive, they would never "dare" to press Ctrl+C while a query is executing. Incidentally, this is also what `psql` does, for pretty much the same reason. Note that the new behavior is restricted to *interactive* shells, when stdin is a terminal. The behavior for non-interactive shells remains unchanged, where Ctrl+C will terminate everything. This is also what other SQL shells do. Release note (cli change): `cockroach sql` (and `demo`) now continue to accept user input when Ctrl+C is pressed at the interactive prompt and the current input line is empty. Previously, it would terminate the shell. To terminate the shell, the client-side command `\q` is still supported. The user can also terminate the input altogether via EOF (Ctrl+D). The behavior in non-interactive use remains unchanged.
cockroachdb · Feb 11, 2022 · 7064116 · 7064116
1 parent aa30839
commit 7064116
Show file tree

Hide file tree

Showing 24 changed files with 73 additions and 51 deletions.
diff --git a/pkg/cli/clisqlshell/sql.go b/pkg/cli/clisqlshell/sql.go
@@ -1021,9 +1021,11 @@ func (c *cliState) doReadLine(nextState cliStateEnum) cliStateEnum {
 			return cliStartLine
 		}
 
-		// Otherwise, also terminate with an interrupt error.
-		c.exitErr = err
-		return cliStop
+		// If a human is looking, tell them that quitting is done in another way.
+		if c.sqlExecCtx.TerminalOutput {
+			fmt.Fprintf(c.iCtx.stdout, "^C\nUse \\q or terminate input to exit.\n")
+		}
+		return cliStartLine
 
 	case errors.Is(err, io.EOF):
 		c.atEOF = true

diff --git a/pkg/cli/interactive_tests/test_audit_log.tcl b/pkg/cli/interactive_tests/test_audit_log.tcl
@@ -58,7 +58,7 @@ eexpect root@
 system "grep -q 'sensitive_table_access.*ALTER TABLE.*helloworld.*SET OFF.*AccessMode\":\"rw\"' $logfile"
 end_test
 
-interrupt
+send_eof
 eexpect eof
 
 stop_server $argv
@@ -81,7 +81,7 @@ eexpect "ALTER TABLE"
 eexpect root@
 send "select x from d.helloworld;\r"
 eexpect root@
-interrupt
+send_eof
 eexpect eof
 
 # Check the file was created and populated properly.

diff --git a/pkg/cli/interactive_tests/test_client_side_checking.tcl b/pkg/cli/interactive_tests/test_client_side_checking.tcl
@@ -52,7 +52,7 @@ send "commit;\r"
 eexpect "ROLLBACK"
 eexpect root@
 
-interrupt
+send_eof
 eexpect eof
 end_test
 

diff --git a/pkg/cli/interactive_tests/test_contextual_help.tcl b/pkg/cli/interactive_tests/test_contextual_help.tcl
@@ -128,7 +128,7 @@ eexpect root@
 end_test
 
 # Finally terminate with Ctrl+C.
-interrupt
+send_eof
 eexpect eof
 
 start_test "Check that the hint for a single ? is also printed in non-interactive sessions."

diff --git a/pkg/cli/interactive_tests/test_demo.tcl b/pkg/cli/interactive_tests/test_demo.tcl
@@ -35,7 +35,7 @@ eexpect "brief introduction"
 eexpect root@
 # Ensure db is movr.
 eexpect "movr>"
-interrupt
+send_eof
 eexpect eof
 end_test
 
@@ -68,7 +68,7 @@ eexpect ":26257"
 eexpect "sslmode=disable"
 eexpect "defaultdb>"
 
-interrupt
+send_eof
 eexpect eof
 
 # With command-line override.
@@ -88,7 +88,7 @@ eexpect "(sql/unix)"
 eexpect "root:unused@"
 eexpect "defaultdb>"
 
-interrupt
+send_eof
 eexpect eof
 
 end_test
@@ -125,7 +125,7 @@ eexpect ":26257"
 eexpect "sslmode=require"
 eexpect "defaultdb>"
 
-interrupt
+send_eof
 eexpect eof
 
 # With command-line override.
@@ -146,7 +146,7 @@ eexpect "(sql/unix)"
 eexpect "demo:"
 eexpect "defaultdb>"
 
-interrupt
+send_eof
 eexpect eof
 
 end_test
@@ -213,7 +213,7 @@ eexpect "http://"
 eexpect ":8005"
 eexpect "defaultdb>"
 
-interrupt
+send_eof
 eexpect eof
 
 spawn $argv demo --no-example-database --nodes 3 --sql-port 23000
@@ -249,7 +249,7 @@ eexpect "(sql)"
 eexpect ":23002"
 eexpect "defaultdb>"
 
-interrupt
+send_eof
 eexpect eof
 
 
@@ -264,7 +264,7 @@ eexpect "defaultdb>"
 # Check the URL is valid. If the connection fails, the system command will fail too.
 system "$argv sql --url `cat test.url` -e 'select 1'"
 
-interrupt
+send_eof
 eexpect eof
 
 # Ditto, insecure
@@ -275,7 +275,7 @@ eexpect "defaultdb>"
 # Check the URL is valid. If the connection fails, the system command will fail too.
 system "$argv sql --url `cat test.url` -e 'select 1'"
 
-interrupt
+send_eof
 eexpect eof
 
 

diff --git a/pkg/cli/interactive_tests/test_demo_global.tcl b/pkg/cli/interactive_tests/test_demo_global.tcl
@@ -28,6 +28,6 @@ send "\\demo shutdown 3\r"
 eexpect "shutting down nodes is not supported in --global configurations"
 eexpect "defaultdb>"
 
-interrupt
+send_eof
 eexpect eof
 end_test
diff --git a/pkg/cli/interactive_tests/test_demo_multitenant.tcl b/pkg/cli/interactive_tests/test_demo_multitenant.tcl
@@ -25,6 +25,6 @@ send "SELECT gateway_region();\n"
 eexpect "us-east1"
 eexpect "defaultdb>"
 
-interrupt
+send_eof
 eexpect eof
 end_test
diff --git a/pkg/cli/interactive_tests/test_demo_node_cmds.tcl b/pkg/cli/interactive_tests/test_demo_node_cmds.tcl
@@ -13,10 +13,12 @@ eexpect "movr>"
 # Wrong number of args
 send "\\demo node\r"
 eexpect "invalid syntax: \\\\demo node. Try \\\\? for help."
+eexpect "movr>"
 
 # Cannot shutdown node 1
 send "\\demo shutdown 1\r"
 eexpect "cannot shutdown node 1"
+eexpect "movr>"
 
 # Cannot operate on a node which does not exist.
 send "\\demo shutdown 8\r"
@@ -27,25 +29,30 @@ send "\\demo decommission 8\r"
 eexpect "node 8 does not exist"
 send "\\demo recommission 8\r"
 eexpect "node 8 does not exist"
+eexpect "movr>"
 
 # Cannot restart a node that is not shut down.
 send "\\demo restart 2\r"
 eexpect "node 2 is already running"
+eexpect "movr>"
 
 # Shut down a separate node.
 send "\\demo shutdown 3\r"
 eexpect "node 3 has been shutdown"
+eexpect "movr>"
 
 send "select node_id, draining, decommissioning, membership from crdb_internal.gossip_liveness ORDER BY node_id;\r"
 eexpect "1 |  false   |      false      | active"
 eexpect "2 |  false   |      false      | active"
 eexpect "3 |   true   |      false      | active"
 eexpect "4 |  false   |      false      | active"
 eexpect "5 |  false   |      false      | active"
+eexpect "movr>"
 
 # Cannot shut it down again.
 send "\\demo shutdown 3\r"
 eexpect "node 3 is already shut down"
+eexpect "movr>"
 
 # Expect queries to still work with just one node down.
 send "SELECT count(*) FROM movr.rides;\r"
@@ -55,38 +62,46 @@ eexpect "movr>"
 # Now restart the node.
 send "\\demo restart 3\r"
 eexpect "node 3 has been restarted"
+eexpect "movr>"
 
 send "select node_id, draining, decommissioning, membership from crdb_internal.gossip_liveness ORDER BY node_id;\r"
 eexpect "1 |  false   |      false      | active"
 eexpect "2 |  false   |      false      | active"
 eexpect "3 |  false   |      false      | active"
 eexpect "4 |  false   |      false      | active"
 eexpect "5 |  false   |      false      | active"
+eexpect "movr>"
 
 # Try commissioning commands
 send "\\demo decommission 4\r"
 eexpect "node 4 has been decommissioned"
+eexpect "movr>"
 
 send "select node_id, draining, decommissioning, membership from crdb_internal.gossip_liveness ORDER BY node_id;\r"
 eexpect "1 |  false   |      false      | active"
 eexpect "2 |  false   |      false      | active"
 eexpect "3 |  false   |      false      | active"
 eexpect "4 |  false   |      true       | decommissioned"
 eexpect "5 |  false   |      false      | active"
+eexpect "movr>"
 
 send "\\demo recommission 4\r"
 eexpect "can only recommission a decommissioning node"
+eexpect "movr>"
 
 send "\\demo add blah\r"
 eexpect "internal server error: tier must be in the form \"key=value\" not \"blah\""
+eexpect "movr>"
 
 send "\\demo add region=ca-central,zone=a\r"
 eexpect "node 6 has been added with locality \"region=ca-central,zone=a\""
+eexpect "movr>"
 
 send "show regions from cluster;\r"
 eexpect "ca-central | \{a\}"
 eexpect "us-east1   | \{b,c,d\}"
 eexpect "us-west1   | \{b\}"
+eexpect "movr>"
 
 # We use kv_node_status here because gossip_liveness is timing dependant.
 # Node 4's status entry should have been removed by now.
@@ -96,10 +111,12 @@ eexpect "2 | region=us-east1,az=c"
 eexpect "3 | region=us-east1,az=d"
 eexpect "5 | region=us-west1,az=b"
 eexpect "6 | region=ca-central,zone=a"
+eexpect "movr>"
 
 # Shut down the newly created node.
 send "\\demo shutdown 6\r"
 eexpect "node 6 has been shutdown"
+eexpect "movr>"
 
 # By now the node should have stabilized in gossip which allows us to query the more detailed information there.
 send "select node_id, draining, decommissioning, membership from crdb_internal.gossip_liveness ORDER BY node_id;\r"
@@ -109,7 +126,8 @@ eexpect "3 |  false   |      false      | active"
 eexpect "4 |  false   |      true       | decommissioned"
 eexpect "5 |  false   |      false      | active"
 eexpect "6 |   true   |      false      | active"
+eexpect "movr>"
 
-interrupt
+send_eof
 eexpect eof
 end_test
diff --git a/pkg/cli/interactive_tests/test_demo_partitioning.tcl b/pkg/cli/interactive_tests/test_demo_partitioning.tcl
@@ -95,7 +95,7 @@ eexpect "8"
 eexpect "(1 row)"
 eexpect "movr>"
 
-interrupt
+send_eof
 eexpect $prompt
 end_test
 
@@ -119,7 +119,7 @@ eexpect "  survival_goal"
 eexpect "zone"
 eexpect "movr>"
 
-interrupt
+send_eof
 eexpect $prompt
 
 send "$argv demo movr --geo-partitioned-replicas --multi-region --survive=region\r"
@@ -131,7 +131,7 @@ eexpect "  survival_goal"
 eexpect "region"
 eexpect "movr>"
 
-interrupt
+send_eof
 eexpect $prompt
 
 end_test

diff --git a/pkg/cli/interactive_tests/test_demo_telemetry.tcl b/pkg/cli/interactive_tests/test_demo_telemetry.tcl
@@ -18,7 +18,7 @@ send "alter table vehicles partition by list (city) (partition p1 values in ('ny
 # expect that it failed, as no license was requested.
 eexpect "use of partitions requires an enterprise license"
 # clean up after the test
-interrupt
+send_eof
 eexpect eof
 
 end_test
diff --git a/pkg/cli/interactive_tests/test_demo_workload.tcl b/pkg/cli/interactive_tests/test_demo_workload.tcl
@@ -30,8 +30,9 @@ if {!$workloadRunning} {
   report "Workload is not running"
   exit 1
 }
+eexpect "movr>"
 
-interrupt
+send_eof
 eexpect eof
 end_test
 
@@ -48,7 +49,8 @@ eexpect "movr>"
 send "SELECT count(*) FROM \[SHOW RANGES FROM TABLE USERS\];\r"
 eexpect "6"
 eexpect "(1 row)"
+eexpect "movr>"
 
-interrupt
+send_eof
 eexpect eof
 end_test
diff --git a/pkg/cli/interactive_tests/test_dump_sig.tcl b/pkg/cli/interactive_tests/test_dump_sig.tcl
@@ -37,7 +37,7 @@ send "\r"
 eexpect root@
 
 # Finish the test.
-interrupt
+send_eof
 eexpect ":/# "
 end_test
 

diff --git a/pkg/cli/interactive_tests/test_explain_analyze_debug.tcl b/pkg/cli/interactive_tests/test_explain_analyze_debug.tcl
@@ -42,7 +42,7 @@ eexpect "Bundle saved to"
 
 file_exists "stmt-bundle-$id.zip"
 
-interrupt
+send_eof
 eexpect eof
 
 end_test
@@ -81,7 +81,7 @@ eexpect "Bundle saved to"
 
 file_exists "stmt-bundle-$id.zip"
 
-interrupt
+send_eof
 eexpect eof
 
 stop_tenant 5 $argv

diff --git a/pkg/cli/interactive_tests/test_flags.tcl b/pkg/cli/interactive_tests/test_flags.tcl
@@ -89,7 +89,7 @@ eexpect "cli.demo.explicitflags.logtostderr"
 eexpect "cli.demo.explicitflags.no-example-database"
 eexpect "cli.demo.runs"
 eexpect "defaultdb>"
-interrupt
+send_eof
 eexpect ":/# "
 end_test
 
@@ -112,7 +112,7 @@ eexpect "cli.start-single-node.explicitflags.listening-url-file"
 eexpect "cli.start-single-node.explicitflags.max-sql-memory"
 eexpect "cli.start-single-node.runs"
 eexpect "defaultdb>"
-interrupt
+send_eof
 eexpect ":/# "
 end_test
 
@@ -121,7 +121,7 @@ send "export COCKROACH_URL=`cat server_url`;\r"
 eexpect ":/# "
 send "$argv sql\r"
 eexpect "defaultdb>"
-interrupt
+send_eof
 eexpect ":/# "
 end_test
 

diff --git a/pkg/cli/interactive_tests/test_history.tcl b/pkg/cli/interactive_tests/test_history.tcl
@@ -81,8 +81,8 @@ eexpect "1 row"
 eexpect root@
 end_test
 
-# Finally terminate with Ctrl+C
-interrupt
+# Finally terminate with Ctrl+D
+send_eof
 eexpect eof
 
 stop_server $argv
diff --git a/pkg/cli/interactive_tests/test_init_command.tcl b/pkg/cli/interactive_tests/test_init_command.tcl
@@ -61,7 +61,7 @@ expect {
 }
 
 interrupt
-interrupt
+send_eof
 eexpect eof
 
 end_test