Skip to content

[scd] Don't disable lock with timeBasedNotificationIndex on yugabyte #1619

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

Open
the-glu wants to merge 1 commit into
base: master
Choose a base branch
from
+5 −2
Open

[scd] Don't disable lock with timeBasedNotificationIndex on yugabyte #1619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion pkg/scd/store/sqlstore/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type repo struct {
globalLock bool
hashLock bool
timeBasedNotificationIndex bool
version *sqlstore.Version
}

// Init initializes the SQL-backed sid store. It return a concrete sqlstore.Store[sid.repos.Repository] providing the
Expand All @@ -39,14 +40,15 @@ func Init(ctx context.Context, logger *zap.Logger, withCheckCron bool) (*sqlstor
DBName: "scd",
CrdbMajorSchemaVersion: currentCrdbMajorSchemaVersion,
YbMajorSchemaVersion: currentYugabyteMajorSchemaVersion,
NewRepo: func(q dssql.Queryable, clock clockwork.Clock, _ *sqlstore.Version) repos.Repository {
NewRepo: func(q dssql.Queryable, clock clockwork.Clock, version *sqlstore.Version) repos.Repository {
return &repo{
q: q,
clock: clock,
logger: logging.WithValuesFromContext(ctx, logger),
globalLock: opts.GlobalLock,
hashLock: opts.HashLock,
timeBasedNotificationIndex: opts.TimeBasedNotificationIndex,
version: version,
}
},
Registry: actions.Registry,
Expand Down
3 changes: 2 additions & 1 deletion pkg/scd/store/sqlstore/subscriptions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
dssmodels "github.com/interuss/dss/pkg/models"
scdmodels "github.com/interuss/dss/pkg/scd/models"
dsssql "github.com/interuss/dss/pkg/sql"
"github.com/interuss/dss/pkg/sqlstore"
"github.com/interuss/stacktrace"
"go.uber.org/zap"

Expand Down Expand Up @@ -444,7 +445,7 @@ func cellLockKeys(cells s2.CellUnion) []int64 {

func (c *repo) LockSubscriptionsOnCells(ctx context.Context, cells s2.CellUnion, subscriptionIds []dssmodels.ID, startTime *time.Time, endTime *time.Time) error {

if c.timeBasedNotificationIndex { // No lock when working with timeBasedNotificationIndex
if c.timeBasedNotificationIndex && c.version.Type == sqlstore.CockroachDB { // No lock when working with timeBasedNotificationIndex on CockroachDB

@BenjaminPelletier BenjaminPelletier Aug 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems very surprising. Isn't the purpose of locking subscriptions to serialize contention between modification of their notification indices? If their notification indices aren't being modified (because of the use of the time-based notification index), why would performance suffer in Yugabyte when we didn't lock the subscriptions while using the time-based notification index? I'm confused as to what mechanism would lead to increased performance in Yugabyte when adding a lock on subscriptions when the time-based notification index was already in use.

@the-glu the-glu Aug 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have fewer datapoints/investigations on Yugabyte than CockroachDB, as Cockroach was the focus, but based on what I know:

  • The subscriptions table still needs to be read, which means that if one query adds a subscription, we still have a form of contention (hence the less-than-expected improvements on CockroachDB with the flag)
  • Yugabyte seems to perform worse than CockroachDB under contention, as of now. I had to fix a few things because we didn't pass performance tests with Yugabyte:

I haven't performed a deep dive into Yugabyte mechanisms, but in general, it seems we need to lock elements correctly to avoid contention performance issues, whereas CockroachDB automatically handles some of these optimizations.

return nil
}

Expand Down
Loading