Skip to content
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

Implemented database optimization routine #721

Open
wants to merge 4 commits into
base: next
Choose a base branch
from

Conversation

polydez
Copy link
Contributor

@polydez polydez commented Feb 26, 2025

Implemented database optimization routine:

  1. Full optimization with shrinking and analysis on node start.
  2. Limited indexes analysis in background each day (default value, configurable).

This is a recommended way according to docs: https://www.sqlite.org/pragma.html#pragma_optimize

Such optimization routine should help query planner to better make decision which index to use when it has choice.

@polydez polydez marked this pull request as ready for review February 26, 2025 17:36
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions.

This optimises the query planner i.e. index selection and ordering. Do you think our queries are complex enough for this to be beneficial? Or put differently - we still need to create indices for it to choose from, but more often than not we'll only have a single index for it to choose from per query. This doesn't automatically create new indices.

There is also the issue of reproducibility:

Some developers prefer that once the design of an application is frozen, SQLite will always pick the same query plans as it did during development and testing. Then if a millions of copies of the application are shipped to customers, the developers are assured that all of those millions of copies are running the same query plans regardless of what data the individual customers insert into their particular databases. This can help in reproducing complaints of performance problems coming back from the field.

To achieve this objection, never run a full ANALYZE nor the "PRAGMA optimize" command in the application. Rather, only run ANALYZE during development, manually using ...


This also does not shrink the database. That is done using VACUUM but it will take forever on large databases and should not be done automatically. Database compaction should really only be done manually on demand as the node will have significant downtime during it.

@polydez
Copy link
Contributor Author

polydez commented Feb 27, 2025

This optimises the query planner i.e. index selection and ordering. Do you think our queries are complex enough for this to be beneficial?

I think, yes, we run pretty complex queries for account states computing with lots of ANDs which require query analyzer to choose which index to use.

Or put differently - we still need to create indices for it to choose from, but more often than not we'll only have a single index for it to choose from per query. This doesn't automatically create new indices.

I didn't expect from SQLite to create indices, I'm adding new indices in another PR in terms of #712.

There is also the issue of reproducibility:

Yes, and we should probably discuss, what is better: performance over reproducibility of performance issues or reproducibility over performance. To be honest, I can't imagine, how PRAGMA optimize could ruin performance.

This also does not shrink the database. That is done using VACUUM but it will take forever on large databases and should not be done automatically. Database compaction should really only be done manually on demand as the node will have significant downtime during it.

Ah, my bad! I was confused by documentation and though that 0x10000 flag would cause database to shrink. I have checked this on small PoC code and now I agree, it doesn't shrink it.

polydez and others added 2 commits February 27, 2025 16:33
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM - just need to decide whether we want determinism.

@Mirko-von-Leipzig
Copy link
Contributor

There is also the issue of reproducibility:

Yes, and we should probably discuss, what is better: performance over reproducibility of performance issues or reproducibility over performance. To be honest, I can't imagine, how PRAGMA optimize could ruin performance.

I've had it make a bad decision once or twice where it picked the wrong ordering. In a way its like any optimiser/compiler - sometimes it does go the wrong way. Mostly these issues were related to changes in data composition over time, e.g. more output notes per tx and block

Comment on lines +22 to +28
info!(target: COMPONENT, "Starting database optimization");

match self.state.optimize_db().await {
Ok(_) => info!(target: COMPONENT, "Finished database optimization"),
Err(err) => error!(target: COMPONENT, %err, "Database optimization failed"),
}
}
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using trace events to communicate the time start and end, prefer using a span to cover it instead.

This essentially means we want a new root span to cover the self.state.optimize_db call.

Suggested change
info!(target: COMPONENT, "Starting database optimization");
match self.state.optimize_db().await {
Ok(_) => info!(target: COMPONENT, "Finished database optimization"),
Err(err) => error!(target: COMPONENT, %err, "Database optimization failed"),
}
}
let root_span = tracing::info_span!("optimize_database", interval=self.optimization_interval.as_secs_f32());
self.state.optimize_db()
.instrument(root_span)
.inspect_err(|err| root_span.set_error(err))
.await;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general the store component still needs a telemetry make-over but this seems like and important one to trace so that we can get the actual performance impact of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants