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

[Merged by Bors] - Add rowid to tables with inefficient clustered indices #5394

Closed
wants to merge 1 commit into from

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented Dec 26, 2023

Motivation

WITHOUT ROWID doesn't work well for table rows over about 1/20th the size of a database page (50 bytes for 1KiB pages). This causes slower database access and increased disk usage.

Changes

Remove WITHOUT ROWID from problematic tables, add integer n column (also serving as rowid) for forthcoming syncv2 changes

Test Plan

TODO: verify that mainnet nodes function correctly after migration

TODO

  • Explain motivation or link existing issue(s)
  • Test changes and document test plan
  • Update changelog as needed

Copy link

codecov bot commented Dec 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (90916d4) 77.4% compared to head (cb0d910) 77.6%.
Report is 2 commits behind head on develop.

Additional details and impacted files
@@            Coverage Diff            @@
##           develop   #5394     +/-   ##
=========================================
+ Coverage     77.4%   77.6%   +0.1%     
=========================================
  Files          265     265             
  Lines        30894   30928     +34     
=========================================
+ Hits         23942   24012     +70     
+ Misses        5431    5393     -38     
- Partials      1521    1523      +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dshulyak
Copy link
Contributor

dshulyak commented Jan 2, 2024

what do you think about removing rowid from activesets table?

@ivan4th
Copy link
Contributor Author

ivan4th commented Jan 2, 2024

@dshulyak it can be done but unless activesets are pruned, that will take outrageously long to time to complete. Also, forgot to mention that, but actually w/o updating activesets, the database size increase (before vacuum) is not that big currently. So we can either do the full update after the pruning is done and in this case I can add activesets table update to this PR, or we can update activesets in a separate PR that we can apply after pruning.

@dshulyak
Copy link
Contributor

dshulyak commented Jan 3, 2024

we can safely drop all activesets before epoch 12 (current epoch)

@pigmej
Copy link
Member

pigmej commented Jan 14, 2024

That will tremendously help with cache warmup step.

Copy link
Contributor

@dshulyak dshulyak left a comment

Choose a reason for hiding this comment

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

any reason not to merge it? i was using it for quite some time now, probably once you posted it.

@@ -0,0 +1,105 @@
DROP INDEX atxs_by_pubkey_by_epoch_desc;
Copy link
Contributor

Choose a reason for hiding this comment

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

i think 0009 is taken now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@ivan4th ivan4th force-pushed the fix/rowids branch 2 times, most recently from 11e890d to adb2962 Compare January 15, 2024 15:52
@ivan4th ivan4th marked this pull request as ready for review January 15, 2024 15:53
@ivan4th ivan4th requested review from fasmat and poszu as code owners January 15, 2024 15:53
Stop using WITHOUT ROWID for tables with large row size
@ivan4th
Copy link
Contributor Author

ivan4th commented Jan 15, 2024

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Jan 15, 2024
## Motivation
`WITHOUT ROWID` doesn't work well for table rows over about 1/20th the size of a database page (50 bytes for 1KiB pages). This causes slower database access and increased disk usage.

## Changes
Remove WITHOUT ROWID from problematic tables, add integer `n` column (also serving as rowid) for forthcoming syncv2 changes

## Test Plan
**TODO**: verify that mainnet nodes function correctly after migration

## TODO
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [x] Update [changelog](../CHANGELOG.md) as needed
@spacemesh-bors
Copy link

Build failed:

@ivan4th
Copy link
Contributor Author

ivan4th commented Jan 16, 2024

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Jan 16, 2024
## Motivation
`WITHOUT ROWID` doesn't work well for table rows over about 1/20th the size of a database page (50 bytes for 1KiB pages). This causes slower database access and increased disk usage.

## Changes
Remove WITHOUT ROWID from problematic tables, add integer `n` column (also serving as rowid) for forthcoming syncv2 changes

## Test Plan
**TODO**: verify that mainnet nodes function correctly after migration

## TODO
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [x] Update [changelog](../CHANGELOG.md) as needed
@spacemesh-bors
Copy link

Pull request successfully merged into develop.

Build succeeded:

@spacemesh-bors spacemesh-bors bot changed the title Add rowid to tables with inefficient clustered indices [Merged by Bors] - Add rowid to tables with inefficient clustered indices Jan 16, 2024
@spacemesh-bors spacemesh-bors bot closed this Jan 16, 2024
@spacemesh-bors spacemesh-bors bot deleted the fix/rowids branch January 16, 2024 04:48
ivan4th added a commit that referenced this pull request Jan 16, 2024
`WITHOUT ROWID` doesn't work well for table rows over about 1/20th the size of a database page (50 bytes for 1KiB pages). This causes slower database access and increased disk usage.

Remove WITHOUT ROWID from problematic tables, add integer `n` column (also serving as rowid) for forthcoming syncv2 changes

**TODO**: verify that mainnet nodes function correctly after migration

- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [x] Update [changelog](../CHANGELOG.md) as needed
ivan4th added a commit that referenced this pull request Jan 16, 2024
`WITHOUT ROWID` doesn't work well for table rows over about 1/20th the size of a database page (50 bytes for 1KiB pages). This causes slower database access and increased disk usage.

Remove WITHOUT ROWID from problematic tables, add integer `n` column (also serving as rowid) for forthcoming syncv2 changes

**TODO**: verify that mainnet nodes function correctly after migration

- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [x] Update [changelog](../CHANGELOG.md) as needed
spacemesh-bors bot pushed a commit that referenced this pull request Jan 17, 2024
#5443)

## Motivation
Backport #5394 for `v1.3`

## Changes
Backport #5394
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