cockroachdb / cockroachdb/cockroach

*: re-audit uses of `ForSystemTenant`

Open
#116,445 0 comments 0 reactions 0 assignees View on GitHub
C-bug
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Describe the problem**

Each call to `ForSystemTenant` represents a potential feature gap or bug in UA/MT. As of 3e4643e6b03e865d2e4300acced60c22e7e277aa there are 125 of these in non-test files.

The following issues were a result of manually scanning the list of these call sites and making a judgement about whether they might represent a problem.

**Major Concerns**

- [x] https://github.com/cockroachdb/cockroach/issues/116534 (target fix: **??**)
- `pkg/sql/distsql_physical_planner.go:4855:133: distribute := distributionType == DistributionTypeAlways || (distributionType == DistributionTypeSystemTenantOnly && evalCtx.Codec.ForSystemTenant())`
- https://github.com/cockroachdb/cockroach/pull/116559
- [ ] https://github.com/cockroachdb/cockroach/issues/116536 (target fix: **23.2.1**)
- `pkg/sql/index_split_scatter.go:55:15: if !is.codec.ForSystemTenant() || indexToBackfill.IsTemporaryIndexForBackfill() {`
- `pkg/sql/importer/import_processor_planning.go:217:19: if evalCtx.Codec.ForSystemTenant() {`
- `pkg/sql/backfill.go:2242:22: if sc.execCfg.Codec.ForSystemTenant() {`
- [x] https://github.com/cockroachdb/cockroach/issues/116539 (target fix: **23.2.1**)
- `pkg/sql/schema_changer.go:3100:36: if codec := execCfg.Codec; codec.ForSystemTenant() {`
- https://github.com/cockroachdb/cockroach/pull/116547
- [x] https://github.com/cockroachdb/cockroach/pull/116312 (target fix: **23.2.0**)
- `pkg/sql/gcjob/gc_job.go:203:20: if !execCfg.Codec.ForSystemTenant() {`
- `pkg/sql/gcjob/gc_job.go:228:20: if !execCfg.Codec.ForSystemTenant() {`

**Minor Concerns**
- [x] https://github.com/cockroachdb/cockroach/issues/116529
- `pkg/ccl/backupccl/restore_job.go:2549:12: if codec.ForSystemTenant() {`
- https://github.com/cockroachdb/cockroach/pull/116555
- [ ] https://github.com/cockroachdb/cockroach/issues/116537
- `pkg/sql/conn_executor_exec.go:2754:79: AllowSessionRevival.Get(&ex.server.cfg.Settings.SV) && !ex.server.cfg.Codec.ForSystemTenant(),`
- `pkg/sql/session_revival_token.go:43:75: AllowSessionRevival.Get(&p.ExecCfg().Settings.SV) && !p.ExecCfg().Codec.ForSystemTenant(),`
- `pkg/sql/session_revival_token.go:79:77: if !AllowSessionRevival.Get(&p.ExecCfg().Settings.SV) || p.ExecCfg().Code`
- [ ] `pkg/sql/conn_executor.go:3925:26: if !ex.server.cfg.Codec.ForSystemTenant() ||` (the comment suggests that it was written with ephemeral SQL pods of serverless in mind, and now it will apply in UA too)
- [ ] `pkg/sql/temporary_schema.go:476:13: if c.codec.ForSystemTenant() {`: The comment says there is no harm here, but I wonder if that comment considered the UA case.
- [x] `pkg/sql/explain_bundle.go:69:26: } else if execCfg.Codec.ForSystemTenant() {`
- Yahor: this is only a very minor UX difference where in system tenant we get a direct link to download a stmt bundle whereas in the secondary tenant we'll need to navigate to it on the UI or use the SQL / CLI command
- [x] `pkg/server/server_sql.go:1317:11: if codec.ForSystemTenant() {`
- Yahor: currently, the key visualizer page is only implemented for system tenant, so in UA users will need to switch there to see it. Also, this feature is disabled by default, so it seems like a pretty minor limitation.

**Call sites not believed to be a problem**

This list out of order because of concurrent updates.

