Skip to content

Commit

Permalink
Merge #138706 #138748
Browse files Browse the repository at this point in the history 
138706: slstorage: do not use InitPut with FailOnTombstones on Insert r=yuzefovich a=yuzefovich

This commit switches the usage of `InitPut` KV request when inserting a new sql liveness session from `FailOnTombstones=true` to `FailOnTombstones=false`. The contract of the `Insert` method is that the caller should never provide previously used session ID which is achieved by generating a random V4 UUID, so the probability of ever generating a duplicate is miniscule. The usage of `FailOnTombstones=true` option provides additional protection if the same session ID is reused within the GC TTL of the relevant range (because tombstones would get removed on MVCC garbage collection and/or compactions in pebble). This usage was introduced in 686f323 where we switched from SQL INSERT statement - which behaved similar to `FailOnTombstones=false`.

My main motivation behind this change is that this is the only place where we use `FailOnTombstones=true` option of the InitPut, and we're about to deprecate the InitPuts in favor of CPuts, yet CPuts don't have this option which would make the deprecation process more involved.

Also I think the current handling of this scenario by the code is partially broken. Namely, if previously used session ID is provided while there is a tombstone on the relevant key, the ConditionFailedError would be returned, and the loop in `Instance.createSession` would exceed the retry duration because on this error type we do not reset `session.id` field. This will result in not being able to create a session by the SQL instance, so it will fatal the process. On a quick search via Glean we never encountered this fatal before though.

With the change in this commit in the case of inserting the previously used session ID the behavior is undefined (I think one possible scenario is that other SQL instances won't be able to see this new instance because they have cached the session ID in the deadSessions cache, so the new instance won't be used for distributed queries; there are probably other consequences too).

Still, it seems reasonable to me to rely on randomly-generated UUIDs never repeating without additional - but only temporary - protection of `FailOnTombstones`.

Informs: #71074.
Epic: None

Release note: None

138748: sql: minor refactor of partial stats r=mgartner a=mgartner

Epic: None

Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
  • Loading branch information
@yuzefovich @mgartner
3 people committed Jan 9, 2025
3 parents 4fe1d00 + 6259e8e + 8fc6c3a commit c603477
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 18 deletions.
2 changes: 1 addition & 1 deletion pkg/sql/sqlliveness/slstorage/slstorage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ func (s *Storage) Insert(

}
v := encodeValue(expiration)
batch.InitPut(k, &v, true)
batch.InitPut(k, &v, false /* failOnTombstones */)

