roachtest: adjust tpchvec and tpcdsvec#49966
Conversation
|
This change is |
asubiotto
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 1 of 1 files at r3.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/cmd/roachtest/tpchvec.go, line 171 at r1 (raw file):
// Since this is a performance test, each query should be run with both // vectorize modes. return []bool{false, true}
As usual with these changes, could you do a quick local run to make sure nothing breaks?
pkg/cmd/roachtest/tpchvec.go, line 425 at r2 (raw file):
testCase tpchVecTestCase, testRun func(ctx context.Context, t *test, c *cluster, version crdbVersion, tc tpchVecTestCase), disableStatsCreation bool,
Can you make the stats creation part of the base test and override that in tpchVecPerfTest instead of introducing a flag to this general function? It seems wrong to introduce an argument for all test cases when it is only used for one.
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/tpchvec.go, line 171 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
As usual with these changes, could you do a quick local run to make sure nothing breaks?
Sure, kicked off the run of old perf and new perf_no_stats configs.
pkg/cmd/roachtest/tpchvec.go, line 425 at r2 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Can you make the stats creation part of the base test and override that in
tpchVecPerfTestinstead of introducing a flag to this general function? It seems wrong to introduce an argument for all test cases when it is only used for one.
Done.
However, we can't easily move the stats creation into the base case because tpchVecPerfTest reuses that logic. I looked into whether we could create stats in the base and then delete them (when stats creation is disabled), but our docs say that you need to restart the nodes for the statistics cache to be cleared. I think it's ok that every test config will be determining whether it needs the stats or not.
This commit adds a new `tpchvec/perf_no_stats` config that is the same as `tpchvec/perf` except for the fact that stats creation is disabled. The plans without stats are likely to be different, so it gives us an easy way to get more test coverage. One caveat here is that query 9 without stats takes insanely long to run, so some new plumbing has been added to skip that query. Additionally, `tpcdsvec` has been adjusted. The test runs all queries with and without stats present with `on` and `off` vectorize options. However, when stats are not present, `on` config will be reduced to `off` because of `vectorize_row_count_threshold` heuristic. This commit disables that heuristic. Release note: None
Let's see whether it makes difference to occasional failures of `tpchvec/perf` which are very hard to explain. This commit also changes the workload command for `perf` config to run only against node 1, thus, eliminating one possible source of "randomness" for the failures. Release note: None
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/tpchvec.go, line 171 at r1 (raw file):
Previously, yuzefovich wrote…
Sure, kicked off the run of old
perfand newperf_no_statsconfigs.
I added a skip of query 9 for "no stats" case (because it was taking forever), and then rerun both perf and perf_no_stats configs, and the tests passed.
asubiotto
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, 1 of 1 files at r5.
Reviewable status:
|
TFTR! bors r+ |
Build succeeded |
roachtest: add new tpchvec config
This commit adds a new
tpchvec/perf_no_statsconfig that is the sameas
tpchvec/perfexcept for the fact that stats creation is disabled.The plans without stats are likely to be different, so it gives us an
easy way to get more test coverage. One caveat here is that query
9 without stats takes insanely long to run, so some new plumbing has
been added to skip that query.
Additionally,
tpcdsvechas been adjusted. The test runs all querieswith and without stats present with
onandoffvectorize options.However, when stats are not present,
onconfig will be reduced tooffbecause ofvectorize_row_count_thresholdheuristic. This commitdisables that heuristic.
Release note: None
roachtest: switch the config order in tpchvec/perf
Let's see whether it makes difference to occasional failures of
tpchvec/perfwhich are very hard to explain.This commit also changes the workload command for
perfconfig to runonly against node 1, thus, eliminating one possible source of
"randomness" for the failures.
Addresses: #49955.
Release note: None