spanconfig: carve empty ranges for pseudo-table IDs #74171
Conversation
|
This change is |
irfansharif
force-pushed
the
211221.translator-pseudo-ids
branch
2 times, most recently
from
bb527c6
to
f9b9ee1
Compare
There was a problem hiding this comment.
Reviewed 13 of 14 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, and @irfansharif)
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 90 at r1 (raw file):
} if (id == keys.SystemDatabaseID || id == keys.RootNamespaceID) && s.codec.ForSystemTenant() {
Could you add a comment explaining why we need to handle this case when id == keys.SystemDatabaseID || id == keys.RootNamespaceID?
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 107 at r1 (raw file):
// removed (#70560). for _, pseudoTableID := range keys.PseudoTableIDs { zone, err := sql.GetHydratedZoneConfigForDatabase(ctx, txn, s.codec, keys.SystemDatabaseID)
We're setting the system database's zone config for each pseudo table? Don't we allow custom zone configs to be set on individual pseudo IDs? Is that handled here? My memory of this is hazy, so I might be misremembering something.
Actually, I might be misunderstanding this patch. It's not trying to respect the zone configs configured on the keyspaces referenced by these pseudo IDs — that's already handled by generateSpanConfigurationsForNamedZone. This is just trying to add split points in the system database on them. Is that the correct understanding?
If so, would it be possible to unify this code further by having findDescendantLeafIDs return the IDs of all psuedo tables when given the SystemDatabaseID and then having generateSpanConfigurations handle these IDs properly? Would that avoid the SystemDatabaseID || RootNamespaceID complication above?
There was a problem hiding this comment.
Reviewable status:
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 90 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Could you add a comment explaining why we need to handle this case when
id == keys.SystemDatabaseID || id == keys.RootNamespaceID?
Did you happen to see the comment right below? Moved it above the addedPseudoTableSpans no-op check to make it clearer. For your question specifically:
// We have special handling for the system database (and RANGE
// DEFAULT, which the system database inherits from). The system
Also see comment below.
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 107 at r1 (raw file):
It's not trying to respect the zone configs configured on the keyspaces referenced by these pseudo IDs — that's already handled by generateSpanConfigurationsForNamedZone. This is just trying to add split points in the system database on them. Is that the correct understanding?
Exactly! Added a footnote to the comment block, hopefully making it clearer.
// [1]: Consider the liveness range [/System/NodeLiveness,
// /System/NodeLivenessMax). It's identified using the
// pseudo ID 22 (i.e. keys.LivenessRangesID). Because we're
// using a pseudo ID, what of [/Table/22-/Table/23)? This
// is a keyspan with no contents, yet one the system config
// span splits along to create an empty range. It's
// precisely this "feature" we're looking to emulate. As
// for what config to apply over said range -- we do as the
// system config span does, applying the config for the
// system database.
If so, would it be possible to unify this code further by having findDescendantLeafIDs return the IDs of all psuedo tables when given the SystemDatabaseID and then having generateSpanConfigurations handle these IDs properly?
It's not possible with how the code is currently structured, and I wanted to keep this diff minimal. The problem is that generateSpanConfigurations knows to generate "special" spans when passed in a pseudo ID. In our example above, even if we augmented findDescendantLeafIDs to include these pseudo IDs when operating off of DATABASE SYSTEM/RANGE DEFAULT, when a specific pseudo range ID (say, keys.LivenessRangesID) is passed into generateSpanConfigurations, it would generate what the pseudo ID represents (in our example [/System/NodeLiveness, /System/NodeLivenessMax), instead of [/Table/22-/Table/23)). It's possible to make it work, but I found this to be simpler.
irfansharif
force-pushed
the
211221.translator-pseudo-ids
branch
from
f9b9ee1
to
e6925ae
Compare
There was a problem hiding this comment.
Reviewed 12 of 14 files at r1, all commit messages.
Reviewable status:
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 90 at r2 (raw file):
} if (id == keys.SystemDatabaseID || id == keys.RootNamespaceID) && s.codec.ForSystemTenant() {
nit: consider pulling all this logic to handle empty ranges corresponding to pseudo IDs into its own function. You'll be iterating over IDs twice, but I think it reads clearer.
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 104 at r2 (raw file):
// removed (#70560). // // [2]: Consider the liveness range [/System/NodeLiveness,
s/[2]/[1]/g?
pkg/sql/zone_config.go, line 364 at r2 (raw file):
} zoneID, zone, _, _, err := getZoneConfig( codec, id, getKey, false /* getInheritedDefault */, true, /* mayBeTable */
Should mayBeTable be false here?
irfansharif
force-pushed
the
211221.translator-pseudo-ids
branch
from
e6925ae
to
d6aa093
Compare
This commit is similar in spirit to cockroachdb#73746 in that it gets rid of a benign difference between the span configs infrastructure and the system config span. Whe nnabling span configs by default (cockroachdb#73876), we observed a wide blast radius with respect to test failures. This was in large part due to assumptions we've baked in regarding the number of splits we should expect at cluster start, often waiting for the same number of ranges to form before executing the rest of the test. The differences between the SystemConfigSpan and the span configs infrastructure in how they handled (empty) pseudo table keyspaces makes for an annoying large list of tests to adjust manually. Instead we take the lazy route and generate empty ranges for pseudo table IDs as we did before. We can get rid of this special handling once we actually get rid of the system config span, and adjust all these tests accordingly. Doing it later is also preferable should we need to revert pieces of cockroachdb#73876 due to unforeseen instability. Release note: None
irfansharif
force-pushed
the
211221.translator-pseudo-ids
branch
from
d6aa093
to
a04b49b
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 90 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: consider pulling all this logic to handle empty ranges corresponding to pseudo IDs into its own function. You'll be iterating over IDs twice, but I think it reads clearer.
Done.
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 104 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
s/[2]/[1]/g?
Done.
pkg/sql/zone_config.go, line 364 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Should
mayBeTablebe false here?
Done.
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
74265: *: future proof tests for span configs r=irfansharif a=irfansharif This PR is a (mostly) test-only change that paves the way for enabling span configs by default (#73876). Various tests throughout CRDB rely on timing assumptions with respect to how quickly splits are induced or configs applied, assumptions that are no longer true with the span configs infrastructure because: - updated descriptors/zone configs make its way to the global span config state `system.span_configurations` asynchronously, and - kv learns about learns about these updates asynchronously. Consider TestTruncatePreservesSplitPoints for example, which relied on range split decisions to happen near-instantaneously (especially for the single node variant) -- a fair assumption with the gossiped SystemConfigSpan, but no longer applicable. Another example is TestChangefeedProtectedTimestamps, where we use an extremely low gc.ttlseconds. Given span configs makes use of rangefeeds internally, feeds that error out if started at timestamps already GC-ed, we need to ensure a sufficiently lax GC TTL. For most of these tests, making them agnostic to either subsystem is just a matter of throwing in a SucceedsSoon block at the right places. See individual commits/commentary for the individual test changes. Release note: None --- First commit is from #74171 and should be reviewed there. 74751: sql: migrate has_database_privilege from evalPrivilegeCheck to ctx.Planner.HasPrivilege r=otan a=ecwall refs #66173 Migrate has_database_privilege from evalPrivilegeCheck to ctx.Planner.HasPrivilege. Release note: None I'm splitting up has_database_privilege, has_schema_privilege, has_sequence_privilege into separate PRs because each of them has some auxiliary work (in this case changing `'` to `"` in a test, but it looks like schema and sequence will have a bit more) to complete them. 74800: roachtest: fix sequelize nightly r=otan a=rafiss fixes #74057 Upstream changed how imports are done, so this library had to be updated. Release note: None 74807: kvstreamer: fix potential deadlock r=yuzefovich a=yuzefovich We have two different locks in the `Streamer` infrastructure: one is for the `Streamer` itself and another one for the `budget`. Previously, there was no contract about which mutex needs to be acquired first which led to the deadlock detector thinking that there is a potential deadlock situation. This is now fixed by requiring that the `budget`'s mutex is acquired first and by releasing the `Streamer`'s mutex in `Enqueue` early in order to not overlap with the interaction with the `budget`. I believe that it was a false positive (i.e. the deadlock cannot actually occur) because without the support of pipelining, `Enqueue` calls and asynchronous requests evaluation never overlap in time. Still, it's good to fix the order of mutex acquisitions. Fixes: #74782. Fixes: #74785. Fixes: #74787. Fixes: #74788. Fixes: #74789. Fixes: #74790. Fixes: #74791. Release note: None Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com> Co-authored-by: Evan Wall <wall@cockroachlabs.com> Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
This commit is similar in spirit to #73746 in that it gets rid of a
benign difference between the span configs infrastructure and the system
config span. When enabling span configs by default (#73876), we observed
a wide blast radius with respect to test failures. This was in large
part due to assumptions we've baked in regarding the number of splits we
should expect at cluster start, often waiting for the same number of
ranges to form before executing the rest of the test.
The differences between the SystemConfigSpan and the span configs
infrastructure in how they handled (empty) pseudo table keyspaces makes
for an annoying large list of tests to adjust manually. Instead we take
the lazy route and generate empty ranges for pseudo table IDs as we did
before. We can get rid of this special handling once we actually get rid
of the system config span, and adjust all these tests accordingly. Doing
it later is also preferable should we need to revert pieces of #73876
due to unforeseen instability.
Release note: None