return txn.CommitInBatch(ctx, batch)
}); err != nil {
Expand Down
11 changes: 8 additions & 3 deletions pkg/sql/stats/automatic_stats.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,11 @@ func (r *Refresher) autoStatsEnabled(desc catalog.TableDescriptor) bool {
return enabledForTable == catpb.AutoStatsCollectionEnabled
}

// autoPartialStatsEnabled returns true if the
// sql_stats_automatic_partial_collection_enabled setting of the table
// descriptor set to true. If the table descriptor is nil or the table-level
// setting is not set, the function returns true if the automatic partial stats
// cluster setting is enabled.
func (r *Refresher) autoPartialStatsEnabled(desc catalog.TableDescriptor) bool {
if desc == nil {
// If the descriptor could not be accessed, defer to the cluster setting.
Expand Down Expand Up @@ -848,7 +853,7 @@ func (r *Refresher) maybeRefreshStats(
explicitSettings *catpb.AutoStatsSettings,
rowsAffected int64,
asOf time.Duration,
maybeRefreshPartialStats bool,
partialStatsEnabled bool,
) {
tableStats, err := r.cache.getTableStatsFromCache(ctx, tableID, nil /* forecast */, nil /* udtCols */, nil /* typeResolver */)
if err != nil {
Expand Down Expand Up @@ -900,8 +905,8 @@ func (r *Refresher) maybeRefreshStats(
(r.knobs != nil && r.knobs.DisableFullStatsRefresh) {
// No full statistics refresh is happening this time. Let's try a partial
// stats refresh.
if !maybeRefreshPartialStats {
// No refresh is happening this time, full or partial
if !partialStatsEnabled {
// No refresh is happening this time, full or partial.
return
}

Expand Down
28 changes: 14 additions & 14 deletions pkg/sql/stats/automatic_stats_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func TestMaybeRefreshStats(t *testing.T) {
// There are no stats yet, so this must refresh the full statistics on table t
// even though rowsAffected=0.
refresher.maybeRefreshStats(
ctx, s.AppStopper(), descA.GetID(), nil /* explicitSettings */, 0 /* rowsAffected */, time.Microsecond /* asOf */, true, /* maybeRefreshPartialStats */
ctx, s.AppStopper(), descA.GetID(), nil /* explicitSettings */, 0 /* rowsAffected */, time.Microsecond /* asOf */, true, /* partialStatsEnabled */
)
if err := checkStatsCount(ctx, cache, descA, 1 /* expectedFull */, 0 /* expectedPartial */); err != nil {
t.Fatal(err)
Expand All @@ -95,7 +95,7 @@ func TestMaybeRefreshStats(t *testing.T) {
// Try to refresh again. With rowsAffected=0, the probability of a refresh
// is 0, so refreshing will not succeed.
refresher.maybeRefreshStats(
ctx, s.AppStopper(), descA.GetID(), nil /* explicitSettings */, 0 /* rowsAffected */, time.Microsecond /* asOf */, true, /* maybeRefreshPartialStats */
ctx, s.AppStopper(), descA.GetID(), nil /* explicitSettings */, 0 /* rowsAffected */, time.Microsecond /* asOf */, true, /* partialStatsEnabled */
)
if err := checkStatsCount(ctx, cache, descA, 1 /* expectedFull */, 0 /* expectedPartial */); err != nil {
t.Fatal(err)
Expand All @@ -106,7 +106,7 @@ func TestMaybeRefreshStats(t *testing.T) {
minStaleRows := int64(100000000)
explicitSettings := catpb.AutoStatsSettings{MinStaleRows: &minStaleRows}
refresher.maybeRefreshStats(
ctx, s.AppStopper(), descA.GetID(), &explicitSettings, 10 /* rowsAffected */, time.Microsecond /* asOf */, true, /* maybeRefreshPartialStats */
ctx, s.AppStopper(), descA.GetID(), &explicitSettings, 10 /* rowsAffected */, time.Microsecond /* asOf */, true, /* partialStatsEnabled */
)
if err := checkStatsCount(ctx, cache, descA, 1 /* expectedFull */, 1 /* expectedPartial */); err != nil {
t.Fatal(err)
Expand All @@ -115,7 +115,7 @@ func TestMaybeRefreshStats(t *testing.T) {
// Do the same for partialMinStaleRows to also prevent a partial refresh.
explicitSettings.PartialMinStaleRows = &minStaleRows
refresher.maybeRefreshStats(
ctx, s.AppStopper(), descA.GetID(), &explicitSettings, 10 /* rowsAffected */, time.Microsecond /* asOf */, true, /* maybeRefreshPartialStats */
ctx, s.AppStopper(), descA.GetID(), &explicitSettings, 10 /* rowsAffected */, time.Microsecond /* asOf */, true, /* partialStatsEnabled */
)
if err := checkStatsCount(ctx, cache, descA, 1 /* expectedFull */, 1 /* expectedPartial */); err != nil {
t.Fatal(err)
Expand All @@ -127,7 +127,7 @@ func TestMaybeRefreshStats(t *testing.T) {
fractionStaleRows := float64(100000000)
explicitSettings = catpb.AutoStatsSettings{FractionStaleRows: &fractionStaleRows}
refresher.maybeRefreshStats(
ctx, s.AppStopper(), descA.GetID(), &explicitSettings, 10 /* rowsAffected */, time.Microsecond /* asOf */, true, /* maybeRefreshPartialStats */
ctx, s.AppStopper(), descA.GetID(), &explicitSettings, 10 /* rowsAffected */, time.Microsecond /* asOf */, true, /* partialStatsEnabled */
)
if err := checkStatsCount(ctx, cache, descA, 1 /* expectedFull */, 2 /* expectedPartial */); err != nil {
t.Fatal(err)
Expand All @@ -136,7 +136,7 @@ func TestMaybeRefreshStats(t *testing.T) {
// Do the same for partialFractionStaleRows to also prevent a partial refresh.
explicitSettings.PartialFractionStaleRows = &fractionStaleRows
refresher.maybeRefreshStats(
ctx, s.AppStopper(), descA.GetID(), &explicitSettings, 10 /* rowsAffected */, time.Microsecond /* asOf */, true, /* maybeRefreshPartialStats */
ctx, s.AppStopper(), descA.GetID(), &explicitSettings, 10 /* rowsAffected */, time.Microsecond /* asOf */, true, /* partialStatsEnabled */
)
if err := checkStatsCount(ctx, cache, descA, 1 /* expectedFull */, 2 /* expectedPartial */); err != nil {
t.Fatal(err)
Expand All @@ -147,7 +147,7 @@ func TestMaybeRefreshStats(t *testing.T) {
// Partial stats should not be refreshed since full stats are being refreshed,
// and stale partial stats should be cleared.
refresher.maybeRefreshStats(
ctx, s.AppStopper(), descA.GetID(), nil /* explicitSettings */, 10 /* rowsAffected */, time.Microsecond /* asOf */, true, /* maybeRefreshPartialStats */
ctx, s.AppStopper(), descA.GetID(), nil /* explicitSettings */, 10 /* rowsAffected */, time.Microsecond /* asOf */, true, /* partialStatsEnabled */
)
if err := checkStatsCount(ctx, cache, descA, 2 /* expectedFull */, 0 /* expectedPartial */); err != nil {
t.Fatal(err)
Expand All @@ -158,7 +158,7 @@ func TestMaybeRefreshStats(t *testing.T) {
descRoleOptions :=
desctestutils.TestingGetPublicTableDescriptor(s.DB(), codec, "system", "role_options")
refresher.maybeRefreshStats(
ctx, s.AppStopper(), descRoleOptions.GetID(), nil /* explicitSettings */, 10000 /* rowsAffected */, time.Microsecond /* asOf */, true, /* maybeRefreshPartialStats */
ctx, s.AppStopper(), descRoleOptions.GetID(), nil /* explicitSettings */, 10000 /* rowsAffected */, time.Microsecond /* asOf */, true, /* partialStatsEnabled */
)
if err := checkStatsCount(ctx, cache, descRoleOptions, 5 /* expectedFull */, 0 /* expectedPartial */); err != nil {
t.Fatal(err)
Expand All @@ -168,7 +168,7 @@ func TestMaybeRefreshStats(t *testing.T) {
descLease :=
desctestutils.TestingGetPublicTableDescriptor(s.DB(), codec, "system", "lease")
refresher.maybeRefreshStats(
ctx, s.AppStopper(), descLease.GetID(), nil /* explicitSettings */, 10000 /* rowsAffected */, time.Microsecond /* asOf */, true, /* maybeRefreshPartialStats */
ctx, s.AppStopper(), descLease.GetID(), nil /* explicitSettings */, 10000 /* rowsAffected */, time.Microsecond /* asOf */, true, /* partialStatsEnabled */
)
if err := checkStatsCount(ctx, cache, descLease, 0 /* expectedFull */, 0 /* expectedPartial */); err != nil {
t.Fatal(err)
Expand All @@ -178,7 +178,7 @@ func TestMaybeRefreshStats(t *testing.T) {
descTableStats :=
desctestutils.TestingGetPublicTableDescriptor(s.DB(), codec, "system", "table_statistics")
refresher.maybeRefreshStats(
ctx, s.AppStopper(), descTableStats.GetID(), nil /* explicitSettings */, 10000 /* rowsAffected */, time.Microsecond /* asOf */, true, /* maybeRefreshPartialStats */
ctx, s.AppStopper(), descTableStats.GetID(), nil /* explicitSettings */, 10000 /* rowsAffected */, time.Microsecond /* asOf */, true, /* partialStatsEnabled */
)
if err := checkStatsCount(ctx, cache, descTableStats, 0 /* expectedFull */, 0 /* expectedPartial */); err != nil {
t.Fatal(err)
Expand All @@ -189,7 +189,7 @@ func TestMaybeRefreshStats(t *testing.T) {
// TODO(rytaft): Should not enqueue views to begin with.
descVW := desctestutils.TestingGetPublicTableDescriptor(s.DB(), codec, "t", "vw")
refresher.maybeRefreshStats(
ctx, s.AppStopper(), descVW.GetID(), nil /* explicitSettings */, 0 /* rowsAffected */, time.Microsecond /* asOf */, true, /* maybeRefreshPartialStats */
ctx, s.AppStopper(), descVW.GetID(), nil /* explicitSettings */, 0 /* rowsAffected */, time.Microsecond /* asOf */, true, /* partialStatsEnabled */
)
select {
case <-refresher.mutations:
Expand Down Expand Up @@ -558,7 +558,7 @@ func TestAverageRefreshTime(t *testing.T) {
// the statistics on table t. With rowsAffected=0, the probability of refresh
// is 0.
refresher.maybeRefreshStats(
ctx, s.AppStopper(), table.GetID(), nil /* explicitSettings */, 0 /* rowsAffected */, time.Microsecond /* asOf */, true, /* maybeRefreshPartialStats */
ctx, s.AppStopper(), table.GetID(), nil /* explicitSettings */, 0 /* rowsAffected */, time.Microsecond /* asOf */, true, /* partialStatsEnabled */
)
if err := checkStatsCount(ctx, cache, table, 20 /* expectedFull */, 0 /* expectedPartial */); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -608,7 +608,7 @@ func TestAverageRefreshTime(t *testing.T) {
// remain (5 from column k and 5 from column v), since the old stats on k
// and v were deleted.
refresher.maybeRefreshStats(
ctx, s.AppStopper(), table.GetID(), nil /* explicitSettings */, 0 /* rowsAffected */, time.Microsecond /* asOf */, true, /* maybeRefreshPartialStats */
ctx, s.AppStopper(), table.GetID(), nil /* explicitSettings */, 0 /* rowsAffected */, time.Microsecond /* asOf */, true, /* partialStatsEnabled */
)
if err := checkStatsCount(ctx, cache, table, 10 /* expectedFull */, 0 /* expectedPartial */); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -757,7 +757,7 @@ func TestNoRetryOnFailure(t *testing.T) {
// Try to refresh stats on a table that doesn't exist.
r.maybeRefreshStats(
ctx, s.AppStopper(), 100 /* tableID */, nil /* explicitSettings */, math.MaxInt32,
time.Microsecond /* asOfTime */, false, /* maybeRefreshPartialStats */
time.Microsecond /* asOfTime */, false, /* partialStatsEnabled */
)

// Ensure that we will not try to refresh tableID 100 again.
Expand Down

0 comments on commit c603477

Please sign in to comment.