opt: Fix and improve zone-based index selection #36689

andy-kimball · 2019-04-09T22:41:11Z

This PR fixes a couple of related issues with zone-based index selection.

First, placeholder zones were incorrectly discarded. Placeholder zones are
used when there are indexes with Constraints/Leaseholder Preferences on a
table that doesn't have either of those things. The fix is to merge the
indexes placeholder into the table zone.

Second, the optimizer required that a prefix of the locality tiers match
constraints. That is no longer required after this commit. For example, if
locality=region=us;dc=east, then it now matches any of these constraint
sets equally well:

[+locality=region=us;+dc=east]
[+dc=east]
[+dc=east,+dc=west]

A missing constraint match is OK (i.e. region=us), as long as a more specific
locality tier matches (i.e. dc=east).

Fixes #36642
Fixes #36644

Release note: None

cockroach-teamcity · 2019-04-09T22:41:19Z

This change is

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @justinj, @RaduBerinde, and @rytaft)

pkg/config/system.go, line 273 at r1 (raw file):

}

// getCachedZone returns the ZoneConfig for the given object ID. In the fast

Great change here, I was worried we'd have to add some special cases for our usecase, but this actually cleans up existing code!

pkg/config/system.go, line 298 at r1 (raw file):

			// Merge placeholder with zone by copying over subzone information.
			// Placeholders should only define the Subzones and SubzoneSpans fields.
			copy := *zone

[nit] zoneCopy (the syntax highlighting is annoying)

pkg/sql/opt/xform/coster.go, line 577 at r1 (raw file):

//   0.5 = [+region=us]           // 1st tier matches
//   0.5 = [+region=us,-dc=east]  // 1st tier matches, 2nd tier PROHIBITED
//   0.5 = [+region=eu,+dc=east]  // 2nd tier matches, but 1st tier doesn't

Wouldn't we want to stop if we find a key that doesn't match (getting 0 here?) Seems wrong to award points for matching a dc in the wrong region.

+region=eu should be logically equivalent to -region=us if we have just these two regions; but they are scored differently.

I would define "match" for a given tier as:

0 = key not present in constraint
+1 = key present in constraint and a) constraint is required and value matches; or b) constraint is prohibited and value doesn't match*
-1 = otherwise

[*a minor detail - if the key shows up multiple times in the constraint, (a) or (b) must hold for all instances; this would be useful in practice for something like [-region=eu,-region=asia] which should match us but not match eu or asia]

Then the score would be the length of the longest prefix that ends in +1 and doesn't contain a -1.

In any case, please add the examples [-region=eu] and [-region=eu,+dc=east] above and to the test.

pkg/sql/opt/xform/coster.go, line 600 at r1 (raw file):

	}

	// matchConstraints returns the constraint set score described in the method

not quite, it returns

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @justinj, and @rytaft)

pkg/sql/opt/xform/coster.go, line 600 at r1 (raw file):

Previously, RaduBerinde wrote…

not quite, it returns

means to say it returns what's described above as (m-x) (I believe)

RaduBerinde

I left some comments on the scoring, but they have relatively minor practical implications. I'm definitely ok with this merging as-is, we really want the zone config fix.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball, @justinj, and @rytaft)

rytaft

Reviewed 8 of 8 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @justinj)

pkg/sql/opt/xform/coster.go, line 606 at r1 (raw file):

		noMatchCount := 0
		for i := range locality.Tiers {
			tier := locality.Tiers[i]

[nit] maybe you meant tier := &locality.Tiers[i]? If not, why not use the syntax for i, tier := range locality.Tiers?

andy-kimball

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @justinj and @rytaft)

pkg/sql/opt/xform/coster.go, line 577 at r1 (raw file):

Previously, RaduBerinde wrote…

Wouldn't we want to stop if we find a key that doesn't match (getting 0 here?) Seems wrong to award points for matching a dc in the wrong region.

+region=eu should be logically equivalent to -region=us if we have just these two regions; but they are scored differently.

I would define "match" for a given tier as:

0 = key not present in constraint

+1 = key present in constraint and a) constraint is required and value matches; or b) constraint is prohibited and value doesn't match*

-1 = otherwise

[*a minor detail - if the key shows up multiple times in the constraint, (a) or (b) must hold for all instances; this would be useful in practice for something like [-region=eu,-region=asia] which should match us but not match eu or asia]

Then the score would be the length of the longest prefix that ends in +1 and doesn't contain a -1.

In any case, please add the examples [-region=eu] and [-region=eu,+dc=east] above and to the test.

OK, I made changes along these lines. Also, I defined the "match" like this:

	//    0 = key not present in constraint set or key only matches prohibited
	//        constraints where value doesn't match
	//   +1 = key matches any required constraint key + value
	//   -1 = otherwise

