sql: migrate prepared statements during session migration by rafiss · Pull Request #76399 · cockroachdb/cockroach

rafiss · 2022-02-10T21:19:42Z

See individual commits. The last one contains the business logic changes,
and the rest are just refactors.

Release note (sql change): The crdb_internal.serialize_session and
crdb_internal.deserialize_session now can handle prepared statements.
When deserializing, any prepared statements that existed when the
session was serialized are re-prepared. It is an error to re-prepare a
statement if the current session already has a statement with that name.

cockroach-teamcity · 2022-02-10T21:19:56Z

This change is

otan

changes lgtm.

one thing i am curious about - what happens to prepared statements if a schema change happened and the types change? in turn, does AddPreparedStatement during a session migration potentially change the result of the prepared statement producing an entirely different statement? is it worth testing?

andy-kimball · 2022-02-11T01:35:14Z

CC @RaduBerinde to make sure to take a look at this.

rafiss · 2022-02-13T20:47:50Z

what happens to prepared statements if a schema change happened and the types change?

This is the behavior without any session migration involved:

exec
CREATE TABLE t(a INT4, b TEXT, c DATE)
----

wire_prepare s2
INSERT INTO t VALUES($1, $2, $3)
----

exec
ALTER TABLE t DROP COLUMN c
----

wire_exec s2 1 cat 2022-02-10
----
ERROR: INSERT has more expressions than target columns, 3 expressions for 2 targets (SQLSTATE 42601)

in turn, does AddPreparedStatement during a session migration potentially change the result of the prepared statement producing an entirely different statement? is it worth testing?

yeah we definitely should test. given the above, i guess the two options are to not serialize a prepared statement if it's not valid at the time of serialization, or to ignore certain types of prepare errors at the time of deserialization.

rafiss · 2022-02-13T20:50:19Z

We could also keep the behavior as-is and just error out. (The user-impact is that their client would get a hard disconnect rather than a graceful session migration.)

otan · 2022-02-13T20:53:47Z

happy with erroring out. also happy for you to add this test in a separate PR.

rafiss · 2022-02-23T01:59:38Z

confirmed the behavior for prepared statements after a schema change at the last multi-tenant team meeting with signoff from @jeffswenson so going to merge this.

bors r=otan

craig · 2022-02-23T02:02:42Z

Canceled.

rafiss · 2022-02-23T02:02:54Z

bors r=otan

jaylim-crl

I believe this will fail:

cockroach/pkg/ccl/testccl/sqlccl/show_transfer_state_test.go

Lines 199 to 225 in 696cdd5

    
           t.Run("prepared_statements", func(t *testing.T) { 
        
           	pgURL, cleanup := sqlutils.PGUrl( 
        
           		t, 
        
           		tenant.SQLAddr(), 
        
           		"TestShowTransferState-errors-prepared_statements", 
        
           		url.UserPassword(security.TestUser, "hunter2"), 
        
           	) 
        
           	defer cleanup() 
        
           	conn, err := gosql.Open("postgres", pgURL.String()) 
        
           	require.NoError(t, err) 
        
           	defer conn.Close() 
        
           	// Use a dummy prepared statement. 
        
           	stmt, err := conn.Prepare("SELECT 1 WHERE 1 = 1") 
        
           	require.NoError(t, err) 
        
           	defer stmt.Close() 
        
           	var errVal, sessionState, sessionRevivalToken gosql.NullString 
        
           	err = conn.QueryRow("SHOW TRANSFER STATE").Scan(&errVal, &sessionState, &sessionRevivalToken) 
        
           	require.NoError(t, err) 
        
           	require.True(t, errVal.Valid) 
        
           	require.Equal(t, "cannot serialize a session which has portals or prepared statements", errVal.String) 
        
           	require.False(t, sessionState.Valid) 
        
           	require.False(t, sessionRevivalToken.Valid) 
        
           })

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jaylim-crl and @rafiss)

pkg/sql/types/types.go, line 1746 at r5 (raw file):

// messages
//and also to produce the output of SHOW CREATE.

nit: typo here?

craig · 2022-02-23T04:17:26Z

Build failed (retrying...):

GitHub CI (Cockroach)

rafiss

ah, whoops. fixing

bors r-

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jaylim-crl)

pkg/sql/types/types.go, line 1746 at r5 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

// messages
//and also to produce the output of SHOW CREATE.

nit: typo here?

fixed

craig · 2022-02-23T04:22:11Z

Canceled.

rafiss

@jaylim-crl i edited that test, in case you want to take a look

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jaylim-crl)

otan · 2022-02-23T05:52:46Z

~~do you think this is causing the TestShowTransferState/errors/prepared_statements flake?~~

edit: nvm i see it is done above

jaylim-crl

@jaylim-crl i edited that test, in case you want to take a look

Thanks. The test LGTM.

Reviewed 2 of 2 files at r1, 2 of 2 files at r2, 4 of 4 files at r3, 13 of 16 files at r4, 4 of 4 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

This will allow us to more easily call into the code to prepare statements when deserializing. Release note: None

Release note: None

After the recent refactor for deserialize_session, it no longer needs to be exposed. Release note: None

