Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/sql: remove usages of createTestServerParams to increase tenant testing coverage #140446

Open
Tracked by #138912
shubhamdhama opened this issue Feb 4, 2025 · 3 comments · May be fixed by #143115
Open
Tracked by #138912

pkg/sql: remove usages of createTestServerParams to increase tenant testing coverage #140446

shubhamdhama opened this issue Feb 4, 2025 · 3 comments · May be fixed by #143115
Assignees
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-db-server target-release-25.2.0

Comments

@shubhamdhama
Copy link
Contributor

shubhamdhama commented Feb 4, 2025

This issue is part of our effort to ensure all tests run with tenant testing, with a focus on the tests in pkg/sql/.

Many of these tests use createTestServerParams, which initializes with tenancy disabled via base.TODOTestTenantDisabled. We should either remove all instances of this or identify missing features in the multi-tenant model.

Part of #138912

Epic: CRDB-48564

Jira issue: CRDB-47162

@shubhamdhama shubhamdhama added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-db-server labels Feb 4, 2025
shubhamdhama added a commit to shubhamdhama/cockroach that referenced this issue Feb 4, 2025
To achieve this, this PR reduces the usage of `createTestServerParams`
by replacing it with `createTestServerParamsAllowTenants`. The changes
are mostly straightforward, including replacing hardcoded
`keys.SystemSQLCodec` with `server.Codec()`, and using
`server.StorageLayer()`/`server.SystemLayer()` where the test is
specific to the system tenant.

Informs: cockroachdb#140446
Epic: CRDB-38970
Release note: None
shubhamdhama added a commit to shubhamdhama/cockroach that referenced this issue Feb 10, 2025
To achieve this, this PR reduces the usage of `createTestServerParams`
by replacing it with `createTestServerParamsAllowTenants`. The changes
are mostly straightforward, including replacing hardcoded
`keys.SystemSQLCodec` with `server.Codec()`, and using
`server.StorageLayer()`/`server.SystemLayer()` where the test is
specific to the system tenant.

Informs: cockroachdb#140446
Epic: CRDB-38970
Release note: None
craig bot pushed a commit that referenced this issue Feb 11, 2025
140447: sql: enable tenant testing for more tests r=rafiss a=shubhamdhama

To achieve this, this PR reduces the usage of `createTestServerParams` by replacing it with `createTestServerParamsAllowTenants`. The changes are mostly straightforward, including replacing hardcoded `keys.SystemSQLCodec` with `server.Codec()`, and using `server.StorageLayer()`/`server.SystemLayer()` where the test is specific to the system tenant.

Informs: #140446
Epic: CRDB-38970
Release note: None

Co-authored-by: Shubham Dhama <[email protected]>
shubhamdhama added a commit to shubhamdhama/cockroach that referenced this issue Feb 12, 2025
Previously all tests in this file were disabled for multitenancy.