I think (b) as you stated it is not quite right, because we only want a +1 if there was a "full" match. We want to treat prohibited constraint with non-matching value as if it was not present.

pkg/sql/opt/xform/coster.go, line 606 at r1 (raw file):

Previously, rytaft wrote…

[nit] maybe you meant tier := &locality.Tiers[i]? If not, why not use the syntax for i, tier := range locality.Tiers?

Done.

rytaft

Reviewed 4 of 4 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball and @justinj)

pkg/config/system.go, line 57 at r2 (raw file):

	// placeholder, there appear to be no guarantees that there can be no overlap.
	//
	// TOD(andyk): Use the combined value everywhere early in 19.2, so there's

[nit] TOD -> TODO

RaduBerinde

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball, @justinj, and @rytaft)

pkg/sql/opt/xform/coster.go, line 675 at r3 (raw file):

		// returned by matchConstraints for any replica. For example:
		//
		//   3: [+region=us,+dc=east]

[nit] are these 2, 1?

pkg/sql/opt/xform/coster_test.go, line 42 at r3 (raw file):

		{locality: "region=us,dc=east", constraints: "[-region=eu]", expected: 0.0},
		{locality: "region=us,dc=east", constraints: "[+region=us]", expected: 0.5},
		{locality: "region=us,dc=east", constraints: "[+region=us,+region=eu]", expected: 0.5},

Just a note - as far as I understand [+region=us,+region=eu] is invalid (it's a contradiction, so no replicas would match). I don't think it matters much what this code returns in these cases though.

On the other hand multiple negative constraints do make sense, can you add a few tests like that? e.g. [-region=asia,-region=eu] should be 0.5 and [-region=asia,-region=us] should be 0.

pkg/sql/opt/xform/coster_test.go, line 78 at r3 (raw file):

		var locality roachpb.Locality
		locality.Set(tc.locality)

linter complains about missing error checking

This PR fixes a couple of related issues with zone-based index selection. First, placeholder zones were incorrectly discarded. Placeholder zones are used when there are indexes with Constraints/Leaseholder Preferences on a table that doesn't have either of those things. The fix is to merge the indexes placeholder into the table zone. Second, the optimizer required that a prefix of the locality tiers match constraints. That is no longer required after this commit. For example, if locality=region=us;dc=east, then it now matches any of these constraint sets equally well: [+locality=region=us;+dc=east] [+dc=east] [+dc=east,+dc=west] A missing constraint match is OK (i.e. region=us), as long as a more specific locality tier matches (i.e. dc=east). Fixes cockroachdb#36642 Fixes cockroachdb#36644 Release note: None

andy-kimball

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @justinj, @RaduBerinde, and @rytaft)

pkg/config/system.go, line 57 at r2 (raw file):

Previously, rytaft wrote…

[nit] TOD -> TODO

Done.

pkg/sql/opt/xform/coster.go, line 675 at r3 (raw file):

Previously, RaduBerinde wrote…

[nit] are these 2, 1?

Could be anything. In this case, I was thinking there are 5 replicas, where 3 are in east, 2 may not be.

pkg/sql/opt/xform/coster_test.go, line 42 at r3 (raw file):

Previously, RaduBerinde wrote…

Just a note - as far as I understand [+region=us,+region=eu] is invalid (it's a contradiction, so no replicas would match). I don't think it matters much what this code returns in these cases though.

On the other hand multiple negative constraints do make sense, can you add a few tests like that? e.g. [-region=asia,-region=eu] should be 0.5 and [-region=asia,-region=us] should be 0.

Hm, I'd assumed that when multiple keys are involved, it should function like an "OR" operator (because "AND" makes no sense), which is actually very useful functionality, since it allows you to say, "put the index in either us or eu".

andy-kimball · 2019-04-10T21:34:40Z

bors r+

andy-kimball

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @justinj, @RaduBerinde, and @rytaft)

pkg/sql/opt/xform/coster_test.go, line 42 at r3 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Hm, I'd assumed that when multiple keys are involved, it should function like an "OR" operator (because "AND" makes no sense), which is actually very useful functionality, since it allows you to say, "put the index in either us or eu".

Since I got green build, I'm going to bors+ this. I can add those new tests in a future commit.

andy-kimball · 2019-04-10T21:55:09Z

bors r+

craig · 2019-04-10T22:35:37Z

Build failed (retrying...)

GitHub CI (Cockroach)

craig · 2019-04-10T22:53:45Z

Build failed

GitHub CI (Cockroach)

andy-kimball · 2019-04-10T23:03:59Z

bors r+

andy-kimball · 2019-04-10T23:04:22Z

Unrelated flaky test caused bors failure.

