Skip to content
This repository has been archived by the owner on Feb 5, 2025. It is now read-only.

separate transactions when updating aggregates #741

Merged
merged 7 commits into from
Nov 25, 2024

Conversation

imabdulbasit
Copy link
Contributor

@imabdulbasit imabdulbasit commented Nov 22, 2024

I am seeing a lot of database locking errors for sqlite during the update of aggregates. I tried separating the read transaction for the aggregate count from the write transaction, and the locking errors are gone. However, I am not sure why this is happening.

@imabdulbasit imabdulbasit changed the title separate transactions for aggregate height and upsert in update_aggre… separate transactions when updating aggregates Nov 22, 2024
Copy link
Member

@jbearer jbearer left a comment

Choose a reason for hiding this comment

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

Not sure how or why this helps either, but it looks correct.

Also see comment about a nice optimization this unlocks

Copy link
Member

@jbearer jbearer left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -340,9 +341,18 @@ impl<Types: NodeType> UpdateAggregatesStorage<Types> for Transaction<Write> {
"aggregate",
["height", "num_transactions", "payload_size"],
["height"],
rows,
rows.clone(),
Copy link
Member

Choose a reason for hiding this comment

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

Nit: it is a bit wasteful to clone all the rows here just to grab the last row below. Might make more sense to pull out the updated aggregate first. Doesn't matter much though as the rows are usually small. This is not a blocker

@imabdulbasit imabdulbasit merged commit d17f0d6 into main Nov 25, 2024
10 checks passed
@imabdulbasit imabdulbasit deleted the ab/split-aggregate-tx branch November 25, 2024 19:54
imabdulbasit added a commit that referenced this pull request Dec 4, 2024
…te-tx"

This reverts commit d17f0d6, reversing
changes made to 970d3c3.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants