roachtest: compile cockroach-short with --crdb_test and use in sqlsmith #86625

yuzefovich · 2022-08-23T00:40:09Z

This commit adds another step to the nightly roachtest invocation to
compile cockroach-short binary with --crdb_test build tag. Then it
also adds plumbing throughout the roachtest infrastructure to expose
that newly-compiled binary which is now used in 50% cases in the sqlsmith
roachtest.

Fixes: #83186.

Release justification: testing-only change.

Release note: None

cockroach-teamcity · 2022-08-23T00:40:16Z

This change is

yuzefovich · 2022-08-23T00:41:11Z

I'm not quite sure how to test the whole pipeline as well as whether this is the right approach, so I'm open to any suggestions.

srosenberg

Looks good modulo the naming asymmetry. Thanks for sending this PR! I am excited to see what else we might find by running other tests with assertions enabled.

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @renatolabs, @rickystewart, and @yuzefovich)

build/teamcity/cockroach/nightlies/roachtest_compile_bits.sh line 8 at r1 (raw file):

      //pkg/cmd/cockroach //pkg/cmd/workload //pkg/cmd/roachtest \
      //c-deps:libgeos
bazel build --config crosslinux --config ci -c opt //pkg/cmd/cockroach-short --crdb_test

Could we maybe call it something else to denote that this type of binary is compiled with runtime assertions? E.g., cockroach-short-ea (ea for "enable assertions"); cockroach-short-crdb_test seems somewhat redundant :)

Also, it would be nice to keep the symmetry between cockroach (cockroach-ea) and cockroach-short (cockroach-short-ea). At some point, we might want to use cockroach-short without assertions, for some of the tests. (None of the tests explicitly use the Console; I think it's mainly a convenience feature in case you'd like to attach to it while a long-running tests is executing, but I am not entirely sure.) @tbg is our anthropologist, so I'll let him chime in :)

yuzefovich

TFTRs!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @renatolabs, @rickystewart, and @tbg)

build/teamcity/cockroach/nightlies/roachtest_compile_bits.sh line 8 at r1 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

Could we maybe call it something else to denote that this type of binary is compiled with runtime assertions? E.g., cockroach-short-ea (ea for "enable assertions"); cockroach-short-crdb_test seems somewhat redundant :)

Also, it would be nice to keep the symmetry between cockroach (cockroach-ea) and cockroach-short (cockroach-short-ea). At some point, we might want to use cockroach-short without assertions, for some of the tests. (None of the tests explicitly use the Console; I think it's mainly a convenience feature in case you'd like to attach to it while a long-running tests is executing, but I am not entirely sure.) @tbg is our anthropologist, so I'll let him chime in :)

Renamed. I didn't find a way to do that by asking the bazel directly to store the compiled binary at a different name - is there a way? Maybe @rickystewart knows.

renatolabs

This looks good! A suggestion: could move the code you added to sqlsmith.go to a place where it can be reused by other tests, e.g.:

c.MaybeUseTestBuild(t, 0.5)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @renatolabs, @rickystewart, @srosenberg, @tbg, and @yuzefovich)

pkg/cmd/roachtest/tests/sqlsmith.go line 102 at r1 (raw file):

			c.Put(ctx, t.Cockroach(), "./cockroach")
		} else {
			t.Status("using cockroach-short binary compiled with --crdb_test build tag")

Could we check if a cockroach-short binary is present as well? When roachtest is called without the cockroach-short option, this message will be misleading, as we'll be using the standard cockroach binary. We could let cockroachShortbe empty if the flag is not passed and check for its value here.

rickystewart · 2022-08-23T15:27:49Z

I didn't find a way to do that by asking the bazel directly to store the compiled binary at a different name - is there a way? Maybe @rickystewart knows.

Nah, the binary name is determined by the BUILD files, you can't adjust it on the command line. As you copy it out of the sandbox, rename it to whatever you want at that point.

This commit adds another step to the nightly roachtest invocation to compile `cockroach-short` binary with `--crdb_test` build tag. Then it also adds plumbing throughout the roachtest infrastructure to expose that newly-compiled binary which is now used in 50% cases in the sqlsmith roachtest. Release justification: testing-only change. Release note: None

yuzefovich

Extracted a helper for reuse.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @renatolabs, @rickystewart, @srosenberg, and @tbg)