- [x] `pkg/sql/distsql_physical_planner.go:1853:19: return dsp.codec.ForSystemTenant() && planCtx.localityFilter.Empty()`
- [x] `pkg/sql/gcjob/refresh_statuses.go:323:20: if execCfg.Codec.ForSystemTenant() &&`
- [x] `pkg/sql/gcjob/refresh_statuses.go:411:20: if !execCfg.Codec.ForSystemTenant() {`
- [x] `pkg/sql/set_cluster_setting.go:134:39: forSystemTenant := p.ExecCfg().Codec.ForSystemTenant()`
- [x] `pkg/sql/set_cluster_setting.go:337:32: params.extendedEvalCtx.Codec.ForSystemTenant(),`
- [x] `pkg/sql/set_var.go:68:93: if _, ok, _ := settings.LookupForLocalAccess(settings.SettingName(name), p.ExecCfg().Codec.ForSystemTenant()); ok {`
- [x] `pkg/sql/conn_executor_exec.go:1884:30: if !planner.ExecCfg().Codec.ForSystemTenant() {`
- [x] `pkg/sql/tenant_accessors.go:42:12: if !codec.ForSystemTenant() {`
- [x] `pkg/sql/tenant_settings.go:55:22: if !p.execCfg.Codec.ForSystemTenant() {`
- [x] `pkg/sql/tenant_settings.go:180:83: setting, ok, nameStatus := settings.LookupForLocalAccess(name, p.ExecCfg().Codec.ForSystemTenant())`
- [x] `pkg/sql/tenant_settings.go:191:22: if !p.execCfg.Codec.ForSystemTenant() {`
- [x] `pkg/sql/crdb_internal.go:2362:53: for _, k := range settings.Keys(p.ExecCfg().Codec.ForSystemTenant()) {`
- [x] `pkg/sql/crdb_internal.go:2367:74: setting, _ := settings.LookupForLocalAccessByKey(k, p.ExecCfg().Codec.ForSystemTenant())`
- [x] `pkg/sql/alter_database.go:583:22: if p.execCfg.Codec.ForSystemTenant() {`
- [x] `pkg/sql/distsql_plan_bulk.go:48:15: if dsp.codec.ForSystemTenant() {`
- [x] `pkg/sql/delegate/show_all_cluster_settings.go:102:22: if !d.evalCtx.Codec.ForSystemTenant() {`
- [x] `pkg/sql/catalog/descs/txn.go:148:23: if !descsCol.codec().ForSystemTenant() {`
- [x] `pkg/sql/catalog/bootstrap/initial_values.go:109:17: if !opts.Codec.ForSystemTenant() {`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:101:17:// AddDescriptorForSystemTenant is like AddDescriptor but only for the system`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:103:40:func (ms *MetadataSchema) AddDescriptorForSystemTenant(desc catalog.Descriptor) {`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:104:15: if !ms.codec.ForSystemTenant() {`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:113:14: if ms.codec.ForSystemTenant() {`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:174:15: if ms.codec.ForSystemTenant() {`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:176:47: // until clusterversion.V23_1DescIDSequenceForSystemTenant is removed.`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:238:14: if ms.codec.ForSystemTenant() {`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:353:12: if !codec.ForSystemTenant() {`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:404:22: target.AddDescriptorForSystemTenant(systemschema.TenantsTable)`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:451:22: target.AddDescriptorForSystemTenant(systemschema.TenantUsageTable)`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:453:22: target.AddDescriptorForSystemTenant(systemschema.SpanConfigurationsTable)`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:457:22: target.AddDescriptorForSystemTenant(systemschema.TenantSettingsTable)`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:471:22: target.AddDescriptorForSystemTenant(systemschema.SystemTaskPayloadsTable)`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:472:22: target.AddDescriptorForSystemTenant(systemschema.SystemTenantTasksTable)`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:475:22: target.AddDescriptorForSystemTenant(systemschema.TenantIDSequence)`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:485:55: // If adding a call to AddDescriptor or AddDescriptorForSystemTenant, please`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:486:38: // bump the value of NumSystemTablesForSystemTenant below. This constant is`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:492:19:// NumSystemTablesForSystemTenant is the number of system tables defined on`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:495:22:const NumSystemTablesForSystemTenant = 55`
- [x] `pkg/sql/catalog/bootstrap/metadata.go:528:12: if !codec.ForSystemTenant() {`
- [x] `pkg/sql/descriptor.go:294:22: if !p.execCfg.Codec.ForSystemTenant() && isSystemDatabase {`
- [x] `pkg/sql/descriptor.go:361:22: if !p.execCfg.Codec.ForSystemTenant() &&`
- [x] `pkg/sql/explain_plan.go:223:13: if !codec.ForSystemTenant() {`
- [x] `pkg/sql/set_zone_config.go:544:30: if !params.p.execCfg.Codec.ForSystemTenant() {`
- [x] `pkg/sql/set_zone_config.go:1040:19: if execCfg.Codec.ForSystemTenant() {`
- [x] `pkg/sql/set_zone_config.go:1045:40: return validateZoneAttrsAndLocalitiesForSystemTenant(ctx, ss.ListNodesInternal, currentZone, newZone)`
- [x] `pkg/sql/set_zone_config.go:1052:34:// validateZoneAttrsAndLocalitiesForSystemTenant performs constraint/ lease`
- [x] `pkg/sql/set_zone_config.go:1065:36:func validateZoneAttrsAndLocalitiesForSystemTenant(`
- [x] `pkg/sql/restricted_system_interface.go:55:27: forSystemTenant := codec.ForSystemTenant()`
- [x] `pkg/sql/restricted_system_interface.go:86:23: if p.ExecCfg().Codec.ForSystemTenant() &&`
- [x] `pkg/sql/distsql_spec_exec_factory.go:72:41: singleTenant: p.execCfg.Codec.ForSystemTenant(),`
- [x] `pkg/sql/regions/region_provider.go:71:13: if p.codec.ForSystemTenant() {`
- [x] `pkg/sql/exec_util.go:3868:11: if codec.ForSystemTenant() || setting.Get(&settings.SV) {`
- [x] `pkg/server/server_systemlog_gc.go:201:45: forSystemTenant := sqlServer.execCfg.Codec.ForSystemTenant()`
- [x] `pkg/server/settingswatcher/settings_watcher.go:347:72: setting, ok := settings.LookupForLocalAccessByKey(settingKey, s.codec.ForSystemTenant())`
- [x] `pkg/server/settingswatcher/settings_watcher.go:352:14: if !s.codec.ForSystemTenant() {`
- [x] `pkg/server/settingswatcher/settings_watcher.go:440:57: if key == clusterversion.KeyVersionSetting && !s.codec.ForSystemTenant() {`
- [x] `pkg/server/settingswatcher/settings_watcher.go:640:66: setting, ok := settings.LookupForLocalAccessByKey(key, settings.ForSystemTenant)`
- [x] `pkg/server/api_v2.go:203:57: if !route.tenantEnabled && !a.sqlServer.execCfg.Codec.ForSystemTenant() {`
- [x] `pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go:248:14: if !s.codec.ForSystemTenant() && name != zonepb.DefaultZoneName {`
- [x] `pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go:361:15: if !s.codec.ForSystemTenant() {`
- [x] `pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go:586:13: if s.codec.ForSystemTenant() {`
- [x] `pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go:603:14: if !s.codec.ForSystemTenant() {`
- [x] `pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go:663:48: if !s.knobs.ConfigureScratchRange || !s.codec.ForSystemTenant() {`
- [x] `pkg/spanconfig/spanconfigreconciler/reconciler.go:305:14: if !f.codec.ForSystemTenant() {`
- [x] `pkg/spanconfig/spanconfigreconciler/reconciler.go:345:13: if f.codec.ForSystemTenant() {`
- [x] `pkg/spanconfig/spanconfigreconciler/reconciler.go:630:15: if r.codec.ForSystemTenant() {`
- [x] `pkg/sql/show_cluster_setting.go:79:33: } else if !p.execCfg.Codec.ForSystemTenant() {`
- [x] `pkg/upgrade/upgrademanager/manager.go:122:11: if codec.ForSystemTenant() {`
- [x] `pkg/upgrade/upgrademanager/manager.go:184:34: if isSystemUpgrade && !m.codec.ForSystemTenant() {`
- [x] `pkg/upgrade/upgrademanager/manager.go:410:38: mustPersistFenceVersion := !m.codec.ForSystemTenant()`
- [x] `pkg/upgrade/upgrademanager/manager.go:676:35: if isSystemMigration && !m.codec.ForSystemTenant() {`
- [x] `pkg/sql/pgwire/auth_methods.go:669:75: if !sql.AllowSessionRevival.Get(&execCfg.Settings.SV) || execCfg.Codec.ForSystemTenant() {`
- [x] `pkg/settings/registry.go:447:4:// ForSystemTenant can be passed to Lookup for code that runs only on the system`
- [x] `pkg/settings/registry.go:449:7:const ForSystemTenant = true`
- [x] `pkg/sql/show_cluster_setting.go:163:83: setting, ok, nameStatus := settings.LookupForLocalAccess(name, p.ExecCfg().Codec.ForSystemTenant())`
- [x] `pkg/spanconfig/spanconfigstore/span_store.go:163:81: s.settings.Version.IsActive(ctx, clusterversion.V23_2_EnableRangeCoalescingForSystemTenant)) {`
- [x] `pkg/server/server_sql.go:578:11: if codec.ForSystemTenant() {`
- [x] `pkg/server/server_sql.go:623:70: canUseNodeDialerAsSQLInstanceDialer := isMixedSQLAndKVNode && codec.ForSystemTenant()`
- [x] `pkg/server/server_sql.go:770:11: if codec.ForSystemTenant() {`
- [x] `pkg/server/server_sql.go:1041:11: if codec.ForSystemTenant() {`
- [x] `pkg/server/server_sql.go:1239:12: if codec.ForSystemTenant() {`
- [x] `pkg/server/server_sql.go:1550:22: if !s.execCfg.Codec.ForSystemTenant() {`
- [x] `pkg/server/server_sql.go:1591:21: if s.execCfg.Codec.ForSystemTenant() {`
- [x] `pkg/server/server_sql.go:1716:22: if !s.execCfg.Codec.ForSystemTenant() && (s.serviceMode != mtinfopb.ServiceModeExternal) {`
- [x] `pkg/cmd/github-pull-request-make/testdata/five_tests.diff:1937:38:@@ -27,7 +28,7 @@ func descIDSequenceForSystemTenant(`
- [x] `pkg/sql/sem/builtins/builtins.go:4949:73: setting, ok, _ := settings.LookupForLocalAccess(name, evalCtx.Codec.ForSystemTenant())`
- [x] `pkg/sql/sem/builtins/builtins.go:4979:73: setting, ok, _ := settings.LookupForLocalAccess(name, evalCtx.Codec.ForSystemTenant())`
- [x] `pkg/clusterversion/cockroach_versions.go:219:32: // V23_2_EnableRangeCoalescingForSystemTenant enables range coalescing for`
- [x] `pkg/clusterversion/cockroach_versions.go:221:29: V23_2_EnableRangeCoalescingForSystemTenant`
- [x] `pkg/clusterversion/cockroach_versions.go:346:29: V23_2_EnableRangeCoalescingForSystemTenant: {Major: 23, Minor: 1, Internal: 8},`
- [x] `pkg/server/server_controller_orchestration_method.go:112:20: // makeServerStateForSystemTenant returns a serverState for the`
- [x] `pkg/server/server_controller_orchestration_method.go:114:17: makeServerStateForSystemTenant(nc *roachpb.TenantNameContainer, systemSrv orchestratedServer) serverState`
- [x] `pkg/server/server_controller.go:142:64: catconstants.SystemTenantName: c.orchestrator.makeServerStateForSystemTenant(systemTenantNameContainer, systemServer),`
- [x] `pkg/server/admin.go:2024:42: showSystem := s.sqlServer.execCfg.Codec.ForSystemTenant()`
- [x] `pkg/server/admin.go:2027:21: target = settings.ForSystemTenant`
- [x] `pkg/server/server_controller_channel_orchestrator.go:149:19:// makeServerStateForSystemTenant is part of the orchestrator interface.`
- [x] `pkg/server/server_controller_channel_orchestrator.go:150:46:func (o *channelOrchestrator) makeServerStateForSystemTenant(`
- [x] `pkg/ccl/backupccl/restore_planning.go:1924:25: if !p.ExecCfg().Codec.ForSystemTenant() {`
- [x] `pkg/ccl/backupccl/restore_planning.go:2358:23: if p.ExecCfg().Codec.ForSystemTenant() ||`
- [x] `pkg/ccl/backupccl/restore_planning.go:2390:24: if !p.ExecCfg().Codec.ForSystemTenant() &&`
- [x] `pkg/ccl/backupccl/backup_job.go:1449:37: if jobDetails.FullCluster && codec.ForSystemTenant() && jobDetails.IncludeAllSecondaryTenants {`
- [x] `pkg/ccl/backupccl/backup_planning.go:704:26: if !p.ExecCfg().Codec.ForSystemTenant() {`
- [x] `pkg/ccl/backupccl/system_schema.go:144:16: if deps.codec.ForSystemTenant() {`
- [x] `pkg/ccl/backupccl/restore_job.go:1380:68: if backupTenantID != roachpb.SystemTenantID && p.ExecCfg().Codec.ForSystemTenant() {`
- [x] `pkg/keys/sql.go:179:4:// ForSystemTenant returns whether the encoder is bound to the system tenant.`
- [x] `pkg/keys/sql.go:180:21:func (e sqlEncoder) ForSystemTenant() bool {`
- [x] `pkg/keys/constants.go:295:73: // TODO(postamar): remove along with clusterversion.V23_1DescIDSequenceForSystemTenant`
- [x] `pkg/cli/gen.go:239:46: for _, key := range settings.Keys(settings.ForSystemTenant) {`
- [x] `pkg/cli/gen.go:240:68: setting, ok := settings.LookupForLocalAccessByKey(key, settings.ForSystemTenant)`
- [x] `pkg/ccl/streamingccl/streamingest/stream_ingestion_planning.go:110:24: if !p.ExecCfg().Codec.ForSystemTenant() {`
- [x] `pkg/ccl/streamingccl/streamingest/alter_replication_job.go:153:24: if !p.ExecCfg().Codec.ForSystemTenant() {`

Jira issue: CRDB-34629

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.