sql: add SHOW TRANSFER STATE observer statement #76127

jaylim-crl · 2022-02-06T19:50:34Z

Informs #76000.

This commit adds the SHOW TRANSFER STATE observer statement, as described in
the sqlproxy connection migration RFC. This observer statement will be used
whenever a connection is about to be migrated to retrieve the relevant values
needed for the transfer process. A unique aspect to this statement is that
serialization or token generation errors will be returned as a SQL value
instead of an ErrorResponse. This will allow the sqlproxy to react accordingly,
and to reduce ambiguity issues during transferring.

This observer statement will only work on tenants (due to the need of token
generation). Since this is meant to be used internally only, there is no
release note.

Release note: None

cockroach-teamcity · 2022-02-06T19:50:41Z

This change is

jaylim-crl

Here's how it looks like:

demo@127.0.0.1:26257/movr> show transfer state with 'fookey';
-[ RECORD 1 ]
transfer_key                 | fookey
error                        | NULL
session_state_base64         | CkwKBG1vdnISECQgY29ja3JvYWNoIGRlbW8aBGRlbW8iBBACIgAoAjgIQgNVVENKBSR1c2VySgZwdWJsaWNaAGCAgIAgegCIAQGQAYBQEj4QkE4wAjgIQAJgAWgBcAF4AYgBAdgBAeABAfABAfgBAZACAbACgIAByAIBoAMBqQMAAAAAAECPQNADAeADAQ==
session_revival_token_base64 | CisKBGRlbW8SB0VkMjU1MTkaDAi52YCQBhC4jamiAyIMCOHUgJAGELiNqaIDEkCLi2FGhVZ7sgf3ZKveZyCq3gR9phInM0qpkW4grCvDpbSkq18DLuH3YOjtXKqs27OXpP+sXl5JFOEqm6xFNwsA


Time: 1ms total (execution 0ms / network 0ms)

This also works in a transaction with an aborted state:

demo@127.0.0.1:26257/movr> begin;
BEGIN


Time: 0ms total (execution 0ms / network 0ms)

demo@127.0.0.1:26257/movr  OPEN> select x();
ERROR: unknown function: x()
SQLSTATE: 42883
demo@127.0.0.1:26257/? ERROR> show transfer state with 'fookey';
-[ RECORD 1 ]
transfer_key                 | fookey
error                        | cannot serialize a session which is inside a transaction
session_state_base64         | NULL
session_revival_token_base64 | NULL


Time: 0ms total (execution 0ms / network 0ms)

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/sem/builtins/builtins.go

pkg/sql/parser/sql.y

pkg/sql/conn_executor_test.go

docs/generated/sql/bnf/show_transfer_stmt.bnf

jaylim-crl

TFTRs!

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

docs/generated/sql/bnf/show_transfer_stmt.bnf, line 3 at r1 (raw file):

Previously, JeffSwenson (Jeff Swenson) wrote…

Can we make the WITH clause required? The sql proxy will always supply the WITH clause and removing the option simplifies the implementation+tests.

This was outlined in the RFC: https://github.com/cockroachdb/cockroach/pull/75707/files#diff-02506fc029046c281e8c18c110a3a87330d6f3d035c91ba6d95579f0cb04d326R287

I'd like to make the WITH clause optional, which will make things cleaner. It is true that the SQL proxy will always use the WITH clause, but an operator might not do so during debugging. I also do not want to restrict this statement to just the SQL proxy. It's awkward to pass in an empty string when we don't need it, e.g. SHOW TRANSFER STATE WITH ''.

pkg/sql/parser/sql.y, line 5802 at r1 (raw file):

Previously, JeffSwenson (Jeff Swenson) wrote…

nit: "display information used by the sql proxy to transfer connections between sql instances"

I'll update this to something more concise, but I don't think it is necessary to mention "between SQL instances". Perhaps something along the lines of "display session state for connection migration". I also do not want to restrict this feature to just the SQL proxy.

If we renamed this to SHOW TRANSFER VARIABLES, we could do something like "display connection migration variables". I originally thought of VARIABLES, but settled with STATE. Happy to bikeshed.

pkg/sql/parser/sql.y, line 5803 at r1 (raw file):

Previously, JeffSwenson (Jeff Swenson) wrote…

nit: the category should be "Misc"

