sql: lease acquisition of OFFLINE descs may starve bulk operations#62611
sql: lease acquisition of OFFLINE descs may starve bulk operations#62611craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
c6992cb to
8db656d
Compare
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @fqazi)
pkg/sql/catalog/lease/lease_test.go, line 2726 at r1 (raw file):
} func TestOfflineLeaseRefresh(t *testing.T) {
a comment sketching the test and what it demonstrates would be nice.
pkg/sql/catalog/lease/lease_test.go, line 2750 at r1 (raw file):
params := base.TestServerArgs{Knobs: base.TestingKnobs{Store: knobs}} //ReplicaRequestFilter
detritus
pkg/sql/catalog/lease/lease_test.go, line 2812 at r1 (raw file):
// Select from an unrelated table _, err = s.InternalExecutor().(sqlutil.InternalExecutor).ExecEx(ctx, "inline-exec", txn, sessiondata.InternalExecutorOverride{User: security.RootUserName()}, "insert into d1.t2 values (10);")
nit: wrap and what not
pkg/sql/catalog/lease/lease_test.go, line 2823 at r1 (raw file):
for notify := range waitForTxn { close(notify) mu.RLock()
I think the locking here would bite you if the transaction above restarted. I feel like it can be simplified.
pkg/sql/catalog/lease/lease_test.go, line 2828 at r1 (raw file):
// leasing out the table again _, err = conn.Query("select * from d1.t1") if err == nil {
this conditional seems off.
6112eb1 to
dd97e90
Compare
ajwerner
left a comment
There was a problem hiding this comment.
This probably should not make 21.1.0 but rather 21.1.1 as it is not new.
It seems like the test times out under stress. Run it on your gceworker with a shorter timeout to shake out these flakes.
Reviewable status:
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status:
pkg/sql/catalog/lease/lease.go, line 1432 at r2 (raw file):
} } return descVersion.Descriptor, descVersion.expiration, nil
nit: roll thing into a helper to attach the commentary. Also note that this is to match behavior which existed before we leased offline descriptors.
c4e55b6 to
21c01ec
Compare
ajwerner
left a comment
There was a problem hiding this comment.
needs a bazel-generate
Reviewable status:
Thanks @ajwerner , didn't realize what was needed |
Fixes: cockroachdb#61798 Previously, offline descriptors would never have their leases cached and they would be released once the reference count hit zero. This was inadequate because when attempting to online these tables again the lease acquisition could be pushed back by other operations, leading to starvation / live locks. To address this, this patch will allow the leases of offline descriptors to be cached. Release note (bug fix): Lease acquisitions of descriptor in a offline state may starve out bulk operations (backup / restore)
|
bors r=ajwerner |
|
Build succeeded: |
|
@fqazi is this a candidate for a backport? |
|
oh nvm please disregard my last comment, I see the changes have been reverted |
Fixes: #61798
Previously, offline descriptors would never have their leases
cached and they would be released once the reference count hit zero.
This was inadequate because when attempting to online these tables
again the lease acquisition could be pushed back by other operations,
leading to starvation / live locks. To address this, this patch will
allow the leases of offline descriptors to be cached.
Release note (bug fix): Lease acquisitions of descriptor in a
offline state may starve out bulk operations (backup / restore)