workload: add querylog workload generator

[yuzefovich]

Adds a querylog workload generator that generates and executes the
queries based on the provided query log (according to the frequency
of the queries found in the log). It assumes that the corresponding
data has already been loaded and attempts to simulate the workload
that was captured by the log.

Release note: None

[cockroach-teamcity]

This change is 

[danhhz]

This seems similar in spirit to pkg/workload/querybench, could they be merged?

[yuzefovich]

Initially, we thought of embedding this querylog into the querybench, but once I started working on that, I realized that, in my opinion, they serve two different purposes - the former's goal is to generate queries on the fly and execute them against the cluster to simulate a customer's workload whereas the latter's goal is to run a set of predefined queries to measure the performance of the cluster. In theory, it should be relatively easy to encompass querybench into querylog, but I'd prefer keeping them separate.

[danhhz]

In theory, it should be relatively easy to encompass querybench into querylog, but I'd prefer keeping them separate.

Can you elaborate on why? I haven't had a chance to dig into the nuance between them, so I may very well be missing something, but if it's easy to have one of these that reads queries from a file and issues them, I definitely prefer that.

[yuzefovich]

Here is what we have at the moment:

• querybench takes in a single file which includes queries to be executed with no freedom of choice regarding the query parameters (i.e. there are no placeholders, queries are executed as is), the file might also contain comments for readability. All the queries are executed in turns in round-robin fashion (as far as I understand). The goal here is measuring performance between different releases.
• querylog takes in a bunch of query log files (that were created due to COCKROACH AUDIT set on the cluster), parses the files to generate a distribution of queries (i.e. calculates the frequency of each query occurring in the log), and then issues randomly chosen - according to the distribution - queries with either sampled or randomly generated values for the placeholders. The goal here is simulating a workload that resembles the one that produced the provided query log in order to stress the cluster.

I said it was "relatively easy" to embed the former into the latter because querylog could support parsing a simple file format that querybench supports and could issue queries in round-robin fashion as well (that would be probably toggled by a flag; with some ifs in the code, but we'd just move the corresponding code from one file to another).

However, that file format is not "query log" per se, so at least the way I see it it no longer would be query-log based workload. We would be creating a bigger, less comprehensible code that attempts to address too many goals at once. I think that if we get other query logs from other users/customers, we will need to "teach" querylog how to parse that query log (at the moment it can correctly handle all trackernetwork queries, but there are a lot more that it can't), so having an additional parsing rules for querybench format files will make that even less maintainable, in my opinion.
(Note: when I say "parse the query log" I mean actual parsing of the text plus trying to figure out the columns/data types that placeholders correspond to.)

[danhhz]

Okay, that makes sense. This conversation makes me realize that long-term we should probably be thinking of a more generic/powerful input format (something like https://github.com/memsql/dbbench maybe) and then simple tooling to convert all these other query text file formats into that one format. But of course I'm not going to ask you to pick that off in this PR. Carry on, thanks for answering my questions

[jordanlewis]

- I am guessing this will need continued tweaking as we see more complicated and differing query files, but let's merge it and edit as necessary.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto, @danhhz, @jordanlewis, and @yuzefovich)

---

pkg/workload/querylog/querylog.go, line 110 at r1 (raw file):

			`the placeholders. Say it is 0.9, then with 0.1 probability the values will be generated randomly.`)
		g.flags.Int64Var(&g.seed, `seed`, 1, `Random number generator seed.`)
		g.flags.Int64Var(&g.stmtTimeoutSeconds, `statement-timeout`, 0, `Sets session's statement_timeout setting (in seconds).'`)

This would be a great variable to move to the main workload generator as every workload could probably use such a thing.

---

pkg/workload/querylog/querylog.go, line 236 at r1 (raw file):

		if err != nil {
			if w.config.verbose {
				log.Infof(ctx, "Encountered an error %s while deducing column names corresponding to the placeholders\n", err.Error())

nit: You don't need an \n in Infof messages, and same throughout.

[yuzefovich]

Yes, I agree. I believe it will be evolving constantly to support different kinds of queries.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto, @danhhz, and @jordanlewis)

---

pkg/workload/querylog/querylog.go, line 110 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

This would be a great variable to move to the main workload generator as every workload could probably use such a thing.

I attempted to do that, but it seems tricky - statement_timeout is a session variable, so it must be set on each connection, and it seems to me that with the current implementation of workload only workers (those in QueryLoad.WorkerFns) will always have access to that, so I currently don't see an easy way of moving this parameter to the main generator. Possibly I'm missing something though.

---

pkg/workload/querylog/querylog.go, line 236 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

nit: You don't need an \n in Infof messages, and same throughout.

Thanks, done.

[jordanlewis]

If it wasn't easy to fix the thing with the session timeout, don't worry about it - perhaps @danhhz could weigh in on a better way to do that. But I would love to see this integrated into all workloads in a future commit, as it would make it much easier to test client cancellation, which is a common real-world workload type.

[danhhz]

One of these days I want to move the workload run impl to be a set of composable pieces that generators can fit together to do what they want. Maybe it'd be possible in that world, but not now.

FYI, this isn't guaranteed to work as written. Session variables are basically impossible to use with go's sql abstraction because there's no hook to run something when the connection pool opens a new connection. If you want to be sure this flag works, either set it on the connection string, or use something like pgx and store a pgx.Conn on each worker.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto, @danhhz, and @jordanlewis)

[yuzefovich]

Thanks @danhhz. Could you take a quick look whether I'm setting up and using pgx.Conn correctly (it seems to be working)?

[danhhz]

IANAPgxExpert but seems fine mod one comment

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto, @jordanlewis, and @yuzefovich)

---

pkg/workload/querylog/querylog.go, line 196 at r2 (raw file):

	}

	connCfg, err := pgx.ParseConnectionString(strings.Join(urls, ` `))

Is joining with the right thing to do here? Pretty sure this works above because of https://github.com/cockroachdb/cockroach/blob/43bfafb30e90b6873bbbe78c9bc92dd7bc4e0ce7/pkg/workload/driver.go, which you'd have to replicate here. Could also leave it as a TODO for now and error if len(urls) != 1

---

pkg/workload/querylog/querylog.go, line 273 at r2 (raw file):

			continue
		}
		// TODO(yuzefovich): do we care about the returned rows?

Not 100% sure on this, but I think we stop incrementally evaluating the query if the results buffer fills up (and is not emptied). Either way, probably want to iterate the query results to make sure it's doing everything it normally would.

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto, @danhhz, and @jordanlewis)

---

pkg/workload/querylog/querylog.go, line 196 at r2 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

Is joining with the right thing to do here? Pretty sure this works above because of https://github.com/cockroachdb/cockroach/blob/43bfafb30e90b6873bbbe78c9bc92dd7bc4e0ce7/pkg/workload/driver.go, which you'd have to replicate here. Could also leave it as a TODO for now and error if len(urls) != 1

Good point, I was blindly following the pattern. Left a TODO.

---

pkg/workload/querylog/querylog.go, line 273 at r2 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

Not 100% sure on this, but I think we stop incrementally evaluating the query if the results buffer fills up (and is not emptied). Either way, probably want to iterate the query results to make sure it's doing everything it normally would.

Thanks, done.

[yuzefovich]

Thanks for the reviews and suggestions! I'll go ahead and merge this, and we'll iterate on it as necessary.

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)