tests: accelerate tests whose latency is sensitive to rangefeeds #111753

knz · 2023-10-04T13:24:53Z

For context, the latency of some unit tests is indirectly largely influenced by the closed timestamp target duration and other rangefeed-related configuration knobs; while not having specific requirements about them.

Prior to this patch, a subset of these tests were trying to decrease their overall run time by tuning some of these knobs. There were two problems with this approach: this was a lot of boilerplate; and the test code was not consistent in updating all the relevant tuning knobs.

This patch fixes it by introducing a new FastRangefeed parameter on TestServerArgs which tunes all the configuration knobs to sensible lower values.

Release note: None
Epic: CRDB-28893

For context, the latency of some unit tests is indirectly largely influenced by the closed timestamp target duration and other rangefeed-related configuration knobs; while not having specific requirements about them. Prior to this patch, a subset of these tests were trying to decrease their overall run time by tuning some of these knobs. There were two problems with this approach: this was a lot of boilerplate; and the test code was not consistent in updating _all_ the relevant tuning knobs. This patch fixes it by introducing a new `FastRangefeed` parameter on TestServerArgs which tunes all the configuration knobs to sensible lower values. Release note: None

cockroach-teamcity · 2023-10-04T13:25:10Z

This change is

erikgrinaker · 2023-10-04T13:28:14Z

pkg/base/test_server_args.go

+	// FastRangefeeds, if set, lowers the latency at which rangefeeds
+	// report changes.


nit: changes are always emitted ~immediately, this knob only affects checkpoint lag and frequency.

erikgrinaker · 2023-10-04T13:31:28Z

pkg/server/testserver.go

+			{"kv.protectedts.poll_interval", 10 * time.Millisecond},
+			{"kv.protectedts.reconciliation.interval", 1 * time.Millisecond},
+			// Speed up span config reconciliation.
+			{"spanconfig.reconciliation_job.checkpoint_interval", 100 * time.Millisecond},


These settings are unrelated to rangefeeds, they are for other mechanisms. We can include them here, but then the parameter should be called something else, and probably also adjust other settings like the replica scanner interval (which determines queue latency).

miretskiy · 2023-10-04T13:38:03Z

pkg/server/testserver.go

@@ -691,6 +691,37 @@ func (ts *testServer) Activate(ctx context.Context) error {
 		return err
 	}

+	// If the test is indirectly dependent on rangefeed latency, but not
+	// particularly interested in observing the effects of the default
+	// rangefeed configuration, make the test faster.


In one way or another, every test uses rangefeeds. I wonder what the impact of these settings on some of the unrelated tests (not touched by this PR). How much of a startup speed this brings (if any)? If there is impact, perhaps, this is just something that should be done unconditionally?

stevendanna

I like the direction of this. I defer to erik and yev on the individual settings and their values. But it certainly will be nice to not have to cargo-cult these around.

111403: sql: add new check relation to help build insert fast path uniqueness checks r=msirek a=msirek sql: add new check relation to help build insert fast path uniqueness checks This creates a `FastPathUniqueChecks` list as a child of `InsertExpr`, similar to `UniqueChecks`, but consisting of relational expressions which can be used to build a fast path uniqueness check using a particular index. Each item in the `FastPathUniqueChecks` list is a `FastPathUniqueChecksItem` whose `Check` expression, if the unique constraint is provisionally eligible to use a fast path uniqueness check, is a filtered scan from the target table of an `INSERT INTO ... VALUES` statement. It is only built if the VALUES clause has a single row. The filter equates each of the unique constraint key columns with its corresponding value from the VALUES clause. The values may either be a `ValuesExpr` or a `WithScanExpr` whose source is a `ValuesExpr`. During exploration, GenerateConstrainedScans will find all valid constrained index scans which are equivalent to the original filtered Scan. The built relations are not executed or displayed in the EXPLAIN, but will be analyzed in a later commit to specify the index and index key values to use for insert fast path uniqueness checks. FastPathUniqueChecksItemPrivate is added with elements which are designed to communicate details of how to build a fast path uniqueness check to the execbuilder. Epic: CRDB-26290 Informs: #58047 Release note: None ---- memo: optbuild unit tests for fast path unique check exprs This commit adds the `ExprFmtHideFastPathChecks` flag, which hides fast path check expressions from the output of expression formatting. It is used in all places except TestBuilder. Also, unit tests for `buildFiltersForFastPathCheck` are added. Epic: CRDB-26290 Informs: #58047 Release note: None ---- xform: opt test for insert fast path checks This commit adds an `insert` transformation rules test which displays optimized fast path checks which are built in the optbuilder and later explored and optimized. This test is modified in a later commit to test for rule firing. Epic: CRDB-26290 Informs: #58047 Release note: None 111443: ccl: regionless restores with rbr table(s) fail fast, add version gate r=msbutler a=annrpom ### ccl: regionless restores with regional by row table(s) fail fast Previously, backing up an MR cluster/db/table that had a regional by row table did not work out-of-the-box (users could not write to said table without performing some operations to ensure hidden column added by regional by row enablement, `crdb_region`, and zone config behaved according to the target RESTORE cluster. This was inadequate because we allowed users to perform a RESTORE where some tables were not "available" right away. This patch addresses this by making sure we "fail fast" if users do decide to attempt a RESTORE where the BACKUP object has a regional by row table. Also: tests. A small amount of tests were fixed to separate the behavior of restoring a database vs a table (just dropping a table after we performed a db restore to test table restore is not a properly isolated test of behavior). Mixed-version tests were added as well. Epic: none Informs: #111348 Part of: https://cockroachlabs.atlassian.net/browse/CRDB-29129 Release note (sql change): Users are not able to perform a `remove_regions` RESTORE if the object being restored contains a regional by row table. --- ### ccl: add 23.2 version gate to regionless restore This patch adds a >= 23.2 version gate to the `remove_regions` RESTORE option, in addition to a mixed-version test to ensure that RESTORE fails fast if it is on a cluster version < 23.2. Epic: none Fixes: #111348 Part of: https://cockroachlabs.atlassian.net/browse/CRDB-29129 Release note: none 111758: server: reduce a flake under race r=stevendanna a=knz Informs #111742. Will be superseded by #111753, but this one needs to be backported (the flake is on the release branch). Epic: CRDB-28893 Release note: None 111761: sql: add select for update in udf tests r=rharding6373 a=rharding6373 This PR exercises simple SELECT FOR UPDATE patterns in UDFs, and also tests UDFs in transactions that are committed or rolled back. Epic: CRDB-25388 Informs: #87289 Release note: None Co-authored-by: Mark Sirek <[email protected]> Co-authored-by: Annie Pompa <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: rharding6373 <[email protected]>

