tabletserver: SBR deprecation by sougou · Pull Request #5940 · vitessio/vitess

sougou · 2020-03-20T06:13:51Z

The planbuilder and query_executor were essentially rewritten. I had to make tweaks in a few other places to accommodate changes in behavior.

It was not necessary, and was obfuscating the fact that it's wrapping TxEngine. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

There was an aspiration that applications will use an event token to validate how fresh a replica read was. The feature was neither very usable nor used by anyone. This has now been deleted. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

go/vt/vttablet/tabletserver/planbuilder/plan.go

systay · 2020-03-20T09:06:27Z

go/vt/vttablet/tabletserver/query_executor_test.go

+			qre := newTestQueryExecutor(ctx, tsv, tcase.input, 0)
+			_, err := qre.Execute()
+			wantErr := "caller id: d: row count exceeded 2 (errno 10001) (sqlstate HY000)"
+			if err == nil || err.Error() != wantErr {


nit: You could use

require.Error(t, wantErr)

For some reason, assert didn't work. In any case, I had to change the test to use "Contains".
But assert.Contains can't check if an error contains a string, which is unfortunate.

systay

👍

harshit-gangal · 2020-03-20T11:23:07Z

go/vt/vttablet/tabletserver/connpool/dbconn.go

 	getModeSQL    = "select @@global.sql_mode"
 	getAutocommit = "select @@autocommit"
 	getAutoIsNull = "select @@sql_auto_is_null"
-	showBinlog    = "show variables like 'binlog_format'"


should we not verify bin log format. Now it has to be ROW.

We still want to allow some legacy usage. People transitioning should be able to run SBR. RBR becomes a hard requirement only while resharding.

harshit-gangal · 2020-03-20T11:31:49Z

go/vt/vttablet/tabletserver/planbuilder/plan.go

 var planName = [NumPlans]string{
-	"PASS_SELECT",
-	"SELECT_LOCK",
-	"NEXTVAL",
-	"PASS_DML",
-	"DML_PK",
-	"DML_SUBQUERY",
-	"INSERT_PK",
-	"INSERT_SUBQUERY",
-	"UPSERT_PK",
-	"INSERT_TOPIC",
-	"INSERT_MESSAGE",
-	"SET",
+	"Select",
+	"SelectLock",
+	"Nextval",
+	"SelectImpossible",
+	"Insert",
+	"InsertTopic",
+	"InsertMessage",
+	"Update",
+	"UpdateLimit",
+	"Delete",
+	"DeleteLimit",
 	"DDL",
-	"SELECT_STREAM",
-	"OTHER_READ",
-	"OTHER_ADMIN",
-	"MESSAGE_STREAM",
-	"SELECT_IMPOSSIBLE",
+	"Set",
+	"OtherRead",
+	"OtherAdmin",
+	"SelectStream",
+	"MessageStream",
 }


I believe this should not be renamed as this impacts the query stats naming. All metrics will get disjoint.

This is a good time to rename everything uniformly because this has been announced as one of the breaking changes. People anyway have to deal with all the other new values already.

harshit-gangal · 2020-03-20T11:59:37Z

go/vt/vttablet/tabletserver/tabletenv/config.go

 	flag.BoolVar(&Config.PassthroughDMLs, "queryserver-config-passthrough-dmls", DefaultQsConfig.PassthroughDMLs, "query server pass through all dml statements without rewriting")
-	flag.BoolVar(&Config.AllowUnsafeDMLs, "queryserver-config-allowunsafe-dmls", DefaultQsConfig.AllowUnsafeDMLs, "query server allow unsafe dml statements")
+	flag.BoolVar(&deprecateAllowUnsafeDMLs, "queryserver-config-allowunsafe-dmls", false, "deprecated")


what is the difference between pass through and unsafe dmls?

Unsafe DMLs are those that were considered unsafe for SBR, like "insert on duplicate key", etc.

harshit-gangal · 2020-03-20T12:22:52Z

go/vt/vttablet/tabletserver/tabletserver.go

 		ctx, tsv.QueryTimeout.Get(),
 		"CreateTransaction", "create_transaction", nil,
-		target, nil, true /* isBegin */, true, /* allowOnShutdown */
+		target, nil, true, /* allowOnShutdown */


for CreateTransaction method: should this be false?

It either doesn't matter, or it's extremely important that it be true. Changing to false is likely risky without knowing why.

harshit-gangal · 2020-03-20T12:46:05Z

proto/query.proto

+  // This used to be ResultExtras.
+  reserved 5;
+
  repeated Field fields = 1;
  uint64 rows_affected = 2;
  uint64 insert_id = 3;
  repeated Row rows = 4;


As this works, I believe ordering is not important.

Better to follow the rules. Then we don't have to worry about corner cases.

harshit-gangal · 2020-03-20T13:01:51Z

go/vt/vttablet/tabletserver/query_executor.go

+		if err := qre.verifyRowCount(int64(len(qr.Rows)), maxrows); err != nil {
+			return nil, err


Test exists for DML Limit. Not able to find a test for select limit.

Good catch. I must have accidentally deleted an existing test.

harshit-gangal · 2020-03-20T13:15:06Z

go/vt/vttablet/tabletserver/query_executor.go

 	case planbuilder.PlanSelectImpossible:
 		if qre.plan.Fields != nil {
 			return &sqltypes.Result{
-				Fields:       qre.plan.Fields,
-				RowsAffected: 0,
-				InsertID:     0,
-				Rows:         nil,
-				Extras:       nil,
+				Fields: qre.plan.Fields,
 			}, nil
 		}
 	}


case planbuilder.PlanSelect, planbuilder.PlanSelectImpossible also exists below. Why "planbuilder.PlanSelectImpossible" is present in two switch statement?

If the fields did not get cached, then we have to send the query to mysql. Added a comment.

go/vt/vttablet/tabletserver/query_executor.go

harshit-gangal · 2020-03-20T13:28:12Z

go/vt/vttablet/tabletserver/query_executor.go

+	if qre.tsv.qe.enableConsolidator || (qre.tsv.qe.enableConsolidatorReplicas && qre.tabletType != topodatapb.TabletType_MASTER) {
 		q, original := qre.tsv.qe.consolidator.Create(string(sqlWithoutComments))


consolidator is default to true and hence test does not go in else part. Can a test be added?

This test requires creating a delay in query execution, which the test framework can't emulate. So, it's covered in the go/vt/vttablet/endtoend/misc_test.go.

harshit-gangal · 2020-03-20T13:51:42Z

go/vt/vttablet/tabletserver/planbuilder/plan.go

 	case *sqlparser.DDL:
-		plan, err = analyzeDDL(stmt, tables), nil
+		plan = &Plan{PlanID: PlanDDL}


This does not contain the DDL Query. Could you explain the reason for not adding the query?

It's because the plan can only store ParsedQuery, but the parsed version of a DDL is incomplete. So, we use the original query at the time of execution. We could add a special-case field for DDLs, but it didn't feel worth it. I've added a comment.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

harshit-gangal

LGTM

sougou added 17 commits March 16, 2020 20:49

tabletserver: remove TxPoolController abstraction

481ff93

It was not necessary, and was obfuscating the fact that it's wrapping TxEngine. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

tabletserver: simplify some code

8647051

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deprecation: deprecate DML_PK

cbd81a3

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deprecation: implement DMLLimit

585fa2a

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deprecation: delete lot of SBR code

4ee0b11

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deprecation: refactor code for readability

11e4978

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deprecation: support direct autocommit

f5a5dc5

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deprecation: tests for planbuilder

0794095

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deprecation: improve DDL pass-through

c6af114

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deprecation: tabletserver tests WIP

a4e107d

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deprecation: tabletserver unit tests pass

e5c87fc

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deprecation: no need to check binlog format

a1e533d

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deprecation: fix other tests under tabletserver

d5a97a3

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deprecation: fix tabletserver endtoend tests

8e728d4

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deprecation: fix other e2e tests

e69b85c

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deprecation: rollback on limit failure

ebd6524

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou requested review from harshit-gangal and systay March 20, 2020 06:13

systay reviewed Mar 20, 2020

View reviewed changes

go/vt/vttablet/tabletserver/planbuilder/plan.go Show resolved Hide resolved

systay reviewed Mar 20, 2020

View reviewed changes

systay approved these changes Mar 20, 2020

View reviewed changes

harshit-gangal reviewed Mar 20, 2020

View reviewed changes

deprecation: address review comments

ac2567f

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

harshit-gangal approved these changes Mar 20, 2020

View reviewed changes

harshit-gangal merged commit 7a5af87 into vitessio:master Mar 20, 2020

sougou added the Breaking change label Mar 20, 2020

sougou deleted the ss-deprecation branch March 20, 2020 20:59

ajm188 mentioned this pull request Jun 24, 2020

6.0 Slack vitess release merge (w/consul 1.5.1, no k3s) tinyspeck/vitess#163

Merged

		if err := qre.verifyRowCount(int64(len(qr.Rows)), maxrows); err != nil {
		return nil, err

		if qre.tsv.qe.enableConsolidator \|\| (qre.tsv.qe.enableConsolidatorReplicas && qre.tabletType != topodatapb.TabletType_MASTER) {
		q, original := qre.tsv.qe.consolidator.Create(string(sqlWithoutComments))

Conversation

sougou commented Mar 20, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

systay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harshit-gangal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants