multitenant: support crdb_internal.list_sql_keys_in_range() for secondary tenants #95100
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @knz, @rafiss, and @yuzefovich)
pkg/sql/region_util.go line 2426 at r1 (raw file):
execCfg := p.execCfg tenantSpan := execCfg.Codec.TenantSpan() rangeDescIterator, err := execCfg.RangeDescIteratorFactory.NewIterator(ctx, tenantSpan)
This fixes the error caused by trying to access Meta2, but the RangeDescriptor can have an EndKey of var KeyMax = []byte{0xff, 0xff} which causes a similar error because {0xff, 0xff} is not inside the tenant keyspace.
Does it make sense to make sure that the RangeDescriptor returned by RangeDescIteratorFactory has keys within the specified tenantSpan? Or should the result be intersected like tenantSpan.Intersect(rangeSpan) whenever it is used?
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status:
pkg/sql/region_util.go line 2426 at r1 (raw file):
Previously, ecwall (Evan Wall) wrote…
This fixes the error caused by trying to access
Meta2, but theRangeDescriptorcan have anEndKeyofvar KeyMax = []byte{0xff, 0xff}which causes a similar error because{0xff, 0xff}is not inside the tenant keyspace.Does it make sense to make sure that the
RangeDescriptorreturned byRangeDescIteratorFactoryhas keys within the specifiedtenantSpan? Or should the result be intersected liketenantSpan.Intersect(rangeSpan)whenever it is used?
I'm not sure. I'm OK leaving that problem open for now (with a test case that demonstrates the problem) and then address this comprehensively when we look at the group of issues around range boundaries.
pkg/sql/sem/builtins/generator_builtins.go line 2145 at r1 (raw file):
func newSpanKeyIterator(evalCtx *eval.Context, span roachpb.Span) *spanKeyIterator { return &spanKeyIterator{
nit: remove this empty line.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/eval/deps.go line 362 at r1 (raw file):
// GetTenantRangeRSpanByID gets the Span of the range (specified by RangeID) // and scopes it to the tenant.
Something about this feels a bit off -- specifically, the "scope it to the tenant" part. Suddenly, what is a span that corresponds to a range in the system is no longer that, which makes me question what we are using it for. Why do we need this?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/eval/deps.go line 362 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Something about this feels a bit off -- specifically, the "scope it to the tenant" part. Suddenly, what is a span that corresponds to a range in the system is no longer that, which makes me question what we are using it for. Why do we need this?
Can we do this at the caller instead of adding this wart to the interface?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/eval/deps.go line 362 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Can we do this at the caller instead of adding this wart to the interface?
Moving it up into the call site is even worse because every caller will need to know that range's span needs an intersection done on it before it can be used.
Maybe it's worth it to make a TenantRangeDescriptor model with only info that the tenant should see?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/builtins/generator_builtins.go line 2145 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
nit: remove this empty line.
Fixed.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/eval/deps.go line 362 at r1 (raw file):
I'm not sure what you mean by every call site -- isn't there just one?
That being said, that's kind of my point, callers should perform this intersection, explicitly, if there's a legitimate reason to do so. The real issue here is that ranges may span across tenant boundaries, and hiding the magic required to deal with this wart behind an interface gives it unwarranted credibility.
Could we change this to GetRangeDescriptor instead?
Maybe it's worth it to make a TenantRangeDescriptor model with only info that the tenant should see?
Instead of introducing a new concept, we should fix #92072 instead.
Wait what? how so? |
There was a problem hiding this comment.
Thanks for fixing #92072 while you were here! Let's add some words about it, and how we fixed it, in the commit message (or, better yet, let's pull it out into its own commit).
Reviewed 3 of 10 files at r1, 9 of 9 files at r3, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Agreed for a better description in the commit message of what is changing here. Currently the commit message is incomplete: it does not explain what is being done to the tenant range boundaries in tenant creation and span configs.
Reviewed 9 of 9 files at r3, all commit messages.
Reviewable status:
pkg/sql/tenant_creation.go line 315 at r3 (raw file):
return roachpb.TenantID{}, err } tenantPrefixEnd := tenantPrefix.PrefixEnd()
Thanks for this. Can you add a comment that explains why this is important.
ecwall
requested review from
herkolategan and
smg260
and removed request for
a team
Fixes #92072 The last range of the last tenant's end key is `/Max` which is outside of the tenant's key space. This is fixed by adding a split at the end of the tenant's key space when the tenant is created. For example, when tenant 10 is created, a split is added at /tenant/11. Release note: None
…dary tenants Fixes #95006 The crdb_internal.list_sql_keys_in_range builtin now uses the RangeDescIterator and scopes the range span by the tenant span to prevent these errors: ``` ERROR: RangeIterator failed to seek to /Meta2/"\x00": rpc error: code = Unauthenticated desc = requested key /Meta2/"\x00" not fully contained in tenant keyspace /Tenant/1{2-3} ``` Release note: None
|
bors r=knz,arulajmani |
|
Build succeeded: |
99186: builtins: add hint for hide_sql_constants and redactable_sql_constants r=msirek a=michae2 Add a hint about using dollar-quotes to the info message for builtin functions `crdb_internal.hide_sql_constants` and `crdb_internal.redactable_sql_constants`. Fixes: #99132 Epic: None Release note: None 99246: multitenant: add tenant end key split to MetadataSchema r=knz a=ecwall Fixes #97985 Previously #95100 created tenant end key split points asynchronously which allowed for a brief period after tenant creation where the tenant's keyspace had an end key of /Max instead of the next tenant's start key. This change creates that split point synchronously on tenant creation to avoid race conditions. Release note: None Co-authored-by: Michael Erickson <[email protected]> Co-authored-by: Evan Wall <[email protected]>
Fixes #97985 Previously #95100 created tenant end key split points asynchronously which allowed for a brief period after tenant creation where the tenant's keyspace had an end key of /Max instead of the next tenant's start key. This change creates that split point synchronously on tenant creation to avoid race conditions. Release note: None
This function was added as part of #95100 to block until the tenant's end key split point was created asynchronously, but is no longer needed because the tenant end key split point is created synchronously on tenant creation. Release note: None
Fixes #95006
The crdb_internal.list_sql_keys_in_range builtin now uses the RangeDescIterator
and scopes the range span by the tenant span to prevent these errors:
Fixes #92072
The last range of the last tenant's end key is
/Maxwhich is outside of thetenant's key space.
This is fixed by adding a split at the end of the tenant's key space when the
tenant is created. For example, when tenant 10 is created, a split is added at
/tenant/11.
Release note: None