yuzefovich

Reviewed 36 of 36 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @miretskiy, @renatolabs, and @smg260)

pkg/server/testserver.go line 696 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

In one way or another, every test uses rangefeeds. I wonder what the impact of these settings on some of the unrelated tests (not touched by this PR). How much of a startup speed this brings (if any)? If there is impact, perhaps, this is just something that should be done unconditionally?

I have a similar question: shouldn't we default to always have "fast rangefeeds" and then only tests that care about "real life" rangefeeds would specify so via SlowRangefeeds testing knobs? What would be the negative consequences of going this route?

erikgrinaker

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @miretskiy, @renatolabs, and @smg260)

pkg/server/testserver.go line 696 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I have a similar question: shouldn't we default to always have "fast rangefeeds" and then only tests that care about "real life" rangefeeds would specify so via SlowRangefeeds testing knobs? What would be the negative consequences of going this route?

I think the main downside would be that it could hide race conditions, e.g. with span configs, if these are always applied ~immediately.

I suppose deadlock/race builds might still slow them down enough to reveal this though. And many tests already speed this up anyway, so we already have this problem to some extent.

knz · 2023-10-05T14:03:56Z

Ok so when I have a slow(er) day I'll try two versions of this PR: one with opt-in and one with opt-out, and see what CI thinks about both. I'll probably prefer to go towards the one that requires modifying fewer tests.

knz requested review from erikgrinaker, stevendanna and yuzefovich October 4, 2023 13:24

knz requested review from a team as code owners October 4, 2023 13:24

knz requested a review from a team October 4, 2023 13:24

knz requested review from a team as code owners October 4, 2023 13:24

knz requested a review from a team October 4, 2023 13:24

knz requested review from a team as code owners October 4, 2023 13:24

knz requested review from smg260 and renatolabs and removed request for a team October 4, 2023 13:24

erikgrinaker reviewed Oct 4, 2023

View reviewed changes

miretskiy reviewed Oct 4, 2023

View reviewed changes

stevendanna approved these changes Oct 4, 2023

View reviewed changes

knz mentioned this pull request Oct 4, 2023

server: reduce a flake under race #111758

Merged

yuzefovich reviewed Oct 5, 2023

View reviewed changes

erikgrinaker reviewed Oct 5, 2023

View reviewed changes

stevendanna self-assigned this Oct 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: accelerate tests whose latency is sensitive to rangefeeds #111753

tests: accelerate tests whose latency is sensitive to rangefeeds #111753

knz commented Oct 4, 2023

cockroach-teamcity commented Oct 4, 2023

erikgrinaker Oct 4, 2023

erikgrinaker Oct 4, 2023 •

edited

Loading

miretskiy Oct 4, 2023

stevendanna left a comment

yuzefovich left a comment

erikgrinaker left a comment

knz commented Oct 5, 2023

		// FastRangefeeds, if set, lowers the latency at which rangefeeds
		// report changes.

tests: accelerate tests whose latency is sensitive to rangefeeds #111753

Are you sure you want to change the base?

tests: accelerate tests whose latency is sensitive to rangefeeds #111753

Conversation

knz commented Oct 4, 2023

cockroach-teamcity commented Oct 4, 2023

erikgrinaker Oct 4, 2023

Choose a reason for hiding this comment

erikgrinaker Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

miretskiy Oct 4, 2023

Choose a reason for hiding this comment

stevendanna left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

erikgrinaker left a comment

Choose a reason for hiding this comment

knz commented Oct 5, 2023

erikgrinaker Oct 4, 2023 •

edited

Loading