pkg/cmd/roachtest/tests/sqlsmith.go line 102 at r1 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Could we check if a cockroach-short binary is present as well? When roachtest is called without the cockroach-short option, this message will be misleading, as we'll be using the standard cockroach binary. We could let cockroachShortbe empty if the flag is not passed and check for its value here.

Good point, done.

renatolabs

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @renatolabs, @rickystewart, @srosenberg, and @tbg)

yuzefovich · 2022-08-23T16:34:21Z

TFTRs!

bors r+

craig · 2022-08-23T17:28:51Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-23T18:34:09Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-23T22:13:46Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-23T23:45:05Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-24T00:09:43Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-24T03:32:02Z

Build failed:

Bazel Essential CI (Cockroach)

yuzefovich · 2022-08-24T22:59:51Z

bors r+

craig · 2022-08-25T02:42:38Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-25T05:57:16Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-25T10:09:59Z

Build succeeded:

Bazel Essential CI (Cockroach)

Fixes a regression introduced by cockroachdb#86625. Release justification: fix roachtest nightly (on master) Release note: None

86834: sql/opt: add session setting to disable stats forecast use in optimizer r=rytaft a=michae2 Add a new session setting, `optimizer_use_forecasts`, which can be used to disable forecast usage in the optimizer. Forecasts will still be generated in the stats cache (this will be controlled by a different setting). Assists: #86350 Release justification: Low-risk update to new functionality. Release note (sql change): This commit adds a new session setting, `optimizer_use_forecasts`, which can be set to false to disable usage of statistics forecasts when optimizing a query. 86920: roachprod{,-stress}: let roachprod-stress use roachprod library r=srosenberg a=healthy-pod This patch migrates roachprod-stress from using the roachprod binary to using the roachprod library. Release justification: Non-production code changes Release note: None 86954: util/mon: remove nameWithPointer to reduce allocations r=yuzefovich a=yuzefovich This field was added in order to help us track down some of the memory leaks which we have already found, and the field didn't turn out to be that useful. When it was introduced, the implications on the increase in allocations were unknown, and now I don't think the field is worth it. ``` name old time/op new time/op delta FlowSetup/vectorize=true/distribute=true-24 168µs ± 5% 164µs ± 4% -2.33% (p=0.007 n=19+20) FlowSetup/vectorize=true/distribute=false-24 167µs ± 6% 164µs ± 6% ~ (p=0.060 n=20+20) FlowSetup/vectorize=false/distribute=true-24 163µs ± 4% 161µs ± 7% ~ (p=0.057 n=19+20) FlowSetup/vectorize=false/distribute=false-24 161µs ± 6% 159µs ± 5% ~ (p=0.309 n=19+20) name old alloc/op new alloc/op delta FlowSetup/vectorize=true/distribute=true-24 19.6kB ± 8% 19.0kB ± 8% -2.62% (p=0.001 n=19+18) FlowSetup/vectorize=true/distribute=false-24 18.2kB ± 1% 17.7kB ± 1% -2.56% (p=0.000 n=17+16) FlowSetup/vectorize=false/distribute=true-24 25.8kB ± 2% 25.4kB ± 0% -1.44% (p=0.000 n=16+16) FlowSetup/vectorize=false/distribute=false-24 24.7kB ± 0% 24.4kB ± 1% -1.36% (p=0.000 n=16+16) name old allocs/op new allocs/op delta FlowSetup/vectorize=true/distribute=true-24 218 ± 2% 205 ± 3% -5.64% (p=0.000 n=19+19) FlowSetup/vectorize=true/distribute=false-24 208 ± 1% 197 ± 3% -5.63% (p=0.000 n=19+19) FlowSetup/vectorize=false/distribute=true-24 206 ± 0% 197 ± 0% -4.40% (p=0.000 n=16+16) FlowSetup/vectorize=false/distribute=false-24 197 ± 0% 188 ± 0% -4.54% (p=0.000 n=16+16) ``` Release justification: low-risk cleanup. Release note: None 86956: roachtest: fix unintended exit when --cockroach-short is passed r=srosenberg a=srosenberg Fixes a regression introduced by #86625. Release justification: fix roachtest nightly (on master) Release note: None Co-authored-by: Michael Erickson <[email protected]> Co-authored-by: healthy-pod <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Stan Rosenberg <[email protected]>

