sql/mysql_db: Rework how we store and access table data for users, grants and replica_source_info. #1904
Conversation
…ants and replica_source_info. In general, we keep the same structure of storing the data in structs, keyed in multimaps, and having converters for going to and from sql.Rows. We change the following important things: 1) We more explicitly model a multimap and an indexed set of entities with multiple keyers. 2) We add read and write locking around edits and reads of the data table. 3) We explicitly do not expose the raw indexed set or multimap from the MySQLDb instance itself. The unprincipled access of the various *Data instances in the old implementation was somewhat problematic.
sql/analyzer/privileges.go
Outdated
| // TODO: The management around closing this editor is sad. If you add | ||
| // an early exit before the Close after the GetUser below, make sure to | ||
| // close this editor. | ||
| ed := mysqlDb.Editor() |
There was a problem hiding this comment.
Is it worth moving this line down to line 47ish, so that the editor open is closer to the ed.Close() call and less likely to have a return statement added in between? Seems like that would prevent you from needing to close it on line 43, too, unless I'm just missing something.
There was a problem hiding this comment.
I'm doing something kinda gross here where I'm using the Editor() to lock the access to Enabled as well. I will poke around to see how to change Enabled somehow to be more thread-safe.
sql/in_mem_table/multimap.go
Outdated
| } | ||
|
|
||
| func (is IndexedSet[V]) VisitEntries(f func(v V)) { | ||
| is.Indexes[0].VisitEntries(f) |
There was a problem hiding this comment.
It wasn't immediately obvious to me why why only iterate over the first element of Indexes. Is that because it's the primary index and we don't want to visit secondary indexes? Might be worth some godocs to explain why this function only looks at the first element of Indexes or maybe even a different name?
| @@ -1201,23 +1200,37 @@ func schemaPrivilegesRowIter(ctx *Context, c Catalog) (RowIter, error) { | |||
| if !ok { | |||
| return nil, ErrDatabaseNotFound.New("mysql") | |||
| } | |||
|
|
|||
| var keys []mysql_db.UserPrimaryKey | |||
There was a problem hiding this comment.
(super minor) might be nice to move this var declaration down to line 1210, so it's right by the code that populates the variable. (Took me an extra second to notice this was where the keys variable came from.)
| @@ -1601,23 +1660,36 @@ func tablePrivilegesRowIter(ctx *Context, c Catalog) (RowIter, error) { | |||
| if !ok { | |||
| return nil, ErrDatabaseNotFound.New("mysql") | |||
| } | |||
|
|
|||
| var keys []mysql_db.UserPrimaryKey | |||
There was a problem hiding this comment.
(super minor) similar comment; this var definition just felt a little "floaty" and would be nice to move it down to line 1670 so it's right before where the keys get populated.
sql/plan/grant.go
Outdated
| @@ -664,33 +666,30 @@ func (n *GrantRole) CheckPrivileges(ctx *sql.Context, opChecker sql.PrivilegedOp | |||
| //TODO: only active roles may be assigned if the SUPER privilege is not held | |||
| mysqlDb := n.MySQLDb.(*mysql_db.MySQLDb) | |||
| client := ctx.Session.Client() | |||
| user := mysqlDb.GetUser(client.User, client.Address, false) | |||
|
|
|||
| // TODO: Locking | |||
There was a problem hiding this comment.
Is there more locking needed here beyond what the reader encapsulates? I didn't understand concretely what this TODO was recommending.
There was a problem hiding this comment.
This was a stale comment from before we had locking! Good catch :D.
…tion location, how locking is done in some places. Make mysql_db.Enabled access thread-safe.
…ocking to read it in our cache check.
In general, we keep the same structure of storing the data in structs, keyed in multimaps, and having converters for going to and from sql.Rows. We change the following important things:
We more explicitly model a multimap and an indexed set of entities with multiple keyers.
We add read and write locking around edits and reads of the data table.
We explicitly do not expose the raw indexed set or multimap from the MySQLDb instance itself. The unprincipled access of the various *Data instances in the old implementation was somewhat problematic.