Skip to content

Commit

Permalink
sql: release leases before running migrations
Browse files Browse the repository at this point in the history
We fortunately prevent setting cluster settings in a transaction. That means
that we have full control over the access to tables that run during the
transaction which runs a cluster version upgrade. One observation is that
upgrades can take a long time if we leak leases in some way. In #85391 we
add function resolution which results in decriptors now being leased when
you run the following statement because we need to resolve the function:

`SET CLUSTER SETTING version = crdb_internal.node_executable_version()`

Then the test ends up taking 5 minutes +/- 15% for the leases to expire.
The downside here is that you don't get a transactional view of UDFs used
in expressions inside `SET CLUSTER VERSION`. That seems fine to me.

Release note: None
  • Loading branch information
ajwerner committed Aug 4, 2022
1 parent 748b89a commit aa52330
Showing 1 changed file with 14 additions and 1 deletion.
15 changes: 14 additions & 1 deletion pkg/sql/set_cluster_setting.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ func (n *setClusterSettingNode) startExec(params runParams) error {
params.p.EvalContext(),
params.extendedEvalCtx.Codec.ForSystemTenant(),
params.p.logEvent,
params.p.descCollection.ReleaseLeases,
)
if err != nil {
return err
Expand Down Expand Up @@ -317,6 +318,7 @@ func writeSettingInternal(
evalCtx *eval.Context,
forSystemTenant bool,
logFn func(context.Context, descpb.ID, eventpb.EventPayload) error,
releaseLeases func(context.Context),
) (expectedEncodedValue string, err error) {
err = execCfg.DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
var reportedValue string
Expand All @@ -336,6 +338,7 @@ func writeSettingInternal(
reportedValue, expectedEncodedValue, err = writeNonDefaultSettingValue(
ctx, execCfg, setting, name, txn,
user, st, value, forSystemTenant,
releaseLeases,
)
if err != nil {
return err
Expand Down Expand Up @@ -384,6 +387,7 @@ func writeNonDefaultSettingValue(
st *cluster.Settings,
value tree.Datum,
forSystemTenant bool,
releaseLeases func(context.Context),
) (reportedValue string, expectedEncodedValue string, err error) {
// Stringify the value set by the statement for reporting in errors, logs etc.
reportedValue = tree.AsStringWithFlags(value, tree.FmtBareStrings)
Expand All @@ -398,7 +402,9 @@ func writeNonDefaultSettingValue(
verSetting, isSetVersion := setting.(*settings.VersionSetting)
if isSetVersion {
if err := setVersionSetting(
ctx, execCfg, verSetting, name, txn, user, st, value, encoded, forSystemTenant); err != nil {
ctx, execCfg, verSetting, name, txn, user, st, value, encoded,
forSystemTenant, releaseLeases,
); err != nil {
return reportedValue, expectedEncodedValue, err
}
} else {
Expand Down Expand Up @@ -438,6 +444,7 @@ func setVersionSetting(
value tree.Datum,
encoded string,
forSystemTenant bool,
releaseLeases func(context.Context),
) error {
// In the special case of the 'version' cluster setting,
// we must first read the previous value to validate that the
Expand Down Expand Up @@ -534,6 +541,12 @@ func setVersionSetting(
})
}

// If we're here, we know we aren't in a transaction because we don't
// allow setting cluster settings in a transaction. We need to release
// our leases because the underlying migrations may affect descriptors
// we currently have a lease for. We don't need to hold on to any leases
// because the code isn't relying on them.
releaseLeases(ctx)
return runMigrationsAndUpgradeVersion(
ctx, execCfg, user, prev, value, updateVersionSystemSetting,
)
Expand Down

0 comments on commit aa52330

Please sign in to comment.