Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix DDL Execution on reserved connection without in active transaction #6514

Merged
merged 4 commits into from
Aug 10, 2020
Merged

Fix DDL Execution on reserved connection without in active transaction #6514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa73211
perform beginagain on ddl only if connection has transaction open
harshit-gangal Jul 31, 2020
1a5f5cd
ddl test on reserved connection with out tx
harshit-gangal Jul 31, 2020
4b4956c
ddl test on reserved connection with tx
harshit-gangal Jul 31, 2020
1e8462f
change match describe to nonzero row
harshit-gangal Jul 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions go/vt/vttablet/endtoend/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,13 @@ var tableACLConfig = `{
"writers": ["dev"],
"admins": ["dev"]
},
{
"name": "vitess_test_ddl",
"table_names_or_prefixes": ["vitess_test_ddl"],
"readers": ["dev"],
"writers": ["dev"],
"admins": ["dev"]
},
{
"name": "vitess_seq",
"table_names_or_prefixes": ["vitess_seq"],
Expand Down
52 changes: 52 additions & 0 deletions go/vt/vttablet/endtoend/reserve_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,6 +799,58 @@ func TestReserveExecuteWithExecuteFailureAndCheckConnectionAndDBState(t *testing
require.Error(t, client.Release())
}

func TestReserveExecuteDDLWithoutTx(t *testing.T) {
client := framework.NewClient()
defer client.Release()

connQuery := "select connection_id()"
createQuery := "create table vitess_test_ddl(id bigint primary key)"
dropQuery := "drop table vitess_test_ddl"
descQuery := "describe vitess_test_ddl"

qr1, err := client.ReserveExecute(connQuery, nil, nil)
require.NoError(t, err)

_, err = client.Execute(createQuery, nil)
require.NoError(t, err)
require.Zero(t, client.TransactionID())
defer client.Execute(dropQuery, nil)

qr2, err := client.Execute(connQuery, nil)
require.NoError(t, err)
assert.Equal(t, qr1.Rows, qr2.Rows)

qr3, err := client.Execute(descQuery, nil)
require.NoError(t, err)
require.NotZero(t, qr3.Rows)
}

func TestReserveExecuteDDLWithTx(t *testing.T) {
client := framework.NewClient()
defer client.Release()

connQuery := "select connection_id()"
createQuery := "create table vitess_test_ddl(id bigint primary key)"
dropQuery := "drop table vitess_test_ddl"
descQuery := "describe vitess_test_ddl"

qr1, err := client.ReserveBeginExecute(connQuery, nil, nil)
require.NoError(t, err)

_, err = client.Execute(createQuery, nil)
require.NoError(t, err)
require.NotZero(t, client.TransactionID())
defer client.Execute(dropQuery, nil)

qr2, err := client.Execute(connQuery, nil)
require.NoError(t, err)
assert.Equal(t, qr1.Rows, qr2.Rows)

qr3, err := client.Execute(descQuery, nil)
require.NoError(t, err)
require.NotZero(t, qr3.Rows)
}

func killConnection(t *testing.T, connID string) {
client := framework.NewClient()
_, err := client.ReserveExecute("select 1", []string{fmt.Sprintf("kill %s", connID)}, nil)
Expand Down
10 changes: 7 additions & 3 deletions go/vt/vttablet/tabletserver/query_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,9 +376,13 @@ func (qre *QueryExecutor) execDDL(conn *StatefulConnection) (*sqltypes.Result, e
if err != nil {
return nil, err
}
err = qre.BeginAgain(qre.ctx, conn)
if err != nil {
return nil, err
// Only perform this operation when the connection has transaction open.
// TODO: This actually does not retain the old transaction. We should see how to provide correct behaviour to client.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove the TODO. The DDL specs say that it's an implicit commit. So, this is correct behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here it to not start a new transaction as the current existing transaction is committed. Instead VTGate session should clear out transaction details it is holding.

if conn.txProps != nil {
err = qre.BeginAgain(qre.ctx, conn)
if err != nil {
return nil, err
}
}
return result, nil
}
Expand Down