sctest: Make DML Injection Testing Framework More Readable

[Xiang-Gu]

This commit tries to make the DML injection testing framework more readable and maintainable.
It mostly added comments and reorganized some code. No function is changed.

Epic: None
Release note: None

[cockroach-teamcity]

This change is 

[fqazi]

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Xiang-Gu)

---

pkg/sql/schemachanger/sctest/cumulative.go line 465 at r1 (raw file):

			_, err := runner.DB.ExecContext(context.Background(), boundSQL)
			if (e.expectedOutput == "" ||
				idx != len(e.stmts)-1) && err != nil {

Think we still need the checking that only the last statement is allowed to create an error.

---

pkg/sql/schemachanger/sctest/cumulative.go line 873 at r1 (raw file):

		_, db, cleanup := newCluster(t, &scexec.TestingKnobs{
			BeforeStage: func(p scplan.Plan, stageIdx int) error {
				if !clusterCreated {

Make this safer by using an atomic

[Xiang-Gu]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

---

pkg/sql/schemachanger/sctest/cumulative.go line 465 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Think we still need the checking that only the last statement is allowed to create an error.

This does not really make much sense to me. I understood what you want to do if there are >1 stmts to execute in one DML injection block:

1. Run each one in its own transaction ("runner.DB.ExecContext")
2. If it's not the last stmt, and it encounters an error, we then either fail the test (if !rewrite) or record the error (if rewrite)
3. If it's the last stmt, and it encounters an error, we then either fail the test (if !rewrite) or record the error (if rewrite)

I expect the user (who is writing those DML injection blocks in the test file) to have only 1 stmt in a DML injection block if they expect the that DML returns an error.

Such a restriction is also somewhat artificial. Instead, I think we should execute all the stmts specified in one block at once in one transaction (i.e., treat them as one line with multiple stmts separated by semi-colons). That will require slightly more changes to the code (e.g. stageExecStmt.stmts []string will change to stageExecStmt.stmt string and it will be the literal string specified in the block, strip the "\n", and join them with ";"). I am not keen to make this change in this PR though.

---

pkg/sql/schemachanger/sctest/cumulative.go line 873 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Make this safer by using an atomic

done

[Xiang-Gu]

TFTR!

bors r+

[fqazi]

This does not really make much sense to me. I understood what you want to do if there are >1 stmts to execute in one DML injection block:

1. Run each one in its own transaction ("runner.DB.ExecContext")
2. If it's not the last stmt, and it encounters an error, we then either fail the test (if !rewrite) or record the error (if rewrite)
3. If it's the last stmt, and it encounters an error, we then either fail the test (if !rewrite) or record the error (if rewrite)

I expect the user (who is writing those DML injection blocks in the test file) to have only 1 stmt in a DML injection block if they expect the that DML returns an error.

Such a restriction is also somewhat artificial. Instead, I think we should execute all the stmts specified in one block at once in one transaction (i.e., treat them as one line with multiple stmts separated by semi-colons). That will require slightly more changes to the code (e.g. stageExecStmt.stmts []string will change to stageExecStmt.stmt string and it will be the literal string specified in the block, strip the "\n", and join them with ";"). I am not keen to make this change in this PR though.

Thats the existing behaviour here, so I'm afraid we are going to have regressions that will get missed (i.e. if the first insert returns an error vs the second):

add_column_default_unique:stage-exec phase=PostCommitPhase stage=5:14 schemaChangeExecError=(.*duplicate key value violates unique constraint.*)
add_column_default_unique-INSERT INTO db.public.tbl VALUES($stageKey);
add_column_default_unique-INSERT INTO db.public.tbl VALUES($stageKey + 1);

Longer term, this should be one transaction, but we also will need to probably improve the error keys. We need to know the exact failure points in cases like this.

[Xiang-Gu]

bors r-

[craig]

Canceled.

[Xiang-Gu]

bors r+

[Xiang-Gu]

bors r-

[craig]

Canceled.

[fqazi]

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained

[Xiang-Gu]

TFTR!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)