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

refactor: change account ID SQL's backing type #603

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

Conversation

tomyrd
Copy link
Collaborator

@tomyrd tomyrd commented Dec 27, 2024

closes #602

In this PR we change the account ID sql columns from blobs to two integers (u64). This allows us to index only with the prefix (as it uniquely identifies accounts) which is faster than indexing with the whole ID.

In this implementation I used two integers in tables where the full account ID was needed and only prefix when it was used as a foreign key.

@tomyrd
Copy link
Collaborator Author

tomyrd commented Dec 27, 2024

I wanted to ask about this part:

used two integers in tables where the full account ID was needed

This is not technically necessary as the suffix may be stored only in the accounts table and retreived using a JOIN but I wasn't sure how this would affect the performance. If we prefer, I can use only prefix and get the suffix with joins if needed.

Copy link
Collaborator

@igamigo igamigo left a comment

Choose a reason for hiding this comment

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

LGTM! I think this approach should work well. I left a small comment, but everything else looks good. Also, not sure but it might be worth mentioning (maybe in a comment on the migration file) why account IDs are broken up into 2, and why the prefix works as a primary key, etc.

@@ -103,14 +104,29 @@ where
})
}

pub fn read_account_id_from_columns(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add some brief doc comments to this function.

@bobbinth
Copy link
Contributor

bobbinth commented Jan 6, 2025

I would probably put this PR on-hold for now. Ideally, we'd have some sort of a framework to measure performance impact of optimizations like this before implementing them.

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.

3 participants