sql: push tenant-bound SQL codec into descriptor key generation#48376
sql: push tenant-bound SQL codec into descriptor key generation#48376craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
tbg
left a comment
There was a problem hiding this comment.
That wasn't fun to review, I bet it was even worse to write. 
Reviewed 6 of 6 files at r1, 136 of 136 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @asubiotto and @nvanbenschoten)
pkg/sql/zone_config.go, line 41 at r2 (raw file):
// to make sure to look at the correct Zones according to the tenant prefix of // its key range? See #48375. var zoneConfigCodec = keys.TODOSQLCodec
I would have expected us to use SystemCodec here as I don't anticipate any plan to allow tenants to store their own zone configs separately from the system tenants. Fine to merge with this TODO in place.
pkg/sql/tests/hash_sharded_test.go, line 139 at r2 (raw file):
} tableDesc := sqlbase.GetTableDescriptor(kvDB, keys.SystemSQLCodec, `d`, `kv_secondary`)
Maybe something we can look into is to replace these calls in tests with RandomCodec() where possible, to improve testing coverage.
asubiotto
left a comment
There was a problem hiding this comment.
Reviewable status:
These were all pretty easy because there was a ExecutorConfig nearby.
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.
cf00e3c to
f618c80
Compare
|
TFTR! I can't imagine that was an enjoyable experience. Merging to avoid any more skew. bors r+ |
Build succeeded |
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>
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:
sqlbase.MetadataSchemais ready to be used for sql: tenant deletion #47904.keys.TODOSQLCodecin pkg/sql. See sql/kv: how does multi-tenancy interact with zone configurations? #48375.