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

sql,config: tenant zone config lookup is broken #75864

Closed
ajwerner opened this issue Feb 2, 2022 · 6 comments · Fixed by #76504
Closed

sql,config: tenant zone config lookup is broken #75864

ajwerner opened this issue Feb 2, 2022 · 6 comments · Fixed by #76504
Assignees
Labels
A-zone-configs branch-master Failures and bugs on the master branch. branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@ajwerner
Copy link
Contributor

ajwerner commented Feb 2, 2022

Describe the problem

In #74612 we made it so that the tenant has access to its own zone configs via the SystemConfig object provided by the systemconfigwatcher. Unfortunately this did not go far enough to actually fully address #70558 because of this special case logic:

func (s *SystemConfig) GetZoneConfigForObject(
codec keys.SQLCodec, id uint32,
) (*zonepb.ZoneConfig, error) {
var sysID SystemTenantObjectID
if codec.ForSystemTenant() {
sysID = SystemTenantObjectID(id)
} else {
sysID = keys.TenantsRangesID
}
entry, err := s.getZoneEntry(sysID)
if err != nil {
return nil, err
}
return entry.combined, nil
}

Solution

We need to either remove the above logic or make it conditional on where the config is coming from and more generally make sure that the SystemConfig methods work for the tenant.

Epic: CRDB-10489

Jira issue: CRDB-12864

@ajwerner ajwerner added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Feb 2, 2022
@blathers-crl blathers-crl bot added the T-sql-schema-deprecated Use T-sql-foundations instead label Feb 2, 2022
@ajwerner
Copy link
Contributor Author

ajwerner commented Feb 2, 2022

h/t to @arulajmani

arulajmani added a commit to arulajmani/cockroach that referenced this issue Feb 2, 2022
We closed cockroachdb#69265 in favour of cockroachdb#70558, and the only remaining work
left to address locality aware planning for tenants is captured
in cockroachdb#75864.

Release note: None
craig bot pushed a commit that referenced this issue Feb 2, 2022
…75871

58261: opt,sql: use paired-joins with non-covering indexes for left joins r=rytaft a=sumeerbhola

This is done when the left outer/semi/anti join can use a
lookup join. Prior to this, when the non-covering index
could not fully evaluate the filter for left join we could
not generate a lookup join.

With this change:
- Left outer join becomes a pair of two left outer joins.
- Left semi join is a pair of inner join followed by left
  semi join.
- Left anti join is a pair of left outer join followed by
  left anti join.

Informs #55452

Release note (performance improvement): The optimizer can now
generate lookup joins in certain cases for non-covering
indexes, when performing a left outer/semi/anti join.

75746: dev: initialize submodules in `dev doctor` r=aayushshah15 a=aayushshah15

This commit adds a check to `dev doctor` to initialize submodules, like we do
in our `Makefile`.

Fixes #72247

Release note: None

75766: server: do not check decommission list for the tenant r=JeffSwenson a=JeffSwenson

Previously, the system tenant would return PermissionDenied if the
tenant's instance_id was equivalent to a decommissioned node's id.

Now, the system tenant does not check the decommissioned node list if
the incoming node_id belongs to a non-system tenant.

This PR feeds the request context down to the OnOutgoingPing and
OnIncomingPing callbacks. Previously the callbacks were using the
ambient context. The only use of the context was a storage.MVCCGet call
in nodeTombstoneStorage.IsDecommissioned.

Release note: None

75804: sql: support RESET ALL statement r=otan a=rafiss

fixes #75435

Release note (sql change): Support for the RESET ALL statement was
added. This statement resets the values of all session variables to
their default values.

75822: sql: error when setting timezone outside of postgres max utc offsets r=otan a=RichardJCai

Release note (sql change): Previously, users would be able to set
a UTC timezone offset of greater than 167 or less than -167. This
now returns an error.

Example:

SET TIME ZONE '168'
Gives error:
invalid value for parameter "timezone": "'168'": cannot find time zone "168": UTC timezone offset is out of range.

SET TIME ZONE '-168'
Gives error:
invalid value for parameter "timezone": "'-168'": cannot find time zone "-168": UTC timezone offset is out of range.

Fixes #75168

Note: If a user has already set a UTC timezone offset outside of these bounds, it will be unchanged. 

75843: c-deps/krb5: fix build for more recent versions of autoconf r=otan a=nicktrav

More recent versions of `autoconf`, when used to build `krb5`, generates
shell scripts with invalid syntax.

Fix by pulling in the [upstream patch][1] for the issue into our tree.

Closes #72529.

[1]:
krb5/krb5@f78edbe

75845: vendor: bump Pebble to 38b68e17aa97 r=jbowens a=nicktrav

Pebble commits:

```
38b68e17 internal/batchskl: return error on index overflow
8440f290 internal/manifest: use a line sweep to optimize NewL0Sublevels
0f5acb26 sstable: add direct block reading to suffix rewriter
26856d10 db: avoid stats flake in TestMemTableReservation
b452808f sstable: Make sstable Writer.Close idempotent
17fe1a65 sstable: add RewriteKeySuffixes function
c9e6edfc db: expose metrics on count and earliest seqnum of snapshots
b958d9a7 sstable: add a writeQueue to the sstable writer
c8dad06c db: disable automatic compactions in `MetricsTest`
015f5e38 internal/rangekey: fix range key iteration bug
```

The commit `38b68e17` contains the fix for #69906.

Closes #69906.

Release note: none

75857: sql: fix small race in distIndexBackfiller r=adityamaru a=stevendanna

This fixes a small race condition in
distIndexBackfiller. updateJobDetails calls SetResumeSpansInJob which
mutates the ResumeSpanList in the job details.  Normally, this is only
called from the periodic updater.  However, when the testing knob
AlwasyUpdateIndexBackfillDetails is set, we also update it on every
ProducerMetadata message we get back