34258: storage: backpressure SSTs before other writes if L0 fills up r=dt a=dt Currently we start compacting as soon as there are two or more files in L0. If we reach 20, RocksDB will start slowing all writes in an attempt to help the compactor catch up. This global slowdown will affect foreground traffic, liveness, etc and could significantly impact latency of live traffic or even availability. Directly ingesting large numbers of small files could make it easier to hit this limit than it usually is with normal writes -- normal writes are buffered in the memtable and flushed all at once but direct ingestion could add potentially lots of files rather quickly. Additionally, SST ingetions are currently only done by bulk operations like RESTORE, IMPORT or index backfills. These are all less latency sensitive then foreground traffic, so we'd prefer to backpressure them before we risk approaching the global slowdown trigger. Release note (performance improvement): backpressure bulk operations before other traffic. 36689: opt: Fix and improve zone-based index selection r=andy-kimball a=andy-kimball This PR fixes a couple of related issues with zone-based index selection. First, placeholder zones were incorrectly discarded. Placeholder zones are used when there are indexes with Constraints/Leaseholder Preferences on a table that doesn't have either of those things. The fix is to merge the indexes placeholder into the table zone. Second, the optimizer required that a prefix of the locality tiers match constraints. That is no longer required after this commit. For example, if locality=region=us;dc=east, then it now matches any of these constraint sets equally well: [+locality=region=us;+dc=east] [+dc=east] [+dc=east,+dc=west] A missing constraint match is OK (i.e. region=us), as long as a more specific locality tier matches (i.e. dc=east). Fixes #36642 Fixes #36644 Release note: None Co-authored-by: David Taylor <[email protected]> Co-authored-by: Andrew Kimball <[email protected]>

craig · 2019-04-10T23:35:31Z

Build succeeded

GitHub CI (Cockroach)

andy-kimball requested review from justinj, rytaft, RaduBerinde and a team April 9, 2019 22:41

andy-kimball requested a review from a team as a code owner April 9, 2019 22:41

andy-kimball requested a review from a team April 9, 2019 22:41

RaduBerinde reviewed Apr 10, 2019

View reviewed changes

RaduBerinde approved these changes Apr 10, 2019

View reviewed changes

rytaft approved these changes Apr 10, 2019

View reviewed changes

andy-kimball force-pushed the zones branch from f3eac83 to ff3d01b Compare April 10, 2019 19:49

andy-kimball commented Apr 10, 2019

View reviewed changes

rytaft approved these changes Apr 10, 2019

View reviewed changes

andy-kimball force-pushed the zones branch from ff3d01b to fa9f729 Compare April 10, 2019 20:08

RaduBerinde reviewed Apr 10, 2019

View reviewed changes

andy-kimball force-pushed the zones branch from fa9f729 to 024c297 Compare April 10, 2019 20:54

andy-kimball commented Apr 10, 2019

View reviewed changes

andy-kimball mentioned this pull request Apr 10, 2019

release-19.1: opt: Fix and improve zone-based index selection #36737

Merged

craig bot merged commit 024c297 into cockroachdb:master Apr 10, 2019

andy-kimball deleted the zones branch April 10, 2019 23:43

andy-kimball mentioned this pull request Jun 26, 2019

sql: enhance setting of locality #37429

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opt: Fix and improve zone-based index selection #36689

opt: Fix and improve zone-based index selection #36689

andy-kimball commented Apr 9, 2019

cockroach-teamcity commented Apr 9, 2019

RaduBerinde left a comment

RaduBerinde left a comment

RaduBerinde left a comment

rytaft left a comment

andy-kimball left a comment

rytaft left a comment

RaduBerinde left a comment

andy-kimball left a comment

andy-kimball commented Apr 10, 2019

andy-kimball left a comment

andy-kimball commented Apr 10, 2019

craig bot commented Apr 10, 2019

craig bot commented Apr 10, 2019

andy-kimball commented Apr 10, 2019

andy-kimball commented Apr 10, 2019

craig bot commented Apr 10, 2019

opt: Fix and improve zone-based index selection #36689

opt: Fix and improve zone-based index selection #36689

Conversation

andy-kimball commented Apr 9, 2019

cockroach-teamcity commented Apr 9, 2019

RaduBerinde left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

andy-kimball left a comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

andy-kimball left a comment

Choose a reason for hiding this comment

andy-kimball commented Apr 10, 2019

andy-kimball left a comment

Choose a reason for hiding this comment

andy-kimball commented Apr 10, 2019

craig bot commented Apr 10, 2019

Build failed (retrying...)

craig bot commented Apr 10, 2019

Build failed

andy-kimball commented Apr 10, 2019

andy-kimball commented Apr 10, 2019

craig bot commented Apr 10, 2019

Build succeeded