(Continuation of cockroachdb#140447 for `schema_changer_test.go`)

Informs: cockroachdb#140446
Epic: CRDB-38970
Release note: None
craig bot pushed a commit that referenced this issue Feb 14, 2025
141213: sql: enable tenant testing for schema_changer tests r=cthumuluru-crdb,rafiss a=shubhamdhama

Previously all tests in this file were disabled for multitenancy.

(Continuation of #140447 for `schema_changer_test.go`)

Informs: #140446
Epic: CRDB-38970
Release note: None

141392: sqlstats: Reuse sessionphase.Times on stats collector r=xinhaoz a=xinhaoz

StatsCollector.Reset shows up in sqlstats section of cpu/mem
profiles. This seemed to mostly be allocations of new session.PhaseTimes
objects. This commit avoids new allocations of session.PhaseTimes
when resetting the stats collector.

Epic: none

Release note: None

```
name                                           old time/op    new time/op    delta
Sysbench/SQL/3node/oltp_read_write-24            13.2ms ± 2%    13.0ms ± 1%  -1.69%  (p=0.001 n=10+10)
ParallelSysbench/SQL/3node/oltp_read_write-24    1.22ms ± 3%    1.22ms ± 1%    ~     (p=0.315 n=10+10)

name                                           old errs/op    new errs/op    delta
Sysbench/SQL/3node/oltp_read_write-24              0.00           0.00         ~     (all equal)
ParallelSysbench/SQL/3node/oltp_read_write-24      0.01 ±20%      0.01 ±52%    ~     (p=0.231 n=9+10)

name                                           old alloc/op   new alloc/op   delta
Sysbench/SQL/3node/oltp_read_write-24            2.18MB ± 1%    2.18MB ± 1%    ~     (p=0.666 n=9+9)
ParallelSysbench/SQL/3node/oltp_read_write-24    2.03MB ± 1%    2.03MB ± 1%    ~     (p=0.971 n=10+10)

name                                           old allocs/op  new allocs/op  delta
Sysbench/SQL/3node/oltp_read_write-24             10.5k ± 2%     10.5k ± 2%    ~     (p=0.529 n=10+10)
ParallelSysbench/SQL/3node/oltp_read_write-24     9.16k ± 2%     9.17k ± 2%    ~     (p=0.853 n=10+10)
```


141446: backup: fix panic on encrypted incremental after non-encrypted backup r=msbutler a=kev-cao

When attempting an encrypted backup on a non-encrypted backup chain, an error should be surfaced to the user indicating an error and inability to do so. Due to a missing error check, this currently panics.

Epic: none

Release note: None

Co-authored-by: Shubham Dhama <[email protected]>
Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: Kevin Cao <[email protected]>
@shubhamdhama shubhamdhama changed the title sql: improve tenant testing coverage sql: increase tenant testing coverage of unit tests Feb 17, 2025
@shubhamdhama

This comment has been minimized.

@shubhamdhama

This comment has been minimized.

shubhamdhama added a commit to shubhamdhama/cockroach that referenced this issue Feb 20, 2025
This change is in a separate PR because updating `TestValidSetShowZones` to
use `createTestServerParamsAllowTenants` instead of
`createTestServerParams` required some investigation. It turned out simpler
than expected. Zone settings for meta ranges aren't supported for secondary
tenants, so this change simply exclude them from setup and validating.

Informs: cockroachdb#140446
Epic: CRDB-38970
Release note: None
annrpom pushed a commit to annrpom/cockroach that referenced this issue Feb 20, 2025
To achieve this, this PR reduces the usage of `createTestServerParams`
by replacing it with `createTestServerParamsAllowTenants`. The changes
are mostly straightforward, including replacing hardcoded
`keys.SystemSQLCodec` with `server.Codec()`, and using
`server.StorageLayer()`/`server.SystemLayer()` where the test is
specific to the system tenant.

Informs: cockroachdb#140446
Epic: CRDB-38970
Release note: None
annrpom pushed a commit to annrpom/cockroach that referenced this issue Feb 20, 2025
Previously all tests in this file were disabled for multitenancy.

(Continuation of cockroachdb#140447 for `schema_changer_test.go`)

Informs: cockroachdb#140446
Epic: CRDB-38970
Release note: None
shubhamdhama added a commit to shubhamdhama/cockroach that referenced this issue Feb 21, 2025
(Stacked on cockroachdb#141490.)

This PR continues the work from cockroachdb#140447, replacing occurrences of
`createTestServerParams` with `createTestServerParamsAllowTenants`
to enable tenant testing in these tests.

Informs: cockroachdb#140446
Epic: CRDB-38970
Release note: None
craig bot pushed a commit that referenced this issue Feb 21, 2025
141604: sql: increase tenant testing coverage r=rafiss,yuzefovich a=shubhamdhama

This PR continues the work from #140447, replacing occurrences of
`createTestServerParams` with `createTestServerParamsAllowTenants`
to enable tenant testing in these tests.

Informs: #140446
Epic: CRDB-38970
Release note: None


141833: upgrades: report progress during 25.1 upgrade jobs backfill r=dt a=dt

Release note: none.
Epic: none.

Co-authored-by: Shubham Dhama <[email protected]>
Co-authored-by: David Taylor <[email protected]>
@shubhamdhama
Copy link
Contributor Author

Progress by this week,

File Total Done Status Notes
pkg/sql/as_of_test.go 2 2
pkg/sql/comment_on_column_test.go 5 5
pkg/sql/conn_executor_test.go 3 3
pkg/sql/crdb_internal_test.go 4 4
pkg/sql/create_test.go 1 1
pkg/sql/delete_preserving_index_test.go 4 4
pkg/sql/descriptor_mutation_test.go 7 7
pkg/sql/err_count_test.go 2 2
pkg/sql/explain_test.go 1 1
pkg/sql/grant_revoke_test.go 2 2
pkg/sql/grant_role_test.go 1 1
pkg/sql/index_mutation_test.go 1 0
pkg/sql/indexbackfiller_test.go 1 1
pkg/sql/internal_test.go 8 8
pkg/sql/jobs_profiler_execution_details_test.go 3 0
pkg/sql/materialized_view_test.go 4 4
pkg/sql/metric_test.go 4 4
pkg/sql/mutation_test.go 2 2
pkg/sql/mvcc_backfiller_test.go 6 5
pkg/sql/privileged_accessor_test.go 1 1
pkg/sql/run_control_test.go 1 1
pkg/sql/schema_changer_test.go 76 76
pkg/sql/show_create_all_tables_builtin_test.go 1 1
pkg/sql/show_test.go 5 5
pkg/sql/sort_test.go 1 1
pkg/sql/split_test.go 1 1
pkg/sql/table_ref_test.go 1 1
pkg/sql/txn_restart_test.go 13 0
pkg/sql/type_change_test.go 7 7
pkg/sql/unsplit_range_test.go 1 0
pkg/sql/unsplit_test.go 1 1
pkg/sql/zone_config_test.go 3 0
pkg/sql/zone_test.go 3 2
Total 177 142

sambhav-jain-16 pushed a commit to sambhav-jain-16/cockroach that referenced this issue Mar 10, 2025
(Stacked on cockroachdb#141490.)

This PR continues the work from cockroachdb#140447, replacing occurrences of
`createTestServerParams` with `createTestServerParamsAllowTenants`
to enable tenant testing in these tests.

Informs: cockroachdb#140446
Epic: CRDB-38970
Release note: None
shubhamdhama added a commit to shubhamdhama/cockroach that referenced this issue Mar 12, 2025
Updating `TestValidSetShowZones` to use
`createTestServerParamsAllowTenants` instead of `createTestServerParams`
required some investigation. It turned out simpler than expected. Zone
settings for meta ranges aren't supported for secondary tenants, so this
change simply exclude them from setup and validating.

Informs: cockroachdb#140446
Epic: CRDB-38970
Release note: None
@shubhamdhama shubhamdhama changed the title sql: increase tenant testing coverage of unit tests pkg/sql: remove usages of createTestServerParams to increase tenant testing coverage Mar 13, 2025
craig bot pushed a commit that referenced this issue Mar 13, 2025
141735: sql: enable tenant testing for zone tests r=rafiss a=shubhamdhama

`` sql: enable tenant testing for `zone_test.go` ``

Updating `TestValidSetShowZones` to use
`createTestServerParamsAllowTenants` instead of `createTestServerParams`
required some investigation. It turned out simpler than expected. Zone
settings for meta ranges aren't supported for secondary tenants, so this
change simply exclude them from setup and validating.

Informs: #140446
Epic: CRDB-48357
Release note: None

`` sql: enable tenant testing for `zone_config_test` ``

Since `GetSpanConfigForKey` is only available in system tenant, it is not
used when running under secondary tenants.

142186: kvserver: mark replica as unavailable if leaderless for a long time r=iskettaneh a=iskettaneh

This PR marks the replica as unavailable in if it has been leaderless for a duration above:
`kv.replica_raft.leaderless_unavailable_threshold`. This helps requests to bail early on unavailable ranges without relying on the replica circuit breaker to trip. This has multiple benefits:

1) Faster reaction time than the replica circuit breaker: If two nodes fail (assuming R=3), many ranges will become unavailable. With the replica circuit breaker, a scan query will need to trip the circuit
breaker for multiple replicas, causing added delays. However, with this approach, the replica will basically become unavailable on the tick path (rather than on the request path). Meaning that a scan query wouldn't need to trip one circuit breaker after the other.

2) Lighter weight: Instead of relying on the replica circuit breaker to test the replication pipeline before it marks the range as
available again, this approach relies on the Raft signal to know when there is a leader again, indicating that the range is
available again.

3) With leader leases, a replica won't propose a lease if it's not the leader. This means that with leader leases, the replica circuit breaker might not trip if the range have lost quorum. However, with this commit, the replica will eventually forget who the leader was, and eventually the leaderlessWatcher would mark it as unavailable.

Fixes: #139638

Release note: None

142489: row, sql: implement mutations on vector indexes r=mw5h a=mw5h

This patch plumbs the output of the vector search operators into rowenc for encoding into vector indexes. The output of these vector search operators is included in the row values for mutation operators after partial index values and are plumbed into a new vector index update helper, which tracks the column values until they're needed by rowenc.

While we're here, we homogonize how
pkg/sql/{delete,insert,update,upsert}.go consume row values, hopefully improving legibility.

Epic: CRDB-42943
Release note: None

142829: sql/catalog: fix object renames for PCR reader catalogs r=fqazi a=fqazi

Previously, the PCR reader catalog would only delete namespace entries for a descriptor if it was not modified. This meant the reader catalog logic could leave behind stale entries in the system.namespace table after a object was renamed. To address this, this patch detects if an object is renamed, and allows the old namespace entry to be deleted.

Fixes: #142828

Release note (bug fix): PCR reader catalogs could have orphan rows in system.namespace after a object is renamed.

142836: kvserver: deflake TestLeasePreferencesDuringOutage r=kvoli a=arulajmani

There was a race here, when heartbeating node liveness epochs, where another node could increment our epoch. This patch retries in such situations.

Fixes #142795

Release note: None

Co-authored-by: Shubham Dhama <[email protected]>
Co-authored-by: Ibrahim Kettaneh <[email protected]>
Co-authored-by: Matt White <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>
craig bot pushed a commit that referenced this issue Mar 13, 2025
141735: sql: enable tenant testing for zone tests r=rafiss a=shubhamdhama

`` sql: enable tenant testing for `zone_test.go` ``

Updating `TestValidSetShowZones` to use
`createTestServerParamsAllowTenants` instead of `createTestServerParams`
required some investigation. It turned out simpler than expected. Zone
settings for meta ranges aren't supported for secondary tenants, so this
change simply exclude them from setup and validating.

Informs: #140446
Epic: CRDB-48357
Release note: None

`` sql: enable tenant testing for `zone_config_test` ``

Since `GetSpanConfigForKey` is only available in system tenant, it is not
used when running under secondary tenants.

142684: changefeedccl: optimize enriched source provider r=andyyang890 a=asg0451

Optimize enriched source provider by avoiding unnecessary
allocations and json construction.

Fixes: #141798

Release note: None


142836: kvserver: deflake TestLeasePreferencesDuringOutage r=kvoli a=arulajmani

There was a race here, when heartbeating node liveness epochs, where another node could increment our epoch. This patch retries in such situations.

Fixes #142795

Release note: None

Co-authored-by: Shubham Dhama <[email protected]>
Co-authored-by: Miles Frankel <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>
craig bot pushed a commit that referenced this issue Mar 18, 2025
142391: sql: increase tenant testing coverage r=rafiss,msbutler a=shubhamdhama

`` sql: increase tenant testing coverage for `jobs_profiler_execution_details_test` ``


`` sql: increase tenant testing coverage for `mvcc_backfiller_test` ``


`` sql: increase tenant testing coverage for `txn_restart_test` ``


`` sql: enable shared-process mode tenant testing for `TestUnsplitRanges` ``

I opened issue #142388 to track the work for enabling external-process mode
testing, as it turned out to be a non-trivial effort.

Informs: #140446
Epic: CRDB-48564
Release note: None


Co-authored-by: Shubham Dhama <[email protected]>
shubhamdhama added a commit to shubhamdhama/cockroach that referenced this issue Mar 19, 2025
The only remaining use of `createTestServerParams` was in
`TestIndexMutationKVOps`, but that test is really something the SQL queries
team better suited to handle. So, I swapped it out for
`createTestServerParamsAllowTenants` and override `DefaultTestTenant` with
`TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet` (tracked in cockroachdb#143114).

Now that's taken care of, `createTestServerParams` is fully gone—one less
way for test authors to accidentally skip running tests with tenants.

Next step: we should rename `createTestServerParamsAllowTenants` back
to `createTestServerParams` to keep things tidy.

Fixes: cockroachdb#140446
Epic: CRDB-48564
Release note: none
@shubhamdhama shubhamdhama linked a pull request Mar 19, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-db-server target-release-25.2.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant