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.

Sign up for GitHub

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

sqlccl: fix a data race in TestTenantTempTableCleanup #111054

Merged
merged 2 commits into from
Sep 22, 2023
Merged

sqlccl: fix a data race in TestTenantTempTableCleanup #111054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c319c09
sqlccl: fix a data race in TestTenantTempTableCleanup
knz Sep 21, 2023
e0b095b
settings: clarify missharing of cluster.Settings
knz Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions pkg/ccl/testccl/sqlccl/temp_table_clean_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,18 +96,21 @@ func TestTenantTempTableCleanup(t *testing.T) {
)
tenantStoppers := []*stop.Stopper{stop.NewStopper(), stop.NewStopper()}

tenantSettings := cluster.MakeTestingClusterSettings()
sql.TempObjectCleanupInterval.Override(ctx, &tenantSettings.SV, time.Second)
sql.TempObjectWaitInterval.Override(ctx, &tenantSettings.SV, time.Second*0)
// Set up sessions to expire within 5 seconds of a
// nodes death.
slinstance.DefaultTTL.Override(ctx, &tenantSettings.SV, 5*time.Second)
slinstance.DefaultHeartBeat.Override(ctx, &tenantSettings.SV, time.Second)
tenantSettings := func() *cluster.Settings {
st := cluster.MakeTestingClusterSettings()
sql.TempObjectCleanupInterval.Override(ctx, &st.SV, time.Second)
sql.TempObjectWaitInterval.Override(ctx, &st.SV, time.Second*0)
// Set up sessions to expire within 5 seconds of a
// nodes death.
slinstance.DefaultTTL.Override(ctx, &st.SV, 5*time.Second)
slinstance.DefaultHeartBeat.Override(ctx, &st.SV, time.Second)
return st
}

_, tenantPrimaryDB := serverutils.StartTenant(t, tc.Server(0),
base.TestTenantArgs{
TenantID: serverutils.TestTenantID(),
Settings: tenantSettings,
Settings: tenantSettings(),
TestingKnobs: tenantTempKnobSettings,
Stopper: tenantStoppers[0],
})
Expand All @@ -124,7 +127,7 @@ func TestTenantTempTableCleanup(t *testing.T) {
_, tenantSecondDB := serverutils.StartTenant(t, tc.Server(1),
base.TestTenantArgs{
TenantID: serverutils.TestTenantID(),
Settings: tenantSettings,
Settings: tenantSettings(),
Stopper: tenantStoppers[1],
})
tenantSecondSQL := sqlutils.MakeSQLRunner(tenantSecondDB)
Expand Down Expand Up @@ -165,7 +168,7 @@ func TestTenantTempTableCleanup(t *testing.T) {
_, tenantPrimaryDB = serverutils.StartTenant(t, tc.Server(0),
base.TestTenantArgs{
TenantID: serverutils.TestTenantID(),
Settings: tenantSettings,
Settings: tenantSettings(),
TestingKnobs: tenantTempKnobSettings,
Stopper: tenantStoppers[0]})
tenantSQL = sqlutils.MakeSQLRunner(tenantPrimaryDB)
Expand Down
20 changes: 14 additions & 6 deletions pkg/settings/values.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ type Values struct {
opaque interface{}
}

type classCheck int8
type classCheck uint32

const (
// classCheckUndefined is used when the settings.Values hasn't been
Expand All @@ -67,6 +67,14 @@ const (
classCheckVirtualCluster
)

func (ck *classCheck) get() classCheck {
return classCheck(atomic.LoadUint32((*uint32)(ck)))
}

func (ck *classCheck) set(nv classCheck) {
atomic.StoreUint32((*uint32)(ck), uint32(nv))
}

const numSlots = MaxSettings + 1

type valuesContainer struct {
Expand Down Expand Up @@ -150,20 +158,20 @@ TIP: avoid using the same cluster.Settings or settings.Value object across multi
// SpecializeForSystemInterface marks the values container as
// pertaining to the system interface.
func (sv *Values) SpecializeForSystemInterface() {
if sv.classCheck != classCheckUndefined && sv.classCheck != classCheckSystemInterface {
if ck := sv.classCheck.get(); ck != classCheckUndefined && ck != classCheckSystemInterface {
panic(errors.AssertionFailedf(alreadySpecializedError))
}
sv.classCheck = classCheckSystemInterface
sv.classCheck.set(classCheckSystemInterface)
}

// SpecializeForVirtualCluster marks this container as pertaining to
// a virtual cluster, after which use of SystemOnly values is
// disallowed.
func (sv *Values) SpecializeForVirtualCluster() {
if sv.classCheck != classCheckUndefined && sv.classCheck != classCheckVirtualCluster {
if ck := sv.classCheck.get(); ck != classCheckUndefined && ck != classCheckVirtualCluster {
panic(errors.AssertionFailedf(alreadySpecializedError))
}
sv.classCheck = classCheckVirtualCluster
sv.classCheck.set(classCheckVirtualCluster)
for slot, setting := range slotTable {
if setting != nil && setting.Class() == SystemOnly {
sv.container.forbidden[slot] = true
Expand All @@ -174,7 +182,7 @@ func (sv *Values) SpecializeForVirtualCluster() {
// SpecializedToVirtualCluster returns true if this container is for a
// virtual cluster (i.e. SpecializeToVirtualCluster() was called).
func (sv *Values) SpecializedToVirtualCluster() bool {
return sv.classCheck == classCheckVirtualCluster
return sv.classCheck.get() == classCheckVirtualCluster
}

// Opaque returns the argument passed to Init.
Expand Down