Release note (sql change): The crdb_internal.serialize_session and crdb_internal.deserialize_session now can handle prepared statements. When deserializing, any prepared statements that existed when the session was serialized are re-prepared. It is an error to re-prepare a statement if the current session already has a statement with that name.

A schema change can make an existing prepared statement invalid. Rather than returning an error when deserializing that invalid statement, now the error will be returned only when trying to use the statement. This makes a session migration even more transparent to the end user. Release note: None

rafiss · 2022-02-23T18:23:02Z

tftrs!

bors r=otan

craig · 2022-02-23T19:24:13Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-02-23T19:54:51Z

Build failed:

GitHub CI (Cockroach)

rafiss · 2022-02-23T20:13:07Z

the failures all seem to be flakes as far as i can tell..

bors r=otan

craig · 2022-02-23T21:44:33Z

Build failed:

GitHub CI (Cockroach)

rafiss · 2022-02-23T21:45:26Z

yet another flake..

bors r=otan

craig · 2022-02-23T22:52:06Z

Build succeeded:

GitHub CI (Cockroach)

otan · 2022-02-23T22:57:58Z

congrats on winning the CI lottery!

rafiss requested review from a team and otan February 10, 2022 21:19

rafiss requested a review from a team as a code owner February 10, 2022 21:19

otan approved these changes Feb 10, 2022

View reviewed changes

rafiss force-pushed the prepared-stmt-session-migration branch from 347fce3 to a1477d2 Compare February 23, 2022 01:51

rafiss force-pushed the prepared-stmt-session-migration branch from a1477d2 to ad02cf5 Compare February 23, 2022 02:02

jaylim-crl reviewed Feb 23, 2022

View reviewed changes

jaylim-crl self-requested a review February 23, 2022 03:33

rafiss commented Feb 23, 2022

View reviewed changes

rafiss force-pushed the prepared-stmt-session-migration branch from ad02cf5 to 762f214 Compare February 23, 2022 04:29

rafiss commented Feb 23, 2022

View reviewed changes

jaylim-crl reviewed Feb 23, 2022

View reviewed changes

rafiss added 5 commits February 23, 2022 13:06

sql: move {de}serialze_session to EvalPlanner

c36953f

This will allow us to more easily call into the code to prepare statements when deserializing. Release note: None

sql: expose an interface for preparing statements using evalCtx

7a51c5e

Release note: None

sql: remove CheckCanBecomeUser from EvalPlanner

73a2f47

After the recent refactor for deserialize_session, it no longer needs to be exposed. Release note: None

rafiss force-pushed the prepared-stmt-session-migration branch from 762f214 to dda2fa9 Compare February 23, 2022 18:18

craig bot merged commit 1f4dde2 into cockroachdb:master Feb 23, 2022

cockroach-teamcity mentioned this pull request Feb 23, 2022

sql: migrate prepared statements during session migration cockroachdb/docs#13098

Closed

rafiss deleted the prepared-stmt-session-migration branch February 24, 2022 18:13

	t.Run("prepared_statements", func(t *testing.T) {
	pgURL, cleanup := sqlutils.PGUrl(
	t,
	tenant.SQLAddr(),
	"TestShowTransferState-errors-prepared_statements",
	url.UserPassword(security.TestUser, "hunter2"),
	)
	defer cleanup()

	conn, err := gosql.Open("postgres", pgURL.String())
	require.NoError(t, err)
	defer conn.Close()

	// Use a dummy prepared statement.
	stmt, err := conn.Prepare("SELECT 1 WHERE 1 = 1")
	require.NoError(t, err)
	defer stmt.Close()

	var errVal, sessionState, sessionRevivalToken gosql.NullString
	err = conn.QueryRow("SHOW TRANSFER STATE").Scan(&errVal, &sessionState, &sessionRevivalToken)
	require.NoError(t, err)

	require.True(t, errVal.Valid)
	require.Equal(t, "cannot serialize a session which has portals or prepared statements", errVal.String)
	require.False(t, sessionState.Valid)
	require.False(t, sessionRevivalToken.Valid)
	})

Conversation

rafiss commented Feb 10, 2022

Uh oh!

cockroach-teamcity commented Feb 10, 2022

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

andy-kimball commented Feb 11, 2022

Uh oh!

rafiss commented Feb 13, 2022

Uh oh!

rafiss commented Feb 13, 2022

Uh oh!

otan commented Feb 13, 2022

Uh oh!

rafiss commented Feb 23, 2022

Uh oh!

craig bot commented Feb 23, 2022

Uh oh!

rafiss commented Feb 23, 2022

Uh oh!

jaylim-crl left a comment

Choose a reason for hiding this comment

Uh oh!

craig bot commented Feb 23, 2022

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

craig bot commented Feb 23, 2022

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

otan commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaylim-crl left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss commented Feb 23, 2022

Uh oh!

craig bot commented Feb 23, 2022

Uh oh!

craig bot commented Feb 23, 2022

Uh oh!

rafiss commented Feb 23, 2022

Uh oh!

craig bot commented Feb 23, 2022

Uh oh!

rafiss commented Feb 23, 2022

Uh oh!

craig bot commented Feb 23, 2022

Uh oh!

otan commented Feb 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

otan commented Feb 23, 2022 •

edited

Loading