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

feat: Eliminate remaining repeat_vars() calls (#6359) #6375

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iequidoo
Copy link
Collaborator

Close #6359

@iequidoo iequidoo marked this pull request as ready for review December 31, 2024 03:27
@iequidoo iequidoo requested a review from link2xt December 31, 2024 03:27
for id in msg_ids {
let ts: i64 = context
.sql
.query_get_value("SELECT timestamp FROM msgs WHERE id=?", (id,))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can be optimized using Sql::call() + preparing a statement and executing it in a loop. But below there's a heavier loop calling Message::load_from_db() anyway

for &id in &msg_ids {
if let Some(msg) = context
.sql
.query_row_optional(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also can be optimised executing a prepared statement in a loop, but the function is heavy anyway, it creates SMTP jobs etc.

&format!(
let trans_fn = |t: &mut rusqlite::Transaction| {
t.execute(
"CREATE TABLE contacts_tmp (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not good that this became a write transaction, but the function call is expected to be heavy anyway if the chat isn't found and an ad-hoc group needs to be created

Copy link
Collaborator

@link2xt link2xt Jan 5, 2025

Choose a reason for hiding this comment

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

Shouldn't it be CREATE TEMPORARY TABLE instead?
SQLite supports this syntax: https://www.sqlite.org/lang_createtable.html
I think it is possible to modify temporary tables with read-only connection as they should not be shared between connections anyway, but this needs to be tested.

Temporary table should still be deleted at the end of transaction because we practically never close connections.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It works, but requires PRAGMA query_only=0; to be done before creating a temporary table, so there's some "PRAGMA flickering", but i think it's not a problem, anyway temporary tables won't be used frequently it seems

src/receive_imf.rs Show resolved Hide resolved
Using `repeat_vars()` to generate SQL statements led to some of them having more than
`SQLITE_MAX_VARIABLE_NUMBER` parameters and thus failing, so let's get rid of this pattern. But
let's not optimise for now and just repeat executing an SQL statement in a loop, all the places
where `repeat_vars()` is used seem not performance-critical and containing functions execute other
SQL statements in loops. If needed, performance can be improved by preparing a statement and
executing it in a loop. An exception is `lookup_chat_or_create_adhoc_group()` where `repeat_vars()`
can't be replaced with a query loop, there we need to replace the `SELECT` query with a read
transaction creating a temporary table which is used to perform the SELECT query then.
@iequidoo iequidoo force-pushed the iequidoo/drop-repeat_vars branch from 92adf14 to 50ad130 Compare January 5, 2025 19:06
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.

Eliminate remaining repeat_vars() calls
2 participants