sql: refrain from performing invalid splits during tenant creation #104920
sql: refrain from performing invalid splits during tenant creation #104920craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
bccd767 to
a127da8
Compare
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
a127da8 to
5264f7c
Compare
5264f7c to
f293524
Compare
Normally, `MakeTenantPrefix(ID).PrefixEnd()` is the same as `MakeTenantPrefix(ID + 1)`. However, this isn't true if the ID corresponds to one of the boundary points in `EncodeUvarintAscending`. Prior to this patch, various places in the code did not appreciate this subtlety. This proved problematic when we started creating split points at `MakeTenantPrefix(ID).PrefixEnd()`. Now, if the tenant ID was equal to one of these boundary points (e.g. 109), decoding the key `MakeTenantPrefix(ID).PrefixEnd()` does not return the expected tenant ID (in our case, 110). Instead, it results in an error. Worse yet, various places in KV assume doing such tenant decoding at a range's start key is kosher. We've since disallowed KV from accepting such splits in cockroachdb#104802. This patch fixes the root cause of why these splits were being issued in the first place -- over in cockroachdb@ac54eba, we started creating spilt points at the end of a tenant's keyspace (in addition to the start of it). We did so using `PrefixEnd()`, which we have since learned is not what we wanted here. In addition to the initial split point, we also fix the span config records we seed during tenant creation. We've also added a test to ensure all split points created during cluster creation are kosher. The test uses randomized tenant IDs -- I confirmed that it fails with tenant ID = 109 (without my changes). Lastly, there's a bit more auditing work that needs to be done here about these assumptions. That's captured in a followup issue cockroachdb#104928. Prevents cockroachdb#104606 from happening. Release note (bug fix): Fixes a bug where tenant creation for certain IDs would always fail because of invalid split points. Additionally, such tenant creation failure could leave the host cluster's span config state entirely busted -- we prevent that as well.
f293524 to
42fc585
Compare
|
The datarace failure here seems to be #104838 |
nvb
left a comment
There was a problem hiding this comment.
Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @irfansharif)
|
TFTRs! bors r=nvanbenschoten,ecwall |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 42fc585 to blathers/backport-release-23.1-104920: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Normally,
MakeTenantPrefix(ID).PrefixEnd()is the same asMakeTenantPrefix(ID + 1). However, this isn't true if the IDcorresponds to one of the boundary points in
EncodeUvarintAscending.Prior to this patch, various places in the code did not appreciate this
subtlety. This proved problematic when we started creating split points
at
MakeTenantPrefix(ID).PrefixEnd(). Now, if the tenant ID was equalto one of these boundary points (e.g. 109), decoding the key
MakeTenantPrefix(ID).PrefixEnd()does not return the expected tenantID (in our case, 110). Instead, it results in an error.
Worse yet, various places in KV assume doing such tenant decoding at a
range's start key is kosher. We've since disallowed KV from accepting
such splits in #104802.
This patch fixes the root cause of why these splits were being issued
in the first place -- over in
ac54eba,
we started creating spilt points at the end of a tenant's keyspace
(in addition to the start of it). We did so using
PrefixEnd(), whichwe have since learned is not what we wanted here.
In addition to the initial split point, we also fix the span config
records we seed during tenant creation. We've also added a test to
ensure all split points created during cluster creation are kosher.
The test uses randomized tenant IDs -- I confirmed that it fails with
tenant ID = 109 (without my changes).
Lastly, there's a bit more auditing work that needs to be done here
about these assumptions. That's captured in a followup issue
#104928.
Prevents #104606 from
happening.
Release note (bug fix): Fixes a bug where tenant creation for certain
IDs would always fail because of invalid split points. Additionally,
such tenant creation failure could leave the host cluster's span
config state entirely busted -- we prevent that as well.