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.
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).
github-actions
bot
added
the
area/tooling
| @@ -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.
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 |
| 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.
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. |
No description provided.