sql: refrain from performing invalid splits during tenant creation
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 https://github.com/cockroachdb/cockroach/pull/104802. This patch fixes the root cause of why these splits were being issued in the first place -- over in https://github.com/cockroachdb/cockroach/commit/ac54eba6c238c6ef17c14199b80ad00eab7bf5ac, 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 https://github.com/cockroachdb/cockroach/issues/104928. Prevents https://github.com/cockroachdb/cockroach/issues/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.
Showing
- pkg/keys/sql.go 13 additions, 2 deletionspkg/keys/sql.go
- pkg/rpc/auth_tenant.go 2 additions, 0 deletionspkg/rpc/auth_tenant.go
- pkg/sql/catalog/bootstrap/BUILD.bazel 14 additions, 1 deletionpkg/sql/catalog/bootstrap/BUILD.bazel
- pkg/sql/catalog/bootstrap/bootstrap_test.go 6 additions, 0 deletionspkg/sql/catalog/bootstrap/bootstrap_test.go
- pkg/sql/catalog/bootstrap/main_test.go 34 additions, 0 deletionspkg/sql/catalog/bootstrap/main_test.go
- pkg/sql/catalog/bootstrap/metadata.go 3 additions, 2 deletionspkg/sql/catalog/bootstrap/metadata.go
- pkg/sql/catalog/bootstrap/splits_test.go 74 additions, 0 deletionspkg/sql/catalog/bootstrap/splits_test.go
- pkg/sql/tenant_creation.go 26 additions, 24 deletionspkg/sql/tenant_creation.go
Loading
Please register or sign in to comment