Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @spaskob)

pkg/workload/schemachange/schemachange.go, line 274 at r1 (raw file):

}

func (w *schemaChangeWorker) runInTxn(tx *pgx.Tx) (string, bool, error) {

I have mixed feelings about all of the string inspection happening here. Have you trying pulling a pgconn.PgError out of the error returned from tx.Exec and then pulling its code out?

Addressed the feedback and fixed the TC failures are due the fact that the workload actually finds current issues and fails so this PR will have to be merged after I fix the long tail of these problems. They seem to be related to my recent changed of validation - certain validations for duplicates don't get triggered and we hit the assertion failures in ValidateTable but at least it;'s working as expected and is finding bugs!

Reviewable status: complete! 0 of 0 LGTMs obtained

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

pkg/cmd/roachtest/schemachange_random_load.go, line 48 at r4 (raw file):

	runCmd := []string{
		"./workload run",
		fmt.Sprintf("schemachange  --concurrency %d --max-ops %d --verbose=1", maxOps,

These arguments seem backwards

Also seems weird that this is just a single string.

pkg/workload/schemachange/schemachange.go, line 247 at r4 (raw file):

}

func (w *schemaChangeWorker) runInTxn(tx *pgx.Tx) (string, error) {

This rollback and error classification seems like it might be happening at the wrong level. It seems like any error needs to be dealt with for the whole transaction. It feels like this function should just return the error and then run should rollback on probably all of the errors but then decide whether to tolerate the error or not.

pkg/workload/schemachange/schemachange.go, line 272 at r4 (raw file):

}

func (w *schemaChangeWorker) runInTxn(tx *pgx.Tx) (string, bool, error) {

Can you name the bool return param?

pkg/workload/schemachange/schemachange.go, line 304 at r4 (raw file):

		if seriousErr := handleOpError(err); seriousErr != nil {
			if w.tolerateErrors {
				opsBuf.WriteString(fmt.Sprintf("Serious error: %v\n", seriousErr))

Don’t you still want to bail out of this transaction?

pkg/workload/schemachange/schemachange.go, line 308 at r4 (raw file):

				return opsBuf.String(), rollback, seriousErr
			}
		}

If rollback ever becomes true, do you really want to proceed to the next iteration of the loop? The next command is likely to fail now that you’re in an error state.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @spaskob)

pkg/cmd/roachtest/schemachange_random_load.go, line 48 at r4 (raw file):

good catch - made it more maintainable

pkg/workload/schemachange/schemachange.go, line 247 at r4 (raw file):

well I tried alternatives before coming up with this and also considered what you are suggesting - the trouble is we are still not clear on what errors we want to catch exactly and until we have some clarity as a team on what ultimately should be the purpose of this tool, I prefer to leave it as it is and not sink too much cost upfront. A few open questions:

• do we want to abort txn on first failure or continue to understand failure scenarios under cascaded errors?
• what kind of errors do we want to generate on purpose
• how to actually find out if something broke - currently we don't and we need some kind of database internals consistency checks after each txn commits or is rolled back

So there is too much uncertainty about this tool so I'd rather punt on the issue your highlighting for now.

pkg/workload/schemachange/schemachange.go, line 272 at r4 (raw file):

What do you mean? Use a named return value? I thought that was discouraged unless it's really necessary or you are asking for a comment for the function?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @spaskob)

pkg/workload/schemachange/schemachange.go, line 247 at r4 (raw file):

do we want to abort txn on first failure or continue to understand failure scenarios under cascaded errors?

In SQL once you get an error, unless you ROLLBACK TO SAVEPOINT your transaction is toast and all future statements are invalid. Given this fact, I'm not sure the rest of your points make as much sense. If you want the higher level to be able to cope with the error better, we should wrap the surrounding context and pass it up the stack. Dealing with the error here makes the code hard to follow. It seems to me that this lower level should inform the high level of the relevant context of the error but should leave things like rollback decisions to the higher level based on the context communicated from the lower level.

Maybe in the future we introduce savepoints in the run call, I think that's probably an interesting space for bugs.

pkg/workload/schemachange/schemachange.go, line 272 at r4 (raw file):

It's encouraged to use the named return values, it makes reading signatures easier. It's highly discouraged to do naked returns (which are enabled by using return values).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @spaskob)

pkg/workload/schemachange/schemachange.go, line 247 at r4 (raw file):

I agree about terminating the txn on first error - you are right. As for handling the errors in the caller, here is my point this function has 2 type of errors:

• ones that force terminating the whole workload:
  • unable to create a new op
  • having encountered a serious error (this one is debatable as we may still want to proceed, save the serious error and report to the user in the end)
• ones that just force rollback but the workload continues.

The caller will needs to distinguish b/w the two types and also create the log of operations which kinda depends on the guts of this function. So I am leaning towards rolling back in side this function if we have to and the caller just dealing with questions around should we terminate the whole workload.

WDYT?

Reviewed 1 of 2 files at r6, 1 of 2 files at r7.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @spaskob)

pkg/workload/schemachange/schemachange.go, line 247 at r4 (raw file):

👍

pkg/workload/schemachange/schemachange.go, line 293 at r5 (raw file):

			if _, err = tx.Exec(op); err != nil {
				histBin = "txnRbk"
				log.WriteString(fmt.Sprintf("***FAIL: %v\n", err))

Should this ***FAIL be so loud here? Don't we expect these pretty frequently? It is good to log the error, just seems to indicate that

Also, maybe it makes sense to append the ROLLBACK to the logs in run and similarly add COMMIT upon success there as well.

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

pkg/workload/schemachange/schemachange.go, line 293 at r5 (raw file):

This is at verbosity level 1 and above so only loud if you ask for it - I find it helpful for grepping and classifying the errors. At verbosity 0 only serious errors will be printed.
As for the sites where to ROLLBACK; I've tried both more than once, there's always pros and cons. I'd rather not touch it again.