Misc datastore cleanups & less cloning #1263
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Centril
commented
May 20, 2024
Comment on lines
+61
to
+81
| pub(crate) const ST_TABLES_IDX: usize = 0; | ||
| pub(crate) const ST_COLUMNS_IDX: usize = 1; | ||
| pub(crate) const ST_INDEXES_IDX: usize = 2; | ||
| pub(crate) const ST_CONSTRAINTS_IDX: usize = 3; | ||
| pub(crate) const ST_MODULE_IDX: usize = 4; | ||
| pub(crate) const ST_SEQUENCES_IDX: usize = 5; |
There was a problem hiding this comment.
These should correspond to the indices in the array in system_tables() above.
Centril
commented
May 20, 2024
Comment on lines
-32
to
-34
| fn table_exists(&self, table_id: &TableId) -> Option<&str> { | ||
| self.committed_state_shared_lock.table_exists(table_id) | ||
| } |
There was a problem hiding this comment.
Now a provided function defined via get_schema.
Comment on lines
+38
to
+40
| fn table_name(&self, table_id: TableId) -> Option<&str> { | ||
| self.get_schema(table_id).map(|s| &*s.table_name) | ||
| } |
There was a problem hiding this comment.
Said provided definition.
Comment on lines
-1004
to
-1013
| fn table_exists(&self, table_id: &TableId) -> Option<&str> { | ||
| // TODO(bikeshedding, docs): should this also check if the schema is in the system tables, | ||
| // but the table hasn't been constructed yet? | ||
| // If not, document why. | ||
| self.tx_state | ||
| .insert_tables | ||
| .get(table_id) | ||
| .or_else(|| self.committed_state_write_lock.tables.get(table_id)) | ||
| .map(|table| &*table.schema.table_name) | ||
| } |
There was a problem hiding this comment.
Using the provided definition.
Comment on lines
-63
to
-66
| fn table_exists(&self, table_id: &TableId) -> Option<&str> { | ||
| self.tables.get(table_id).map(|t| &*t.schema.table_name) | ||
| } |
| let schemas = system_tables().map(Arc::new); | ||
| let ref_schemas = schemas.each_ref().map(|s| &**s); |
There was a problem hiding this comment.
We immediately Arc all the system tables and then get out &TableSchema for use as needed. We refer to all system tables by index rather than recreating them.
Shubham8287
approved these changes
May 21, 2024
Centril
force-pushed
the
centril/misc-datastore-cleanup
branch
from
5e1aa59 to
df8b241
Compare
2. Only call system_tables() once in bootstrap_system_tables. 3. Take `TableId` by value more 4. Rename `table_exists` -> `table_name` 5. Dedup `table_name` by using `get_schema()`. 6. Other misc deduping.
Centril
force-pushed
the
centril/misc-datastore-cleanup
branch
from
df8b241 to
8a6241a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of Changes
st_*_schemafunctions, only used in system_tables() now.system_tables()once inbootstrap_system_tables. EagerlyArcthose and then clone as little as possible.TableIdby value moretable_exists->table_nametable_nameby usingget_schema().API and ABI breaking changes
None
Expected complexity level and risk
2, touches some system table code but in a fairly small way.
Still, be sure to double check
ST_TABLES_IDXand thebootstrap_system_tableschanges.