Will update. This was cargo-culted from statements around it.

pkg/sql/sem/builtins/builtins.go, line 6311 at r1 (raw file):

Previously, JeffSwenson (Jeff Swenson) wrote…

I think we should delete crdb_internal.serialize_session.

I strongly think that we shouldn't for a couple of reasons:

SHOW TRANSFER STATE is very generic, and one cannot use that as a statement source, e.g. SELECT session_state FROM [SHOW TRANSFER STATE] will fail. Restricting the use case to just SHOW TRANSFER STATE will make testing and general usage harder.
My understanding is that usually these SHOW variances are backed by a more complex underlying query (see delegate.go). Since (1) is a problem, having the internal builtins would help if one needs to query particular columns.
The internal builtins are helping a lot with debugging and testing. To add on, there's already a deserialize_session built-in, so it is natural to have the other pair. The same argument can be made for crdb_internal.create_session_revival_token.

I find (3) extremely helpful. The output for SHOW TRANSFER STATE is often long, and hard to copy.

pkg/sql/conn_executor_test.go, line 1257 at r1 (raw file):

Previously, JeffSwenson (Jeff Swenson) wrote…

nit: assert the content of the columns instead of the number of columns.
require.Equal(t, ["error", "session_state_base64", "session_revival_token_base64"], resultColumns)

Will update.

otan · 2022-02-08T10:56:11Z

