lease,systemschema: hook up the lease table to regional-by-row partitioning#92588
Conversation
|
This change is |
1ee3bcf to
b9327f6
Compare
72c3cfd to
f95068f
Compare
| // it and returns it. | ||
| func (t *descriptorState) upsertLeaseLocked( | ||
| ctx context.Context, desc catalog.Descriptor, expiration hlc.Timestamp, | ||
| ctx context.Context, desc catalog.Descriptor, expiration hlc.Timestamp, prefix []byte, |
There was a problem hiding this comment.
could you add some comment on prefix? or just make it regionPrefix?
| sem: quotapool.NewIntPool("lease manager", leaseConcurrencyLimit), | ||
| } | ||
| lm.storage.regionPrefix = &atomic.Value{} | ||
| lm.storage.regionPrefix.Store(enum.One) |
There was a problem hiding this comment.
can it be something else like an empty slice, or this is just something conventional?
There was a problem hiding this comment.
We prevent it from being the empty slice. We want it to always be a value. Enums always have this value unless it has been removed.
|
|
||
| // SetRegionPrefix sets the prefix this Manager uses to write leases. If val | ||
| // is empty, this call is a no-op. Note that the default value is enum.One. | ||
| // This means that leases acquired before initial startup may write their |
There was a problem hiding this comment.
What does this mean for a non-multiregion cluster? (I think my question is should we disallow calls to this method if it's not a multiregion cluster?)
There was a problem hiding this comment.
In an non-MR cluster this doesn't do anything.
| return err | ||
| } | ||
|
|
||
| s.leaseMgr.SetRegionPrefix(regionPhysicalRep) |
There was a problem hiding this comment.
regionPhysicalRep may be []byte{} and if it is, we should use enum.One instead. We discussed pushing this into GetLocalityRegionEnumPhysicalRepresentation, but I think that would be surprising behavior for the API. Instead, we could put the branch in server_sql. Alternatively we could defer initializing the leasing subsystem until a sql session is created and the lease could use the region from the session.
If we don't use enum.One, then we run into problems using sql to query leases that were created before the system database is configured as multi-region:
jeff@multiregion-8.6s4p.crdb.io:26257/defaultdb> select * from system.lease;
ERROR: internal error: unexpected error from the vectorized engine: could not find [] in enum "system.public.crdb_internal_region" representation PhysicalReps: [[64] [96] [128]]; LogicalReps: [gcp-asia-southeast1 gcp-europe-west1 gcp-us-central1] goroutine 12874 [running]:
There was a problem hiding this comment.
This is handled inside that call. It has the below logic. The value is initialized to enum.One.
if len(val) > 0 {
m.storage.regionPrefix.Store(val)
}| }, | ||
| descpb.IndexDescriptor{ | ||
| Name: "primary", | ||
| ID: 1, |
There was a problem hiding this comment.
The multi-region index should use ID 2 if we want to support migrating from the old schema to the new schema using a protocol resembling a standard schema change.
…ioning Release note: None
f95068f to
411c8c2
Compare
|
TFTR! bors r+ |
|
Build failed: |
|
bors r+ flake was #93287 |
|
Build succeeded: |
This is an oversight from cockroachdb#92588. Testing will come in cockroachdb#92826. Epic: CRDB-18596 Release note: None
93294: dev: do not log noisy bep flag r=rickystewart a=healthy-pod Release note: None Epic: none 93342: sql: partition lease table when optimizing system database for MR r=ajwerner a=ajwerner This is an oversight from #92588. Testing will come in #92826. Epic: CRDB-18596 Release note: None Co-authored-by: healthy-pod <ahmad@cockroachlabs.com> Co-authored-by: Ahmad Abedalqader <ahmad@cockroachlabs.com> Co-authored-by: Andrew Werner <awerner32@gmail.com>
This PR extends the
system.leasetable with REGIONAL BY ROW partitioning for use with #92395. The PR is itself freestanding.Epic: CRDB-18596
Release note: None