Cache table and schema indexes on schema address #7859
Conversation
|
#benchmark |
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
| } | ||
| } | ||
|
|
||
| type writeSessFunc func(nbf *types.NomsBinFormat, ws *doltdb.WorkingSet, aiTracker globalstate.AutoIncrementTracker, opts editor.Options) WriteSession |
There was a problem hiding this comment.
Should be an exported type since it's in constructors
Also needs comment
| ) | ||
|
|
||
| // SessionCache caches various pieces of expensive to compute information to speed up future lookups in the session. | ||
| type SessionCache struct { | ||
| indexes map[doltdb.DataCacheKey]map[string][]sql.Index | ||
| tables map[doltdb.DataCacheKey]map[TableCacheKey]sql.Table | ||
| // indexes is keyed by table schema |
| func (c *SessionCache) GetCachedWriterState(key doltdb.DataCacheKey) (*WriterState, bool) { | ||
| c.mu.RLock() | ||
| defer c.mu.RUnlock() | ||
| if c.writers == nil { |
There was a problem hiding this comment.
Think you can safely omit this, reads from a nil map work fine
| @@ -33,6 +33,7 @@ import ( | |||
| "github.com/dolthub/dolt/go/libraries/doltcore/schema" | |||
| "github.com/dolthub/dolt/go/libraries/doltcore/sqle/dsess" | |||
| "github.com/dolthub/dolt/go/libraries/doltcore/sqle/sqlutil" | |||
| "github.com/dolthub/dolt/go/libraries/doltcore/sqle/writer" | |||
| "github.com/dolthub/dolt/go/libraries/doltcore/table/editor" | |||
| config2 "github.com/dolthub/dolt/go/libraries/utils/config" | |||
| sqlSch, err := sqlutil.FromDoltSchema("", w.tableName.Name, sch) | ||
| if err != nil { | ||
| return err | ||
| func (w *prollyTableWriter) Reset(ctx *sql.Context, sess *prollyWriteSession, tbl *doltdb.Table, sch schema.Schema) error { |
There was a problem hiding this comment.
This is really bad, we really don't want to couple session state management to correct writer behavior like this. If this needs to happen for correctness, then the control of information flow needs to be one way only. Either the session owns this information and the caches of it and is responsible for clearing them, or this writer only manages its own internal state on reset.
There was a problem hiding this comment.
I'm not sure I follow this. The writer schema state needs to live in the session to be cached between transactions. It's only used for writing, so it's going to be coupled to writers. It's exclusively a property of the table, so anywhere we try to write the table seems valid to check for the cached write state as long as the table is well-formed (its schema is consistent with its data). If we passed the latest table to GetTableWriter would that alleviate your concerns? Then there's no ambiguity maybe, we're disregarding whatever is in the existing write session, replacing it with the schema values for the given table.
There was a problem hiding this comment.
So follow up on this, the preexisting coupling is probably worse than we originally thought. I can't add table parameter to these methods without breaking correctness. Certain writers rely on using the tables in prollyTableWriter.workingSet rather than whatever is available from the calling scope. I don't really like the indirection, but I would at least need to rewrite all of the fulltext logic to make this not the case. It's possible it extends to other areas. All of the action happens in GetTableWriter/Reset and package cycles prevent me from moving the logic out of dsess. The places where we have the context to create writers is the only place to manage the caching lifecycle.
There was a problem hiding this comment.
Punt I guess. This is at least a bit more self-contained now.
|
|
||
| // GetTableWriter implemented WriteSession. | ||
| func (s *prollyWriteSession) GetTableWriter(ctx *sql.Context, table doltdb.TableName, db string, setter SessionRootSetter) (TableWriter, error) { | ||
| func (s *prollyWriteSession) GetTableWriter(ctx *sql.Context, table doltdb.TableName, db string, setter dsess.SessionRootSetter) (dsess.TableWriter, error) { |
There was a problem hiding this comment.
Same comment as above. This is really bad separation of concerns.
Looking around, it appears that this method is called in exactly one place where caching is critical: (t *WritableDoltTable) getTableEditor. That's where this caching logic should be applied.
| @@ -106,6 +123,44 @@ func (s *prollyWriteSession) GetTableWriter(ctx *sql.Context, table doltdb.Table | |||
| return twr, nil | |||
| } | |||
|
|
|||
| func getWriterSchemas(ctx *sql.Context, table *doltdb.Table, tableName string) (*dsess.WriterState, error) { | |||
There was a problem hiding this comment.
This logic belongs in a layer above as well (either tables.go, or in the SessionCache, either would be fine). Then pass this object into GetTableWriter
|
#benchmark |
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@coffeegoddd DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@coffeegoddd DOLT
|
| sqlSch, err := sqlutil.FromDoltSchema("", w.tableName.Name, sch) | ||
| if err != nil { | ||
| return err | ||
| func (w *prollyTableWriter) Reset(ctx *sql.Context, sess *prollyWriteSession, tbl *doltdb.Table, sch schema.Schema) error { |
There was a problem hiding this comment.
Punt I guess. This is at least a bit more self-contained now.
|
@coffeegoddd DOLT
|
|
@coffeegoddd DOLT
|
|
@coffeegoddd DOLT
|
The bulk of ~1ms read and write TPC-C queries benefit from caching table and index schemas, which have a lifecycle between schema migrations/alter statements/new table additions. This is in contrast to how we've typically cached objects using the root value hash, which is great for read-only workflows, but has a much shorter half-life.