yuzefovich requested review from rickystewart and a team August 23, 2022 00:40

yuzefovich requested review from a team as code owners August 23, 2022 00:40

yuzefovich requested review from smg260 and removed request for a team August 23, 2022 00:40

srosenberg requested review from renatolabs and removed request for smg260 August 23, 2022 00:51

srosenberg approved these changes Aug 23, 2022

View reviewed changes

rickystewart approved these changes Aug 23, 2022

View reviewed changes

yuzefovich force-pushed the sqlsmith-crdb-test branch 2 times, most recently from 5796d7d to 1628a15 Compare August 23, 2022 14:43

yuzefovich commented Aug 23, 2022

View reviewed changes

renatolabs reviewed Aug 23, 2022

View reviewed changes

yuzefovich force-pushed the sqlsmith-crdb-test branch from 1628a15 to 287d511 Compare August 23, 2022 15:37

yuzefovich force-pushed the sqlsmith-crdb-test branch from 287d511 to 9abc884 Compare August 23, 2022 15:39

yuzefovich commented Aug 23, 2022

View reviewed changes

renatolabs approved these changes Aug 23, 2022

View reviewed changes

srosenberg mentioned this pull request Aug 23, 2022

enable running non-perf roachtests with runtime assertions (crdb_test build tag) #86678

Closed

craig bot merged commit eaa7eaf into cockroachdb:master Aug 25, 2022

yuzefovich deleted the sqlsmith-crdb-test branch August 25, 2022 14:07

srosenberg mentioned this pull request Aug 26, 2022

roachtest: fix unintended exit when --cockroach-short is passed #86956

Merged

yuzefovich mentioned this pull request Aug 28, 2022

roachtest: inbox received batch larger than max batch size #86982

Closed

rytaft mentioned this pull request Aug 29, 2022

opt: unoptimized-query-oracle/disable-rules=half failed due to timeout #86308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachtest: compile cockroach-short with --crdb_test and use in sqlsmith #86625

roachtest: compile cockroach-short with --crdb_test and use in sqlsmith #86625

yuzefovich commented Aug 23, 2022

cockroach-teamcity commented Aug 23, 2022

yuzefovich commented Aug 23, 2022

srosenberg left a comment

yuzefovich left a comment

renatolabs left a comment

rickystewart commented Aug 23, 2022

yuzefovich left a comment

renatolabs left a comment

yuzefovich commented Aug 23, 2022

craig bot commented Aug 23, 2022

craig bot commented Aug 23, 2022

craig bot commented Aug 23, 2022

craig bot commented Aug 23, 2022

craig bot commented Aug 24, 2022

craig bot commented Aug 24, 2022

yuzefovich commented Aug 24, 2022

craig bot commented Aug 25, 2022

craig bot commented Aug 25, 2022

craig bot commented Aug 25, 2022

roachtest: compile cockroach-short with --crdb_test and use in sqlsmith #86625

roachtest: compile cockroach-short with --crdb_test and use in sqlsmith #86625

Conversation

yuzefovich commented Aug 23, 2022

cockroach-teamcity commented Aug 23, 2022

yuzefovich commented Aug 23, 2022

srosenberg left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

renatolabs left a comment

Choose a reason for hiding this comment

rickystewart commented Aug 23, 2022

yuzefovich left a comment

Choose a reason for hiding this comment

renatolabs left a comment

Choose a reason for hiding this comment

yuzefovich commented Aug 23, 2022

craig bot commented Aug 23, 2022

craig bot commented Aug 23, 2022

craig bot commented Aug 23, 2022

craig bot commented Aug 23, 2022

craig bot commented Aug 24, 2022

craig bot commented Aug 24, 2022

yuzefovich commented Aug 24, 2022

craig bot commented Aug 25, 2022

craig bot commented Aug 25, 2022

craig bot commented Aug 25, 2022