sql/kv: remove all uses of keyutils.TestingSQLCodec (previously keys.TODOSQLCodec) #48123
Closed
8 tasks done
Labels
A-multitenancy
Related to multi-tenancy
C-cleanup
Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior.
T-kv
KV Team
Comments
nvanbenschoten
added
A-multitenancy
nvanbenschoten
changed the title
craig bot
pushed a commit
that referenced
this issue
Apr 29, 2020
46992: sql: Add Logical Column ID field to ColumnDescriptor r=rohany a=RichardJCai The LogicalColumnID field mimics the ColumnID field however LogicalColumnID may be swapped between two columns whereas ColumnID cannot. LogicalColumnID is referenced for virtual tables (pg_catalog, information_schema) and most notably affects column ordering for SHOW COLUMNS. This LogicalColumnID field support swapping the order of two columns - currently only used for ALTER COLUMN TYPE when a shadow column is created and swapped with it's original column. Does not affect existing behaviour. Release note: None 47449: cli: add --cert-principal-map to client commands r=petermattis a=petermattis Add support for the `--cert-principal-map` flag to the certs and client commands. Anywhere we were accepting the `--certs-dir` flag, we now also accept the `--cert-principal-map` flag. Fixes #47300 Release note (cli change): Support the `--cert-principal-map` flag in the `cert *` and "client" commands such as `sql`. 48138: keys: support splitting Ranges on tenant-id prefixed keys r=nvanbenschoten a=nvanbenschoten Fixes #48122. Relates to #47903. Relates to #48123. This PR contains a series of small commits that work towards the introduction of tenant-id prefixed keyspaces and begin the removal of some `keys.TODOSQLCodec` instances. This should be the only time we need to touch C++ throughout this work. 48160: storage,libroach: Check for MaxKeys when reading from intent history r=itsbilal a=itsbilal We weren't checking for MaxKeys (or TargetBytes) being reached in the case where we read from intent history in the MVCC scanner. All other cases go through addAndAdvance(), which had these checks. Almost certainly fixes #46652. Would be very surprised if it was something else. Release note (bug fix): Fixes a bug where a read operation in a transaction would error out for exceeding the maximum count of results returned. 48162: opt: add rule to eliminate Exists when input has zero rows r=rytaft a=rytaft This commit adds a new rule, `EliminateExistsZeroRows`, which converts an `Exists` subquery to False when it's known that the input produces zero rows. Informs #47058 Release note (performance improvement): The optimizer can now detect when an Exists subquery can be eliminated because the input has zero rows. This leads to better plans in some cases. Co-authored-by: richardjcai <[email protected]> Co-authored-by: Peter Mattis <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]> Co-authored-by: Bilal Akhtar <[email protected]> Co-authored-by: Rebecca Taft <[email protected]>
craig bot
pushed a commit
that referenced
this issue
Apr 30, 2020
48190: sql: inject tenant ID in sqlServerArgs, pass through ExecutorConfig r=nvanbenschoten a=nvanbenschoten Fixes #47903. Informs #48123. Also known as "the grand plumbing", this change 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. 48245: kv: declare write access to AbortSpan on all aborting EndTxn reqs r=nvanbenschoten a=nvanbenschoten Fixes #43707. Fixes #48046. Fixes #48189. Part of the change made by #42765 was to clear AbortSpan entries on non-poisoning, aborting EndTxn requests. Specifically, this change was made in 1328787. The change forgot to update the corresponding span declaration logic to reflect the fact that we were now writing to the AbortSpan in cases where we previously weren't. This was triggering an assertion in race builds that tried to catch this kind of undeclared span access. The assertion failure was very rare because it required the following conditions to all be met: 1. running a test with the race detector enabled 2. a txn (A) must have been aborted by another txn (B) 3. txn B must have cleared an intent on txn A's transaction record range 4. txn A must have noticed and issued a non-poisoning EndTxn(ABORT) We should backport this when we get a change (once v20.1.0 has stabilized), but I don't expect that this could actually cause any issues. The AbortSpan update was strictly a matter of performance and we should never be racing with another request that is trying to read the same AbortSpan entry. Co-authored-by: Nathan VanBenschoten <[email protected]>
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this issue
May 4, 2020
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.
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this issue
May 5, 2020
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.
craig bot
pushed a commit
that referenced
this issue
May 5, 2020
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 <[email protected]>
Anzoteh96
pushed a commit
to Anzoteh96/cockroach
that referenced
this issue
May 5, 2020
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 <[email protected]> Release note (<category, see below>): <what> <show> <why>
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this issue
Jun 2, 2020
Fixes cockroachdb#49318. Fixes cockroachdb#49445. Progress towards cockroachdb#48123. Informs cockroachdb#48774. This commit introduces a new pseudo object ID in the system tenant's namespace called "tenants". Like "liveness" and "timeseries" before it, the pseudo object allows zone configurations to be applied to pseudo-objects that do not live directly in the system tenant's SQL keyspace. In this case, the "tenants" zone allows zone configurations to be set by the system tenant and applied to all other tenants in the system. There may come a time when we want secondary tenants to have more control over their zone configurations, but that will require a much larger change to the zone config structure and UX as a whole. While making this change, we rationalize the rest of zone configuration handling and how it relates to multi-tenancy. Now that secondary tenant ranges have a zone config to call their own, we can make sense of calls from SQL into the zone configuration infrastructure. We gate off calls that don't make sense for secondary tenants and clean up hooks in SQL that handle zone config manipulation. All of this works towards a good cause - we eliminate the remaining uses of `keys.TODOSQLCodec` from `pkg/sql/...` and `pkg/config/...`, bringing us a big step closer towards being able to remove the placeholder and close cockroachdb#48123. This work also reveals that in order to address cockroachdb#48774, we need to be able to determine split points from the SystemConfig. This makes it very difficult to split on secondary tenant object (e.g. table) boundaries. However, it makes it straightforward to split on secondary tenant keysapce boundaries. This is already what we were thinking (see cockroachdb#47907), so out both convenience and desire, I expect that we'll follow this up with a PR that splits Ranges only at secondary tenant boundaries - placing the overhead of an otherwise empty tenant at only a single Range and a few KBs of data.
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this issue
Jun 3, 2020
Fixes cockroachdb#49318. Fixes cockroachdb#49445. Progress towards cockroachdb#48123. Informs cockroachdb#48774. This commit introduces a new pseudo object ID in the system tenant's namespace called "tenants". Like "liveness" and "timeseries" before it, the pseudo object allows zone configurations to be applied to pseudo-objects that do not live directly in the system tenant's SQL keyspace. In this case, the "tenants" zone allows zone configurations to be set by the system tenant and applied to all other tenants in the system. There may come a time when we want secondary tenants to have more control over their zone configurations, but that will require a much larger change to the zone config structure and UX as a whole. While making this change, we rationalize the rest of zone configuration handling and how it relates to multi-tenancy. Now that secondary tenant ranges have a zone config to call their own, we can make sense of calls from KV into the zone configuration infrastructure. We gate off calls that don't make sense for secondary tenants and clean up hooks in SQL that handle zone config manipulation. All of this works towards a good cause - we eliminate the remaining uses of `keys.TODOSQLCodec` from `pkg/sql/...` and `pkg/config/...`, bringing us a big step closer towards being able to remove the placeholder and close cockroachdb#48123. This work also reveals that in order to address cockroachdb#48774, we need to be able to determine split points from the SystemConfig. This makes it very difficult to split on secondary tenant object (e.g. table) boundaries. However, it makes it straightforward to split on secondary tenant keysapce boundaries. This is already what we were thinking (see cockroachdb#47907), so out both convenience and desire, I expect that we'll follow this up with a PR that splits Ranges only at secondary tenant boundaries - placing the overhead of an otherwise empty tenant at only a single Range and a few KBs of data.
craig bot
pushed a commit
that referenced
this issue
Jun 4, 2020
49784: config: introduce pseudo "tenants" zone r=nvanbenschoten a=nvanbenschoten Fixes #49318. Fixes #49445. Progress towards #48123. Informs #48774. This commit introduces a new pseudo object ID in the system tenant's namespace called "tenants". Like "liveness" and "timeseries" before it, the pseudo object allows zone configurations to be applied to pseudo-objects that do not live directly in the system tenant's SQL keyspace. In this case, the "tenants" zone allows zone configurations to be set by the system tenant and applied to all other tenants in the system. There may come a time when we want secondary tenants to have more control over their zone configurations, but that will require a much larger change to the zone config structure and UX as a whole. While making this change, we rationalize the rest of zone configuration handling and how it relates to multi-tenancy. Now that secondary tenant ranges have a zone config to call their own, we can make sense of calls from SQL into the zone configuration infrastructure. We gate off calls that don't make sense for secondary tenants and clean up hooks in SQL that handle zone config manipulation. All of this works towards a good cause - we eliminate the remaining uses of `keys.TODOSQLCodec` from `pkg/sql/...` and `pkg/config/...`, bringing us a big step closer towards being able to remove the placeholder and close #48123. This work also reveals that in order to address #48774, we need to be able to determine split points from the SystemConfig. This makes it very difficult to split on secondary tenant object (e.g. table) boundaries. However, it makes it straightforward to split on secondary tenant keysapce boundaries. This is already what we were thinking (see #47907), so out both convenience and desire, I expect that we'll follow this up with a PR that splits Ranges only at secondary tenant boundaries - placing the overhead of an otherwise empty tenant at only a single Range and a few KBs of data. Co-authored-by: Nathan VanBenschoten <[email protected]>
|
The following paragraph in 6953182 was unintentionally interpreted as closing this issue:
Unfortunately, we have not yet completed it. |
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this issue
Jun 5, 2020
Informs cockroachdb#48123. `keys.EnsureSafeSplitKey` itself already knew about tenant prefixes, but `storage.MVCCFindSplitKey` had a bit of extra logic that assumed the system tenant. This commit addresses this and removes the only use of `TODOSQLCodec` in `pkg/storage` while doing so.
|
I'll file sub-issues accordingly and close this. |
This was referenced Jun 14, 2022
|
updated issue description with finer-grain dependencies. Let's keep this issue open to actually remove the codec at the end. |
knz
added
the
C-cleanup
knz
added a commit
to knz/cockroach
that referenced
this issue
Mar 28, 2023
The `TODOSQLCodec` was a bug waiting to happen. The only reasonable remaining purpose is for use in tests. As such, this change moves its definition to a test-only package (we have a linter that verifies that `testutils` is not included in non-test code). This change then identifies the one non-reasonable remaining purposes and identifies it properly as a bug linked to cockroachdb#48123. Release note: None
knz
changed the title
|
Renamed |
craig bot
pushed a commit
that referenced
this issue
Mar 28, 2023
…99752 #99774 99433: opt: fixup CTE stats on placeholder queries r=cucaroach a=cucaroach During optbuilder phase we copy the initial expressions stats into the fake-rel but this value can change when placeholders are assigned so add code in AssignPlaceholders to rebuild the cte if the stats change. Fixes: #99389 Epic: none Release note: none 99516: metrics: improve ux around _status/vars output r=aadityasondhi a=dhartunian Previously, the addition of the `tenant` metric label was applied uniformly and could result in confusion for customers who never enable multi-tenancy or c2c. The `tenant="system"` label carries little meaning when there's no tenancy in use. This change modifies the system tenant label application to only happen when a non- sytem in-process tenant is created. Additionally, an environment variable: `COCKROACH_DISABLE_NODE_AND_TENANT_METRIC_LABELS` can be set to `false` to disable the new `tenant` and `node_id` labels. This can be used on single-process tenants to disable the `tenant` label. Resolves: #94668 Epic: CRDB-18798 Release note (ops change): The `COCKROACH_DISABLE_NODE_AND_TENANT_METRIC_LABELS` env var can be used to disable the newly introduced metric labels in the `_status/vars` output if they conflict with a customer's scrape configuration. 99522: jobsprofiler: store DistSQL diagram of jobs in job info r=dt a=adityamaru This change teaches import, cdc, backup and restore to store their DistSQL plans in the job_info table under a timestamped info key. The generation and writing of the plan diagram is done asynchronously so as to not slow down the execution of the job. A new plan will be stored everytime the job sets up its DistSQL flow. Release note: None Epic: [CRDB-8964](https://cockroachlabs.atlassian.net/browse/CRDB-8964) Informs: #99729 99574: streamingccl: skip acceptance/c2c on remote cluster setup r=stevendanna a=msbutler acceptance/c2c currently fails when run on a remote cluster. This patch ensures the test gets skipped when run on a remote cluster. There's no need to run the test on a remote cluster because the other c2c roachtests provide sufficient coverage. Fixes #99553 Release note: none 99691: codeowners: update sql obs to cluster obs r=maryliag a=maryliag Update mentions of `sql-observability` to `cluster-observability`. Epic: none Release note: None 99712: ui: connect metrics provider to metrics timescale object r=xinhaoz a=dhartunian Previously, the `MetricsDataProvider` component queried the redux store for the `TimeScale` object which contained details of the currently active time window. This piece of state was assumed to update to account for the "live" moving window that metrics show when pre-set lookback time windows are selected. A recent PR: #98331 removed the feature that polled new data from SQL pages, which also disabled polling on metrics pages due to the re-use of `TimeScale`. This commit modifies the `MetricsDataProvider` to instead read the `metricsTime` field of the `TimeScaleState` object. This object was constructed for use by the `MetricsDataProvider` but was not wired up to the component. Resolves #99524 Epic: None Release note: None 99733: telemetry: add FIPS-specific channel r=knz a=rail Previously, all official builds were reporting using the same telemetry channel. This PR adds an new telemetry channel for the FIPS build target. Fixes: CC-24110 Epic: DEVINF-478 Release note: None 99745: spanconfigsqlwatcher: deflake TestSQLWatcherOnEventError r=arulajmani a=arulajmani Previously, this test was setting the no-op checkpoint duration to be every hour to effectively disable checkpoints. Doing so is integral to what the test is testing. However, this was a lie, given how `util.Every` works -- A call to `ShouldProcess` returns true the very first time. This patch achieves the original goal by introducing a new testing knob. Previously, the test would fail in < 40 runs locally. Have this running strong for ~1000 runs. Fixes #76765 Release note: None 99747: roachtest: use persistent disks for disk-stall tests r=jbowens a=nicktrav Currently, the `disk-stall` tests use local SSDs. When run on GCE VMs, a higher test flake rate is observed due to known issues with fsync latency for local SSDs. Switch the test to use persistent disks instead. Touches: #99372. Release note: None. Epic: CRDB-20293 99752: kvserver: bump tolerance more r=ajwerner a=ajwerner I'm not digging into this more, but the test is flakey. Epic: none https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_UnitTests_BazelUnitTests/9161972?showRootCauses=false&expandBuildChangesSection=true&expandBuildProblemsSection=true&expandBuildTestsSection=true Release note: None 99774: *: identify remaining uses of TODOSQLCodec r=stevendanna a=knz The `TODOSQLCodec` was a bug waiting to happen. The only reasonable remaining purpose is for use in tests. As such, this change moves its definition to a test-only package (we have a linter that verifies that `testutils` is not included in non-test code). This change then identifies the one non-reasonable remaining purposes and identifies it properly as a bug linked to #48123. Release note: None Epic: None Co-authored-by: Tommy Reilly <[email protected]> Co-authored-by: David Hartunian <[email protected]> Co-authored-by: adityamaru <[email protected]> Co-authored-by: Michael Butler <[email protected]> Co-authored-by: maryliag <[email protected]> Co-authored-by: Rail Aliiev <[email protected]> Co-authored-by: Arul Ajmani <[email protected]> Co-authored-by: Nick Travers <[email protected]> Co-authored-by: ajwerner <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]>
This was referenced Jun 13, 2023
craig bot
pushed a commit
that referenced
this issue
Jun 14, 2023
104820: backupccl: adjust a test to run for secondary tenant codec too r=yuzefovich a=yuzefovich Fixes: #82882. Release note: None 104823: row: remove leftover reference to TestingSQLCodec in a test r=yuzefovich a=yuzefovich This was missed in #82890. Addresses: #48123. Epic: None Release note: None 104828: base: remove OptionalNodeIDErr r=yuzefovich a=yuzefovich This method was only used in one place where we need to get the SQL instance ID of the gateway. That place has been refactored to pass that ID explicitly from the DistSQLPlanner. Addresses: #100826. Epic: None Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]>
craig bot
pushed a commit
that referenced
this issue
Jun 14, 2023
104823: row: remove leftover reference to TestingSQLCodec in a test r=yuzefovich a=yuzefovich This was missed in #82890. Addresses: #48123. Epic: None Release note: None 104893: build: upgrade go to 1.19.10 r=srosenberg a=rail * [ ] Adjust the Pebble tests to run in new version. * [x] Adjust version in Docker image ([source](./builder/Dockerfile)). * [x] Adjust version in the TeamCity agent image ([setup script](./packer/teamcity-agent.sh)) * [x] Rebuild and push the Docker image (following [Basic Process](#basic-process)) * [x] Update `build/teamcity/internal/release/build-and-publish-patched-go/impl.sh` with the new version and adjust SHA256 sums as necessary. * [x] Adjust `GO_VERSION` and `GO_FIPS_COMMIT` for the FIPS Go toolchain ([source](./teamcity/internal/release/build-and-publish-patched-go/impl-fips.sh)). * [x] Run the `Internal / Cockroach / Build / Toolchains / Publish Patched Go for Mac` build configuration in TeamCity with your latest version of the script above. Note the job depends on another job `Build and Publish Patched Go`. That job prints out the SHA256 of all tarballs, which you will need to copy-paste into `WORKSPACE` (see below). `Publish Patched Go for Mac` is an extra step that publishes the *signed* `go` binaries for macOS. That job also prints out the SHA256 of the Mac tarballs in particular. * [x] Adjust `--`@io_bazel_rules_go//go/toolchain:sdk_version`` in [.bazelrc](../.bazelrc). * [x] Bump the version in `WORKSPACE` under `go_download_sdk`. You may need to bump [rules_go](https://github.com/bazelbuild/rules_go/releases). Also edit the filenames listed in `sdks` and update all the hashes to match what you built in the step above. * [x] Bump the version in `WORKSPACE` under `go_download_sdk` for the FIPS version of Go (`go_sdk_fips`). * [x] Run `./dev generate bazel` to refresh `distdir_files.bzl`, then `bazel fetch `@distdir//:archives`` to ensure you've updated all hashes to the correct value. * [x] Bump the version in `builder.sh` accordingly ([source](./builder.sh#L6)). * [ ] Bump the version in `go-version-check.sh` ([source](./go-version-check.sh)), unless bumping to a new patch release. * [ ] Bump the go version in `go.mod`. * [x] Bump the default installed version of Go in `bootstrap-debian.sh` ([source](./bootstrap/bootstrap-debian.sh)). * [x] Replace other mentions of the older version of go (grep for `golang:<old_version>` and `go<old_version>`). * [ ] Update the `builder.dockerImage` parameter in the TeamCity [`Cockroach`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Cockroach&tab=projectParams) and [`Internal`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Internal&tab=projectParams) projects. * [ ] Ask the Developer Infrastructure team to deploy new TeamCity agent images according to [packer/README.md](./packer/README.md) Epic: none Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Rail Aliiev <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We want to remove uses of
keysutils.TestingSQLCodec(previouslykeys.TODOSQLCodec) then remove that codec altogether.Dependencies:
Feature work:
cockroach debug backup exportbackupccl:debug backup exportnot tested (maybe not working) for tenant backups #82878FIXMEIDONTKNOWWHICHCODECTOUSEin *: identify remaining uses of TODOSQLCodec #99774Test code:
assertEqualKVs()#82883Jira issue: CRDB-4356
Epic: CRDB-26091
The text was updated successfully, but these errors were encountered: