-
Notifications
You must be signed in to change notification settings - Fork 246
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.
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
mysql: use a stable unique ID for stats #546
Conversation
func insertUniqueID(driver *MySQLDriver) string { | ||
return fmt.Sprintf(`INSERT INTO %s (unique_id) VALUES ("%s");`, | ||
driver.Metadata(), | ||
uuid.NewString(), | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We put special care into the design of mysql's datastore migrations to be data-migration free, as that is incompatible with migration tooling used in our production systems (skeema
).
My suggestion is to follow the same strategy used for "tuple transaction zero": to insert on runtime. The PR #539 offers a good foundation to do this, and there is a pattern that guarantees there is no race (i.e. several spicedb processes attempting to insert at the same time on startup).
@@ -491,6 +498,20 @@ func (mds *Datastore) seedDatabase(ctx context.Context) error { | |||
return fmt.Errorf("seedDatabase: failed to get last inserted id: %w", err) | |||
} | |||
|
|||
uuidSQL, uuidArgs, err := sb. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if multiple SpiceDB processes attempt to do this there will be most likely a race, and multiple rows will be inserted. You could solve this by adding an ID column and inserting on a known ID (e.g. zero), and use INSERT IGNORE
. That's the strategy used with the zeroth transaction.
Updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left a remark on the seeding logic being interleaved.
uuidSQL, uuidArgs, err := sb. | ||
Insert(mds.driver.Metadata()). | ||
Options("IGNORE"). | ||
Columns(metadataIDColumn, metadataUniqueIDColumn). | ||
Values(0, uuid.NewString()). | ||
ToSql() | ||
if err != nil { | ||
return fmt.Errorf("seedDatabase: failed to prepare SQL: %w", err) | ||
} | ||
|
||
_, err = tx.ExecContext(ctx, uuidSQL, uuidArgs...) | ||
if err != nil { | ||
return fmt.Errorf("seedDatabase: failed to insert unique ID: %w", err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to move the whole "UUID seeding" block after want to if lastInsertID != noLastInsertID {
, since it seems like interleaving 2 different concerns.
You may also want to do the same lastInsertID
check so you can emit a log message indicating the UUID was successfully inserted
Update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No description provided.