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

pkg/bench/foreachdb.go line 101 at r1 (raw file):

	_, tenantDB := serverutils.StartTenant(b, s, base.TestTenantArgs{
		TenantName: "benchtenant",
		TenantID:   roachpb.MustMakeTenantID(10),

I initially used UseDatabase option with "cluster:"+tenantName+"/bench" value, but this made BenchmarkPgbench fail. Removing UseDatabase seems to work for all benchmarks.

pkg/bench/foreachdb.go line 105 at r1 (raw file):

	// The benchmarks sometime hit the default span limit, so we increase it.
	// NOTE(andrei): Benchmarks drop the tables they're creating, so I'm not sure

I haven't tried running the benchmarks without this, so I dunno whether it is still relevant.

pkg/bench/bench_test.go line 1195 at r1 (raw file):

	defer log.Scope(b).Close(b)
	ForEachDB(b, func(b *testing.B, db *sqlutils.SQLRunner) {
		db.Exec(b, `CREATE TABLE bench.abc (a INT PRIMARY KEY, b INT, c INT, INDEX(b), UNIQUE INDEX(c))`)

I don't understand why the in-memory tenant cockroach benchmarks didn't fail for the cases where bench. prefix was missing - perhaps it's due to explicitly specifying the DB name for OpenDBConn? If I did use similar value when starting a separate-process tenant via UseDatabase parameter, the benchmarks missing bench. prefix still failed 🤷‍♂️

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

pkg/bench/bench_test.go line 1195 at r1 (raw file):

perhaps it's due to explicitly specifying the DB name for OpenDBConn?

yes that would be my understanding as well.

If I did use similar value when starting a separate-process tenant via UseDatabase parameter, the benchmarks missing bench. prefix still failed 🤷‍♂️

I'm not sure I understand the operation mode. How do you "start a separate-process tenant with a UseDatabase parameter"?

pkg/bench/bench_test.go line 1198 at r2 (raw file):

	defer log.Scope(b).Close(b)
	ForEachDB(b, func(b *testing.B, db *sqlutils.SQLRunner) {
		db.Exec(b, `CREATE TABLE bench.abc (a INT PRIMARY KEY, b INT, c INT, INDEX(b), UNIQUE INDEX(c))`)

I'm not sure introducing this bench. prefix everywhere is the right way to go. It makes the SQL longer and (slightly) harder to read and update.

If you can't insert the db name in the connection string, you can execute a USE or SET database statement at the beginning of the benchmark.

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

pkg/bench/bench_test.go line 1195 at r1 (raw file):

I specified base.TestTenantArgs.UseDatabase argument, but only I included "cluster:"+tenantName+"/bench" in there. The correct way is to just use bench there which is what this commit does. This is probably the way to go. (But it shows that I'm a complete newbie with all of this :) )

pkg/bench/bench_test.go line 1198 at r2 (raw file):

Fixed.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

pkg/bench/bench_test.go line 1195 at r1 (raw file):

To be honest the idea that TestTenantArgs has a "UseDatabase" field is utterly confusing to me. The database is a property of the client connection, not of the tenant! And the way this particular field is plumbed through obscures that fact.

But yes what you did here is correct.