cli: additional enhancements after switching to jackc/pgx #82101

rafiss · 2022-05-31T02:44:56Z

Fixes #72065

Release note (cli change): Using COPY in the CLI is now
supported while inside an explicit transaction.

Release note (bug fix): The DateStyle session setting is no longer
ignored when using the CLI when set in the options URL parameter.

Release note (cli change): Ctrl+C (the interrupt signal) can now be used
in the CLI to attempt to cancel the currently executing SQL query.

cockroach-teamcity · 2022-05-31T02:45:05Z

This change is

knz · 2022-06-01T12:50:32Z

i'd like to delay the review of this until the other PR has merged, because if there's a chance for a rebase that will make reviewable unhappy.

or alternatively, you can create a PR using the other PR as base branch, to facilitate the review. (The way I did for e.g. #82069.)

rafiss · 2022-06-01T16:58:46Z

or alternatively, you can create a PR using the other PR as base branch, to facilitate the review. (The way I did for e.g. #82069.)

That's what I intended to do here. Anyway, the other PR should get merged soon, so this will look better soon.

otan

what a nice library pgx is :')

otan · 2022-06-01T21:05:10Z

pkg/cli/clisqlshell/sql.go

@@ -1857,13 +1856,6 @@ func (c *cliState) doRunStatements(nextState cliStateEnum) cliStateEnum {
 }

 func (c *cliState) beginCopyFrom(ctx context.Context, sql string) error {
-	c.refreshTransactionStatus()


nit for this commit: should read

Release note (cli change): Using COPY in a CLI can now be used while inside a transaction.

(normal COPY should still work)

otan · 2022-06-01T22:51:54Z

pkg/cli/clisqlshell/sql.go

-					return
-				case <-intCh:
+	go func() {
+		for {


was there a test for this functionality somewhere?

ah there is - can you rename test_interrupt.tcl.disabled back to test_interrupt.tcl?

pkg/cli/clisqlclient/rows.go

pgx no longer requires us to have this limitation. Release note (cli change): Using COPY in the CLI is now supported while inside an explicit transaction.

knz

modulo missing release note.

also the interrupt test has an issue in CI? need to investigate further maybe.

Reviewed 1 of 3 files at r2, 1 of 59 files at r9, 19 of 19 files at r16, 2 of 2 files at r17, 1 of 1 files at r18, 6 of 6 files at r19, 13 of 13 files at r20, 3 of 3 files at r21, 7 of 7 files at r22, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz, @otan, and @rafiss)

-- commits line 12 at r18:
Maybe add a release note to explain that query cancellation is enabled in the CLI.

rafiss · 2022-06-02T21:15:23Z

also the interrupt test has an issue in CI? need to investigate further maybe.

🤦 looks like pgx has the same default behavior where cancelling the context actually closes the connection. it's defined by the context watcher here: https://github.com/jackc/pgconn/blob/7ddbd74d5e5a52cd24f750cacfc9be91d4f49f76/pgconn.go#L360

rafiss · 2022-06-02T21:36:47Z

I've updated things so it explicitly sends a cancel request instead of relying on context cancellation. i know this wasn't your first preference, so let me know what you think

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz, @otan, and @rafiss)

pkg/cli/clisqlshell/sql.go line 2238 at r28 (raw file):

						break wait
					case <-tooLongTimer:
						fmt.Fprintln(c.iCtx.stderr, "server does not respond to query cancellation; a second interrupt will stop the shell.")

an alternative approach is to call the cancel() function (the one from line 2204 above) here. this would cause the connection to be closed, rather than exiting the shell.

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz, @otan, and @rafiss)

pkg/cli/clisqlshell/sql.go line 2238 at r28 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

an alternative approach is to call the cancel() function (the one from line 2204 above) here. this would cause the connection to be closed, rather than exiting the shell.

oh nvm, that is not correct. we would have to call the cancel() function that's created in runWithInterruptableCtx

rafiss · 2022-06-04T16:02:37Z

@knz ping in case you had any thoughts on this:

I've updated things so it explicitly sends a cancel request instead of relying on context cancellation. i know this wasn't your first preference, so let me know what you think

knz

This looks good with a tweak.

Reviewed 1 of 20 files at r23, 1 of 1 files at r24, 2 of 6 files at r25, 7 of 13 files at r26, 2 of 3 files at r27, 7 of 7 files at r28, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz, @otan, and @rafiss)

pkg/cli/clisqlshell/context.go line 79 at r28 (raw file):

	mu struct {
		syncutil.Mutex
		cancelFn func() error

can you make that take a context.Context argument

pkg/cli/clisqlshell/sql.go line 2221 at r28 (raw file):

				// Cancel the query's context, which should make the driver
				// send a cancellation message.
				if err := cancelFn(); err != nil {

then pass ctx as argument here

pkg/cli/clisqlshell/sql.go line 2268 at r28 (raw file):

	// Inform the Ctrl+C handler that this query is executing.
	c.iCtx.mu.Lock()
	c.iCtx.mu.cancelFn = func() error {

then simplify this .cancel = c.conn.Cancel

Release note (cli change): Ctrl+C (the interrupt signal) can now be used in the CLI to attempt to cancel the currently executing SQL query.

The upgrade to pgx has fixed this. Release note (bug fix): The DateStyle session setting is no longer ignored when using the CLI when set in the `options` URL parameter.

Release note: None

rafiss

tftr! i've added your suggestion

bors r=knz,otan

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz, @otan, and @rafiss)

craig · 2022-06-06T22:09:41Z

Build succeeded:

GitHub CI (Cockroach)

rafiss force-pushed the cli-pgx-combined branch from f16d82c to e487737 Compare May 31, 2022 19:00

rafiss changed the title ~~cli: switch to jackc/pgx~~ cli: additional enhancements after switching to jackc/pgx May 31, 2022

This was referenced May 31, 2022

cli: switch to jackc/pgx #81943

Merged

cli: use pgx and related libraries #81007

Closed

rafiss force-pushed the cli-pgx-combined branch 2 times, most recently from ee5e38b to 25302bf Compare May 31, 2022 22:52

rafiss force-pushed the cli-pgx-combined branch from 25302bf to f9b3e33 Compare June 1, 2022 20:53

rafiss marked this pull request as ready for review June 1, 2022 20:54

rafiss requested review from a team as code owners June 1, 2022 20:54

rafiss requested review from knz, otan and a team and removed request for a team June 1, 2022 20:54

rafiss force-pushed the cli-pgx-combined branch 3 times, most recently from 43f1fd9 to b25839a Compare June 1, 2022 22:14

otan approved these changes Jun 1, 2022

View reviewed changes

cli: allow COPY inside a transaction

d632202

pgx no longer requires us to have this limitation. Release note (cli change): Using COPY in the CLI is now supported while inside an explicit transaction.

rafiss force-pushed the cli-pgx-combined branch from b25839a to 6b3d2a8 Compare June 2, 2022 02:34

otan approved these changes Jun 2, 2022

View reviewed changes

knz approved these changes Jun 2, 2022

View reviewed changes

rafiss force-pushed the cli-pgx-combined branch from 6b3d2a8 to ab7c311 Compare June 2, 2022 21:36

rafiss commented Jun 2, 2022

View reviewed changes

knz approved these changes Jun 6, 2022

View reviewed changes

rafiss added 6 commits June 6, 2022 17:11

cli: remove query cancellation const

9937645

Release note (cli change): Ctrl+C (the interrupt signal) can now be used in the CLI to attempt to cancel the currently executing SQL query.

cli: re-enable test for setting DateStyle

833ec99

The upgrade to pgx has fixed this. Release note (bug fix): The DateStyle session setting is no longer ignored when using the CLI when set in the `options` URL parameter.

cli: simplify FormatValue

a945712

Release note: None

cli: remove ColumnTypeNames

a54a8f6

Release note: None

cli: remove unused return param from GetServerValue

926d1b8

Release note: None

cli: remove ColumnTypeScanType

aa02d14

Release note: None

rafiss force-pushed the cli-pgx-combined branch from ab7c311 to aa02d14 Compare June 6, 2022 21:11

rafiss commented Jun 6, 2022

View reviewed changes

craig bot merged commit e280a7f into cockroachdb:master Jun 6, 2022

This was referenced Jun 6, 2022

cli: allow COPY inside a transaction cockroachdb/docs#14040

Open

cli: remove query cancellation const cockroachdb/docs#14041

Open

rafiss deleted the cli-pgx-combined branch June 7, 2022 04:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: additional enhancements after switching to jackc/pgx #82101

cli: additional enhancements after switching to jackc/pgx #82101

rafiss commented May 31, 2022 •

edited

Loading

cockroach-teamcity commented May 31, 2022

knz commented Jun 1, 2022

rafiss commented Jun 1, 2022

otan left a comment

otan Jun 1, 2022

rafiss Jun 2, 2022

otan Jun 1, 2022

otan Jun 2, 2022

rafiss Jun 2, 2022

knz left a comment

rafiss commented Jun 2, 2022

rafiss commented Jun 2, 2022

rafiss left a comment

rafiss left a comment

rafiss commented Jun 4, 2022

knz left a comment

rafiss left a comment

craig bot commented Jun 6, 2022

cli: additional enhancements after switching to jackc/pgx #82101

cli: additional enhancements after switching to jackc/pgx #82101

Conversation

rafiss commented May 31, 2022 • edited Loading

cockroach-teamcity commented May 31, 2022

knz commented Jun 1, 2022

rafiss commented Jun 1, 2022

otan left a comment

Choose a reason for hiding this comment

otan Jun 1, 2022

Choose a reason for hiding this comment

rafiss Jun 2, 2022

Choose a reason for hiding this comment

otan Jun 1, 2022

Choose a reason for hiding this comment

otan Jun 2, 2022

Choose a reason for hiding this comment

rafiss Jun 2, 2022

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

rafiss commented Jun 2, 2022

rafiss commented Jun 2, 2022

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss commented Jun 4, 2022

knz left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

craig bot commented Jun 6, 2022

rafiss commented May 31, 2022 •

edited

Loading