Release note: None

75865: build: address util.log.logcrash package rename r=knz a=rail

After `util.log` was renamed to `util.log.logcrash`, the build system
stopped updating the Sentry environment variable properly. Instead of
setting it to the release version, it was falling back to the default
"development" value. As a result, all Sentry reports went to the
development environment bucket.

This patch addresses the name change.

Release note: None

75871: logictestccl: fix stale issue number in TODO r=arulajmani a=arulajmani

We closed #69265 in favour of #70558, and the only remaining work
left to address locality aware planning for tenants is captured
in #75864.

Release note: None

Co-authored-by: sumeerbhola <sumeer@cockroachlabs.com>
Co-authored-by: Aayush Shah <aayush.shah15@gmail.com>
Co-authored-by: Jeff <swenson@cockroachlabs.com>
Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Co-authored-by: richardjcai <caioftherichard@gmail.com>
Co-authored-by: Nick Travers <travers@cockroachlabs.com>
Co-authored-by: Steven Danna <danna@cockroachlabs.com>
Co-authored-by: Rail Aliiev <rail@iqchoice.com>
Co-authored-by: arulajmani <arulajmani@gmail.com>
@ajwerner
Copy link
Contributor Author

ajwerner commented Feb 8, 2022

This affects the optimizer's ability to do locality aware search, and nothing else.

@ajwerner
Copy link
Contributor Author

ajwerner commented Feb 8, 2022

Also, this only affects serverless, so we can backport if the need arises.

@irfansharif
Copy link
Contributor

It also affects the SQL GC job, right?

// refreshTenant updates the status of tenant that is waiting to be GC'd. It
// returns whether or the tenant has expired or the duration until it expires.
func refreshTenant(
ctx context.Context,
execCfg *sql.ExecutorConfig,
dropTime int64,
details *jobspb.SchemaChangeGCDetails,
progress *jobspb.SchemaChangeGCProgress,
) (expired bool, deadline time.Time) {
if progress.Tenant.Status != jobspb.SchemaChangeGCProgress_WAITING_FOR_GC {
return true, time.Time{}
}
tenantTTLSeconds := execCfg.DefaultZoneConfig.GC.TTLSeconds
tenID := details.Tenant.ID
cfg := execCfg.SystemConfig.GetSystemConfig()
zoneCfg, err := cfg.GetZoneConfigForObject(keys.MakeSQLCodec(roachpb.MakeTenantID(tenID)), 0)
if err == nil {
tenantTTLSeconds = zoneCfg.GC.TTLSeconds

@ajwerner ajwerner added the release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. label Feb 8, 2022
@blathers-crl
Copy link

blathers-crl bot commented Feb 8, 2022

Hi @ajwerner, please add branch-* labels to identify which branch(es) this release-blocker affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@ajwerner ajwerner added the branch-master Failures and bugs on the master branch. label Feb 8, 2022
@ajwerner
Copy link
Contributor Author

ajwerner commented Feb 8, 2022

😬 yes, that's legit.

@ajwerner ajwerner self-assigned this Feb 28, 2022
@postamar postamar added GA-blocker branch-master Failures and bugs on the master branch. branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 and removed branch-master Failures and bugs on the master branch. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. labels Mar 14, 2022
ajwerner added a commit to ajwerner/cockroach that referenced this issue Mar 27, 2022
Follow-on from cockroachdb#76279.

Fixes cockroachdb#75864.

This commit adds a mechanism to combine the system config data of the tenant
with the data provided over the GossipSubscription from the system tenant.
It then plumbs a version into the zone config methods. In the mixed version
state, the tenant uses the existing override from the system tenant. After
the span config infrastructure has been activated, the tenant uses the
overrides they've set. This affects, realistically, just the GC job, and,
to a lesser extent, the optimizer.

Release note: None
ajwerner added a commit to ajwerner/cockroach that referenced this issue Apr 11, 2022
Follow-on from cockroachdb#76279.

Fixes cockroachdb#75864.

This commit adds a mechanism to combine the system config data of the tenant
with the data provided over the GossipSubscription from the system tenant.
It then plumbs a version into the zone config methods. In the mixed version
state, the tenant uses the existing override from the system tenant. After
the span config infrastructure has been activated, the tenant uses the
overrides they've set. This affects, realistically, just the GC job, and,
to a lesser extent, the optimizer.

Release note: None
ajwerner added a commit to ajwerner/cockroach that referenced this issue Apr 11, 2022
Follow-on from cockroachdb#76279.

Fixes cockroachdb#75864.

This commit adds a mechanism to combine the system config data of the tenant
with the data provided over the GossipSubscription from the system tenant.
It then plumbs a version into the zone config methods. In the mixed version
state, the tenant uses the existing override from the system tenant. After
the span config infrastructure has been activated, the tenant uses the
overrides they've set. This affects, realistically, just the GC job, and,
to a lesser extent, the optimizer.

Release note: None
@craig craig bot closed this as completed in 6de7a18 Apr 11, 2022
@ajwerner ajwerner reopened this Apr 12, 2022
ajwerner added a commit to ajwerner/cockroach that referenced this issue Apr 12, 2022
Follow-on from cockroachdb#76279.

Fixes cockroachdb#75864.

This commit adds a mechanism to combine the system config data of the tenant
with the data provided over the GossipSubscription from the system tenant.
It then plumbs a version into the zone config methods. In the mixed version
state, the tenant uses the existing override from the system tenant. After
the span config infrastructure has been activated, the tenant uses the
overrides they've set. This affects, realistically, just the GC job, and,
to a lesser extent, the optimizer.

Release note: None
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-zone-configs branch-master Failures and bugs on the master branch. branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants