sql: inject tenant ID in sqlServerArgs, pass through ExecutorConfig #48190
Conversation
|
This change is |
There was a problem hiding this comment.
As far as I'm concerned, merge this ASAP.
Reviewed 5 of 5 files at r1, 5 of 5 files at r2, 114 of 114 files at r3.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @asubiotto and @nvanbenschoten)
pkg/keys/sql.go, line 85 at r3 (raw file):
// This function should generally be avoided in favor of passing a server-wide // SQLCodec dependency to code in need. func SQLCodecForKey(key roachpb.Key) (SQLCodec, error) {
Is this needed from within SQL at all or is this a crutch for KV?
pkg/sql/sqlbase/structured.go, line 3615 at r3 (raw file):
func (desc *TableDescriptor) TableSpan(codec keys.SQLCodec) roachpb.Span { // TODO(nvanbenschoten): WHy does IndexSpan consider interleaves but // TableSpan does not? Should it?
cc @asubiotto
There was a problem hiding this comment.
evalCtx but I don't think it's this PR's problem
Reviewed 1 of 114 files at r3.
Reviewable status:
pkg/sql/sqlbase/structured.go, line 3615 at r3 (raw file):
Previously, tbg (Tobias Grieger) wrote…
cc @asubiotto
I'm not sure, maybe @jordanlewis knows?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlbase/structured.go, line 3615 at r3 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
I'm not sure, maybe @jordanlewis knows?
I don't know off hand. I looked at all users of TableSpan, and they all have special cases for interleaved tables. Drop table, "revert table", import, and refresh status for gc job. So I think this is fine but definitely could be a footgun. I say leave the TODO so people can see.
This has been unused for a while, possibly since Delete statements were pulled into the optimizer.
…nner This removes the only instance where the DistSQLPlanner was encoding keys. It allows us to avoid giving it a SQL codec in the next commit.
Fixes cockroachdb#47903. Also known as "the grand plumbing", this commit replaces a few instances of `TODOSQLCodec` in `pkg/sql/sqlbase/index_encoding.go` and watches the house of cards fall apart. It then glues the world back together, this time using a properly injected tenant-bound SQLCodec to encode and decode all SQL table keys. A tenant ID field is added to `sqlServerArgs`. This is used to construct a tenant-bound `keys.SQLCodec` during server creation. This codec morally lives on the `sql.ExecutorConfig`. In practice, it is also copied onto `tree.EvalContext` and `execinfra.ServerConfig` to help carry it around. SQL code is adapted to use this codec whenever it needs to encode or decode keys. If all tests pass after this refactor, there is a good chance it got things right. This is because any use of an uninitialized SQLCodec will panic immediately when the codec is first used. This was helpful in ensuring that it was properly plumbed everywhere.
nvanbenschoten
force-pushed
the
nvanbenschoten/tenantKey3
branch
from
cc336c5
to
164f82b
Compare
There was a problem hiding this comment.
TFTRs!
Reviewable status:
pkg/keys/sql.go, line 85 at r3 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Is this needed from within SQL at all or is this a crutch for KV?
Not, it was a crutch for a single use in NewUniquenessConstraintViolationError, where we take a key and re-encode it just to grab the datums from a row.Fetcher. It's all rather hacky and also a pain to thread a codec down to. I got around it another way so I deleted this.
pkg/sql/sqlbase/structured.go, line 3615 at r3 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
I don't know off hand. I looked at all users of TableSpan, and they all have special cases for interleaved tables. Drop table, "revert table", import, and refresh status for gc job. So I think this is fine but definitely could be a footgun. I say leave the TODO so people can see.
Done.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
bors r+ |
Build succeeded |
Informs cockroachdb#48123. This commit continues with the plumbing that began an cockroachdb#48190. It pushes a tenant-bound SQL codec into the other main source of key generation in the SQL layer - descriptor manipulation and metadata handling. This allows SQL tenants to properly handle metadata descriptors for its database and tables. This ended up being a larger undertaking than I had originally expected. However, now that it's complete, we're in a pretty good spot: 1. `sqlbase.MetadataSchema` is ready to be used for cockroachdb#47904. 2. we can now run SQL migrations for a non-system tenant 3. there is only one remaining use of TODOSQLCodec in pkg/sql. See cockroachdb#48375.
Informs cockroachdb#48123. This commit continues with the plumbing that began an cockroachdb#48190. It pushes a tenant-bound SQL codec into the other main source of key generation in the SQL layer - descriptor manipulation and metadata handling. This allows SQL tenants to properly handle metadata descriptors for its database and tables. This ended up being a larger undertaking than I had originally expected. However, now that it's complete, we're in a pretty good spot: 1. `sqlbase.MetadataSchema` is ready to be used for cockroachdb#47904. 2. we can now run SQL migrations for a non-system tenant 3. there is only one remaining use of TODOSQLCodec in pkg/sql. See cockroachdb#48375.
48376: sql: push tenant-bound SQL codec into descriptor key generation r=nvanbenschoten a=nvanbenschoten Informs #48123. This commit continues with the plumbing that began an #48190. It pushes a tenant-bound SQL codec into the other main source of key generation in the SQL layer - descriptor manipulation and metadata handling. This allows SQL tenants to properly handle metadata descriptors for its database and tables. This ended up being a larger undertaking than I had originally expected. However, now that it's complete, we're in a pretty good spot: 1. `sqlbase.MetadataSchema` is ready to be used for #47904. 2. we can now run SQL migrations for a non-system tenant. 3. there is only one remaining use of `keys.TODOSQLCodec` in pkg/sql. See #48375. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
48376: sql: push tenant-bound SQL codec into descriptor key generation r=nvanbenschoten a=nvanbenschoten Informs cockroachdb#48123. This commit continues with the plumbing that began an cockroachdb#48190. It pushes a tenant-bound SQL codec into the other main source of key generation in the SQL layer - descriptor manipulation and metadata handling. This allows SQL tenants to properly handle metadata descriptors for its database and tables. This ended up being a larger undertaking than I had originally expected. However, now that it's complete, we're in a pretty good spot: 1. `sqlbase.MetadataSchema` is ready to be used for cockroachdb#47904. 2. we can now run SQL migrations for a non-system tenant. 3. there is only one remaining use of `keys.TODOSQLCodec` in pkg/sql. See cockroachdb#48375. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com> Release note (<category, see below>): <what> <show> <why>
Fixes #47903.
Informs #48123.
Also known as "the grand plumbing", this change replaces a few instances of
TODOSQLCodecinpkg/sql/sqlbase/index_encoding.goand watches the house of cards fall apart. It then glues the world back together, this time using a properly injected tenant-bound SQLCodec to encode and decode all SQL table keys.A tenant ID field is added to
sqlServerArgs. This is used to construct a tenant-boundkeys.SQLCodecduring server creation. This codec morally lives on thesql.ExecutorConfig. In practice, it is also copied ontotree.EvalContextandexecinfra.ServerConfigto help carry it around. SQL code is adapted to use this codec whenever it needs to encode or decode keys.If all tests pass after this refactor, there is a good chance it got things right. This is because any use of an uninitialized SQLCodec will panic immediately when the codec is first used. This was helpful in ensuring that it was properly plumbed everywhere.