-
Notifications
You must be signed in to change notification settings - Fork 159
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
perf(505): Track the number of rows in each table #525
perf(505): Track the number of rows in each table #525
Conversation
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.
Just one nit.
3878235
to
1ed88d8
Compare
ae420bf
to
d249d9c
Compare
Question for reviewer: Is it possible to clear the contents of the database without restarting the process? This patch relies on the fact that clearing the database will reset the metrics counters, but that will only happen if a process restart is required. |
@@ -1569,10 +1597,34 @@ impl Locking { | |||
|
|||
// Create the system tables and insert information about themselves into | |||
// st_table, st_columns, st_indexes, and st_sequences. | |||
DB_METRICS | |||
.rdb_num_table_rows | |||
.with_label_values(&database_address, &st_table_schema().table_id.into()) |
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.
The table_id
of these objects could be queried once?
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're right, this should just happen inside bootstrap_system_table
DB_METRICS | ||
.rdb_num_table_rows | ||
.with_label_values(&database_address, &st_table_schema().table_id.into()) | ||
.set(0); | ||
datastore.bootstrap_system_table(st_table_schema())?; |
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.
Now looking it better I think this could be removed here and put inside bootstrap_system_table
:
fn bootstrap_system_table(&mut self, schema: TableSchema) -> Result<(), DBError> {
DB_METRICS
.rdb_num_table_rows
.with_label_values(&database_address, &schema.table_id.into()).set(0);
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.
Oh yes, that's much better.
@@ -380,6 +380,12 @@ impl Inner { | |||
let data_key = row.to_data_key(); | |||
st_tables.rows.insert(RowId(data_key), row); |
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.
Maybe create an insert/delete
indirection on rows
and hide it to be used elsewhere?. This could even reduce the number of places to check for this stats...
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.
Well I don't want to double count non-system tables. For all other tables I update the metrics on commit. That seemed like the best way to capture everything. But of course that doesn't account for the bootstrap process.
d249d9c
to
2434570
Compare
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
Closes #505.
Description of Changes
API and ABI
If the API is breaking, please state below what will break
Expected complexity level and risk
How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.
This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.
If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.