pkg/ccl/testccl/sqlccl/show_transfer_state_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestShowTransferState(t *testing.T) {


Can we make this a data driven test? It would be a lot easier to read.
You can copy an example from

cockroach/pkg/sql/session_migration_test.go

Line 40 in 03c50b0

func TestSessionMigration(t *testing.T) {

otan · 2022-02-08T11:00:04Z

pkg/sql/sem/tree/show.go

+	// statement. This allows an empty string to be passed as a transfer key if
+	// necessary.
+	WithTransferKey bool
+	TransferKey     string


should we make this Name ? then you won't need to deal with ' in formatting.

otan · 2022-02-08T11:03:05Z

pkg/sql/sem/tree/show.go

+	// WithTransferKey indicates that "WITH <transfer_key>" was used in the
+	// statement. This allows an empty string to be passed as a transfer key if
+	// necessary.
+	WithTransferKey bool


is empty string a valid transfer key? if not, can we avoid the WithTransferKey check if it is empty?
(also avoided if we do not use Name)

rafiss

Reviewed 5 of 21 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jaylim-crl, @jeffswenson, and @otan)

pkg/sql/conn_executor_exec.go, line 1745 at r1 (raw file):

	}
	if stmt.WithTransferKey {
		colNames = append([]string{"transfer_key"}, colNames...)

any specific reason to make transfer_key the first column instead of the last? not a huge deal, but i'd expect "optional" columns to appear at the end.

relatedly, we could make it so that if WITH TRANSFER KEY is not given, this command still returns a random transfer key

pkg/sql/conn_executor_exec.go, line 1755 at r1 (raw file):

	row := make(tree.Datums, len(colNames))
	errIdx := 0
	if stmt.WithTransferKey {

could change to if stmt.TransferKey != "" {, and remove WithTransferKey

pkg/sql/conn_executor_exec.go, line 1775 at r1 (raw file):

		fn, ok := showTransferStateFns[colNames[i]]
		if !ok {
			panic(fmt.Errorf("generator fn must exist for column '%s'", colNames[i]))

this will crash the node - i think i'd prefer to just call finishWithError(errors.NewAssertionFailed(...))

pkg/sql/session_revival_token.go, line 37 at r1 (raw file):

// NOTE: This is used within an observer statement directly, and should not rely
// on the planner because those statements do not get planned.
func createSessionRevivalToken(

the signature could change to

func createSessionRevivalToken(ex *connExecutor) (*tree.DBytes, error)

to reduce the amount of unwrapping needed to be done before calling this

pkg/sql/session_state.go, line 38 at r1 (raw file):

// NOTE: This is used within an observer statement directly, and should not rely
// on the planner because those statements do not get planned.
func serializeSessionState(

the signature could change to

func serializeSessionState(ex *connExecutor) (*tree,DBytes, error)

which I think it would make the call-sites more simple

pkg/sql/parser/sql.y, line 5808 at r1 (raw file):

  SHOW TRANSFER STATE WITH non_reserved_word_or_sconst
  {
     $$.val = &tree.ShowTransferState{WithTransferKey: true, TransferKey: $5}

as mentioned above, the bool field can be removed for simplicity

pkg/sql/sem/builtins/builtins.go, line 6311 at r1 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

I strongly think that we shouldn't for a couple of reasons:

SHOW TRANSFER STATE is very generic, and one cannot use that as a statement source, e.g. SELECT session_state FROM [SHOW TRANSFER STATE] will fail. Restricting the use case to just SHOW TRANSFER STATE will make testing and general usage harder.

My understanding is that usually these SHOW variances are backed by a more complex underlying query (see delegate.go). Since (1) is a problem, having the internal builtins would help if one needs to query particular columns.

The internal builtins are helping a lot with debugging and testing. To add on, there's already a deserialize_session built-in, so it is natural to have the other pair. The same argument can be made for crdb_internal.create_session_revival_token.

I find (3) extremely helpful. The output for SHOW TRANSFER STATE is often long, and hard to copy.

re (3): these builtins made it easier to test the underlying functionality using logic tests, which is nice

pkg/sql/sem/tree/show.go, line 895 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

should we make this Name ? then you won't need to deal with ' in formatting.

+1

then you can call ctx.FormatNode(TransferKey)

pkg/sql/parser/testdata/show, line 1527 at r1 (raw file):


parse
SHOW TRANSFER STATE WITH foo

could you add a test for SHOW TRANSFER STATE WITH 'fo''o'

jaylim-crl

TFTRs! This is ready for another round of review.

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

pkg/sql/conn_executor_exec.go, line 1745 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

any specific reason to make transfer_key the first column instead of the last? not a huge deal, but i'd expect "optional" columns to appear at the end.

relatedly, we could make it so that if WITH TRANSFER KEY is not given, this command still returns a random transfer key

Interesting UX. I've updated it to your suggestion (i.e. moved transfer_key to the last column). I punted on the second idea of generating a random transfer key since I don't see a strong use case to that. How strongly do you feel about that?

pkg/sql/conn_executor_exec.go, line 1755 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

could change to if stmt.TransferKey != "" {, and remove WithTransferKey

Done.

pkg/sql/conn_executor_exec.go, line 1775 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this will crash the node - i think i'd prefer to just call finishWithError(errors.NewAssertionFailed(...))

This was on purpose since this is a logic error. That said, I've updated this to your suggestion to be safe.

pkg/sql/session_revival_token.go, line 37 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

the signature could change to
func createSessionRevivalToken(ex *connExecutor) (*tree.DBytes, error)
to reduce the amount of unwrapping needed to be done before calling this

This doesn't seem to be possible since we can't get a connExecutor from a planner. CreateSessionRevivalToken which will be used by the builtin calls this. Am I missing something here?

pkg/sql/session_state.go, line 38 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

the signature could change to
func serializeSessionState(ex *connExecutor) (*tree,DBytes, error)
which I think it would make the call-sites more simple

See my other comment on this. Also, I noticed that you imported this into your other PR. I'll rebase this if that gets merged first.

pkg/sql/parser/sql.y, line 5802 at r1 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

I'll update this to something more concise, but I don't think it is necessary to mention "between SQL instances". Perhaps something along the lines of "display session state for connection migration". I also do not want to restrict this feature to just the SQL proxy.

If we renamed this to SHOW TRANSFER VARIABLES, we could do something like "display connection migration variables". I originally thought of VARIABLES, but settled with STATE. Happy to bikeshed.

Done. Updated to something more concise.

pkg/sql/parser/sql.y, line 5803 at r1 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

Will update. This was cargo-culted from statements around it.

Done.

pkg/sql/parser/sql.y, line 5808 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

as mentioned above, the bool field can be removed for simplicity

Done.

pkg/sql/sem/builtins/builtins.go, line 6311 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

re (3): these builtins made it easier to test the underlying functionality using logic tests, which is nice

I left this as-is. I think we can revisit this decision as an enhancement once we support SHOW TRANSFER STATE as a statement source.

pkg/sql/sem/tree/show.go, line 894 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

is empty string a valid transfer key? if not, can we avoid the WithTransferKey check if it is empty?
(also avoided if we do not use Name)

The transfer key has no meaning to the database, and will only be interpreted by the caller, so both empty and non-empty keys should be valid. That said, I've updated this to Name, so only non-empty ones are valid now. If we pass in an empty string, we will not display the transfer_key column.

pkg/sql/sem/tree/show.go, line 895 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

+1

then you can call ctx.FormatNode(TransferKey)

Done.

pkg/sql/parser/testdata/show, line 1527 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

could you add a test for SHOW TRANSFER STATE WITH 'fo''o'

Done.

pkg/ccl/testccl/sqlccl/show_transfer_state_test.go, line 26 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

Can we make this a data driven test? It would be a lot easier to read.
You can copy an example from

cockroach/pkg/sql/session_migration_test.go

Line 40 in 03c50b0

func TestSessionMigration(t *testing.T) {

I looked into this, and I don't think it is worth it given that there are very few cases. Using a data driven test restricts us from testing a lot of aspects, especially the ones that I'd like to cover (e.g. column names, validity of results, setting params in startup message, etc.), and all of them have a single case each. We can't print the whole token to the testdata file either since that contains timestamps. To add on, not being able to use SHOW TRANSFER STATE as a statement source makes it even difficult. How strongly do you feel?

pkg/sql/conn_executor_test.go, line 1257 at r1 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

Will update.

Done.

otan

lgtm but i'll let @rafiss signoff :)

Reviewed 2 of 21 files at r1, 9 of 15 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jaylim-crl, @jeffswenson, @otan, and @rafiss)

pkg/ccl/testccl/sqlccl/show_transfer_state_test.go, line 26 at r1 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

I looked into this, and I don't think it is worth it given that there are very few cases. Using a data driven test restricts us from testing a lot of aspects, especially the ones that I'd like to cover (e.g. column names, validity of results, setting params in startup message, etc.), and all of them have a single case each. We can't print the whole token to the testdata file either since that contains timestamps. To add on, not being able to use SHOW TRANSFER STATE as a statement source makes it even difficult. How strongly do you feel?

if you feel that way :) i'm not too fussed, it's a lot easier to maintain imo but i don't want to block you on it.

pkg/ccl/testccl/sqlccl/show_transfer_state_test.go, line 220 at r2 (raw file):

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

this might conflict with #76399

pkg/sql/conn_executor_test.go, line 1237 at r2 (raw file):

// will return an error as a SQL value whenever a tenant server is not used.
// For actual tests, see pkg/ccl/testccl/sqlccl.
func TestShowTransferStateError(t *testing.T) {

this feel like it should be tested with a logictest.

in pkg/sql/logictest/testdata/logic_test, add something like

# LogicTest: !3node-tenant

statement error session revival tokens are not supported in this cluster
SHOW TRANSFER STATE


statement error session revival tokens are not supported in this cluster
SHOW TRANSFER STATE FROM "foobar"

jaylim-crl

TFTR!

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

pkg/ccl/testccl/sqlccl/show_transfer_state_test.go, line 26 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

if you feel that way :) i'm not too fussed, it's a lot easier to maintain imo but i don't want to block you on it.

Sounds good :)

pkg/ccl/testccl/sqlccl/show_transfer_state_test.go, line 220 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

this might conflict with #76399

Ack - I'm aware of that, and if that merges in first, I'll just rebase, and this test will be removed.

pkg/sql/conn_executor_test.go, line 1237 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

this feel like it should be tested with a logictest.

in pkg/sql/logictest/testdata/logic_test, add something like
# LogicTest: !3node-tenant

statement error session revival tokens are not supported in this cluster
SHOW TRANSFER STATE


statement error session revival tokens are not supported in this cluster
SHOW TRANSFER STATE FROM "foobar"

That would be ideal, but is not possible. SHOW TRANSFER STATE will never return a statement error (at least for most of the time). Errors are embedded within the error column, so the existing logictest system that checks for statement errors would not work.

Here's what we get for a system tenant:

demo@127.0.0.1:26258/defaultdb> show transfer state;
                           error                           | session_state_base64 | session_revival_token_base64
-----------------------------------------------------------+----------------------+-------------------------------
  session revival tokens are not supported on this cluster | NULL                 | NULL
(1 row)

otan · 2022-02-14T04:33:41Z

pkg/sql/conn_executor_test.go, line 1237 at r2 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

That would be ideal, but is not possible. SHOW TRANSFER STATE will never return a statement error (at least for most of the time). Errors are embedded within the error column, so the existing logictest system that checks for statement errors would not work.

Here's what we get for a system tenant:
demo@127.0.0.1:26258/defaultdb> show transfer state;
                           error                           | session_state_base64 | session_revival_token_base64
-----------------------------------------------------------+----------------------+-------------------------------
  session revival tokens are not supported on this cluster | NULL                 | NULL
(1 row)

huh, remind me why

query TTT
SHOW TRANSFER STATE

won't work / why we don't error in this case instead?

jaylim-crl

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

pkg/sql/conn_executor_test.go, line 1237 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

huh, remind me why
query TTT
SHOW TRANSFER STATE
won't work / why we don't error in this case instead?

Sorry, you're right. I was focusing on the statement error op, and had forgotten about the query op. Done, and updated accordingly. Also note that this approach won't test the column names, but that's covered in the other test, so I'm not too worried about that. Column names are important here because that's what the proxy will be parsing on to look for the right message in the response. If the ordering changes in the future, and the proxy doesn't know about it, we might end up skipping over the response, and sending it back to the client (for a query sent by the proxy), which is a correctness issue.

jeffswenson

LGTM

pkg/sql/conn_executor_exec.go

jaylim-crl

TFTR! I'll address your nit after Rafi takes a look at this.

@rafiss, if you get a chance to, could you give it another review?

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

pkg/sql/conn_executor_exec.go, line 1782 at r2 (raw file):

Previously, JeffSwenson (Jeff Swenson) wrote…

nit: Since 2 of the 4 columns use the showTransferStateFns map, I think this function would be more obvious if it called the functions directly.

E.x. something like:

var sessionState, revivalToken tree.Datums
err := func() error {
  if sessionState, err = sessionStateBase64(...); err != nil {
    return err
  }
  if revivalToken, err = createRevivalToken(...); err != nil {
    return err
  }
}()
if err != nil {
  row = []tree.Datum{ errToDatum(err), nil, nil, nil }
} else {
  row = []tree.Datum{nil, sessionState, revivalToken, transferKey}
}
return res.AddRow(ctx, row)

I wanted this to be extensible. But sure, I can do that.

rafiss

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

pkg/sql/session_revival_token.go, line 37 at r1 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

This doesn't seem to be possible since we can't get a connExecutor from a planner. CreateSessionRevivalToken which will be used by the builtin calls this. Am I missing something here?

ah you're right -- my suggestion would require adding a new interface that is implemented by connExecutor, and use that interface as a field of the planner.

might be a useful refactor, but can be done separately.

pkg/sql/session_state.go, line 38 at r1 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

See my other comment on this. Also, I noticed that you imported this into your other PR. I'll rebase this if that gets merged first.

sgtm!

pkg/sql/sem/tree/show.go, line 899 at r3 (raw file):

	if node.TransferKey != "" {
		ctx.WriteString(" WITH ")
		ctx.WithFlags(ctx.flags & ^FmtBareIdentifiers, func() {

could you leave a comment saying why we need the ^FmtBareIdentifiers here? it's not common that we'd override the flags

pkg/sql/logictest/testdata/logic_test/show_transfer_state, line 4 at r3 (raw file):


# Statement does not work on system tenant.
query TTT

there was a different thread about testing column names. you can do that with query TTT colnames

pkg/sql/parser/testdata/show, line 1531 at r3 (raw file):

SHOW TRANSFER STATE WITH "foo-bar"
SHOW TRANSFER STATE WITH "foo-bar" -- fully parenthesized
SHOW TRANSFER STATE WITH "foo-bar" -- literals removed

hm, interesting, so this needs double quotes now that it is name? i think that's actually slightly less desirable..

one reason is that i think it's better if the transferKey is redacted as part of the "literals removed" step. (this matters for how the statement appears in the DB console. if it's not redacted as a constant, then a separate statement will be shown in the console for each transfer key.)

otan · 2022-02-16T20:43:41Z

pkg/sql/parser/testdata/show, line 1531 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

hm, interesting, so this needs double quotes now that it is name? i think that's actually slightly less desirable..

one reason is that i think it's better if the transferKey is redacted as part of the "literals removed" step. (this matters for how the statement appears in the DB console. if it's not redacted as a constant, then a separate statement will be shown in the console for each transfer key.)

only if there are special characters. i think that's a good thing...

jaylim-crl

TFTR! I applied the patch from @rafiss to use SCONST instead of name, and addressed the remaining concerns.

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

pkg/sql/conn_executor_exec.go, line 1782 at r2 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

I wanted this to be extensible. But sure, I can do that.

Done.

pkg/sql/session_revival_token.go, line 37 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

ah you're right -- my suggestion would require adding a new interface that is implemented by connExecutor, and use that interface as a field of the planner.

might be a useful refactor, but can be done separately.

Right. That said, I'm not sure if it's worth it since it'll probably only be used by the session revival token and session state work. I'll leave it as-is for now.

pkg/sql/session_state.go, line 38 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

sgtm!

Done.

pkg/sql/sem/tree/show.go, line 899 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

could you leave a comment saying why we need the ^FmtBareIdentifiers here? it's not common that we'd override the flags

Done. Also no longer relevant.

pkg/sql/logictest/testdata/logic_test/show_transfer_state, line 4 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

there was a different thread about testing column names. you can do that with query TTT colnames

Done. Thanks - didn't know this is a thing.

pkg/sql/parser/testdata/show, line 1531 at r3 (raw file):

Previously, otan (Oliver Tan) wrote…

only if there are special characters. i think that's a good thing...

Done. I applied the patch from @rafiss to use a constant string instead.

rafiss

nice work!

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

jaylim-crl

TFTR! I'll wait until #75660 gets merged, and then fix up the cluster settings before merging this in.

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

Informs cockroachdb#76000. This commit adds the SHOW TRANSFER STATE observer statement, as described in the sqlproxy connection migration RFC. This observer statement will be used whenever a connection is about to be migrated to retrieve the relevant values needed for the transfer process. A unique aspect to this statement is that serialization or token generation errors will be returned as a SQL value instead of an ErrorResponse. This will allow the sqlproxy to react accordingly, and to reduce ambiguity issues during transferring. This observer statement will only work on tenants (due to the need of token generation). Since this is meant to be used internally only, there is no release note. Release note: None Co-authored-by: Jay <jay@cockroachlabs.com> Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>

jaylim-crl · 2022-02-19T16:15:44Z

bors r=rafiss,JeffSwenson

craig · 2022-02-19T16:38:31Z

Build failed:

GitHub CI (Cockroach)

jaylim-crl · 2022-02-19T17:35:13Z

bors r=rafiss,JeffSwenson

craig · 2022-02-19T18:37:19Z

Build succeeded:

GitHub CI (Cockroach)

jaylim-crl force-pushed the jay/obs-stmt branch from 5279727 to 5a8dab1 Compare February 6, 2022 19:51

jaylim-crl commented Feb 6, 2022

View reviewed changes

jaylim-crl force-pushed the jay/obs-stmt branch 2 times, most recently from 03ae737 to e8b1e3f Compare February 7, 2022 01:42

jaylim-crl marked this pull request as ready for review February 7, 2022 12:16

jaylim-crl requested a review from a team as a code owner February 7, 2022 12:16

jaylim-crl requested review from jeffswenson, rafiss and otan and removed request for a team February 7, 2022 12:17

jeffswenson reviewed Feb 7, 2022

View reviewed changes

jaylim-crl requested a review from jeffswenson February 8, 2022 01:18

jaylim-crl commented Feb 8, 2022

View reviewed changes

otan reviewed Feb 8, 2022

View reviewed changes

rafiss requested changes Feb 8, 2022

View reviewed changes

jaylim-crl mentioned this pull request Feb 11, 2022

sql, ccl/sqlproxyccl: connection migration tracking issue #76000

Closed

14 tasks

jaylim-crl force-pushed the jay/obs-stmt branch from e8b1e3f to 44308d2 Compare February 11, 2022 20:11

blathers-crl bot requested a review from otan February 11, 2022 20:11

jaylim-crl requested a review from rafiss February 11, 2022 20:12

jaylim-crl commented Feb 11, 2022

View reviewed changes

jaylim-crl force-pushed the jay/obs-stmt branch from 44308d2 to 5bb6fe2 Compare February 12, 2022 04:28

otan reviewed Feb 14, 2022

View reviewed changes

jaylim-crl requested a review from otan February 14, 2022 04:31

jaylim-crl commented Feb 14, 2022

View reviewed changes

jaylim-crl force-pushed the jay/obs-stmt branch from 5bb6fe2 to d7867c2 Compare February 14, 2022 12:47

jaylim-crl commented Feb 14, 2022

View reviewed changes

jeffswenson approved these changes Feb 14, 2022

View reviewed changes

pkg/sql/conn_executor_exec.go Outdated Show resolved Hide resolved

jaylim-crl requested a review from jeffswenson February 14, 2022 23:51

jaylim-crl commented Feb 14, 2022

View reviewed changes

jeffswenson approved these changes Feb 15, 2022

View reviewed changes

rafiss requested changes Feb 16, 2022

View reviewed changes

jaylim-crl force-pushed the jay/obs-stmt branch 2 times, most recently from 86bbef6 to 1da044d Compare February 17, 2022 01:38

jaylim-crl requested review from jeffswenson and rafiss February 17, 2022 01:45

jaylim-crl commented Feb 17, 2022

View reviewed changes

jaylim-crl force-pushed the jay/obs-stmt branch from 1da044d to 0072aa8 Compare February 17, 2022 01:46

jaylim-crl requested a review from a team as a code owner February 17, 2022 01:46

jaylim-crl force-pushed the jay/obs-stmt branch from 0072aa8 to 3fdbc14 Compare February 17, 2022 14:24

jaylim-crl removed the request for review from a team February 17, 2022 14:24

rafiss approved these changes Feb 17, 2022

View reviewed changes

jaylim-crl requested a review from rafiss February 17, 2022 15:48

jaylim-crl commented Feb 17, 2022

View reviewed changes

jaylim-crl force-pushed the jay/obs-stmt branch from 3fdbc14 to 36a7a2c Compare February 19, 2022 05:03

craig bot merged commit d05e52e into cockroachdb:master Feb 19, 2022

jaylim-crl deleted the jay/obs-stmt branch February 19, 2022 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add SHOW TRANSFER STATE observer statement #76127

sql: add SHOW TRANSFER STATE observer statement #76127

jaylim-crl commented Feb 6, 2022

cockroach-teamcity commented Feb 6, 2022

jaylim-crl left a comment

jaylim-crl left a comment

otan Feb 8, 2022

otan Feb 8, 2022

otan Feb 8, 2022

rafiss left a comment

jaylim-crl left a comment

otan left a comment

jaylim-crl left a comment

otan commented Feb 14, 2022

jaylim-crl left a comment

jeffswenson left a comment

jaylim-crl left a comment

rafiss left a comment

otan commented Feb 16, 2022

jaylim-crl left a comment

rafiss left a comment

jaylim-crl left a comment

jaylim-crl commented Feb 19, 2022

craig bot commented Feb 19, 2022

jaylim-crl commented Feb 19, 2022

craig bot commented Feb 19, 2022

sql: add SHOW TRANSFER STATE observer statement #76127

sql: add SHOW TRANSFER STATE observer statement #76127

Conversation

jaylim-crl commented Feb 6, 2022

cockroach-teamcity commented Feb 6, 2022

jaylim-crl left a comment

Choose a reason for hiding this comment

jaylim-crl left a comment

Choose a reason for hiding this comment

otan Feb 8, 2022

Choose a reason for hiding this comment

otan Feb 8, 2022

Choose a reason for hiding this comment

otan Feb 8, 2022

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

jaylim-crl left a comment

Choose a reason for hiding this comment

otan left a comment

Choose a reason for hiding this comment

jaylim-crl left a comment

Choose a reason for hiding this comment

otan commented Feb 14, 2022

jaylim-crl left a comment

Choose a reason for hiding this comment

jeffswenson left a comment

Choose a reason for hiding this comment

jaylim-crl left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

otan commented Feb 16, 2022

jaylim-crl left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

jaylim-crl left a comment

Choose a reason for hiding this comment

jaylim-crl commented Feb 19, 2022

craig bot commented Feb 19, 2022

jaylim-crl commented Feb 19, 2022

craig bot commented Feb 19, 2022