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

Improve resource management for block repository backends #77

Closed
3 of 4 tasks
ronoaldo opened this issue Nov 30, 2023 · 3 comments · Fixed by #78 or #80
Closed
3 of 4 tasks

Improve resource management for block repository backends #77

ronoaldo opened this issue Nov 30, 2023 · 3 comments · Fixed by #78 or #80
Assignees

Comments

@ronoaldo
Copy link
Contributor

ronoaldo commented Nov 30, 2023

It may be needed to allow for callers to manage resources when using the map data from the block repository. I see two opportunities for improvement/fixes:

1/ Implement defer Rows.Close() and DB.Close() on all backends: this may solve memory usage issues as reported here minetest-go/mapcleaner#179 as well as resolve final transactions/connections not being properly released (potentially what is happening here: minetest-go/mapcleaner#180.

2/ Implement a new version of Iterator(), IteratorTo() which receives a block bounding box. It could make concurrency easier to implement like for exporting all protected areas in separate go-routines. Also, perhaps a single iterator that keeps sql.Rows always open and only closes at the end could be a bad idea. If there are just too many blocks in the map (the main use case for cleaning the map) it can use just too much memory.

Open for feedback/thoughts on this!


Checklist:

  • Implement BlockRepository.Close().
  • Ensure sql.Rows.Close() is called.
  • Batch Iterator() to avoid single sql.Rows open for too much time.
  • Implement IteratorTo()  #79
@ronoaldo ronoaldo self-assigned this Nov 30, 2023
@ronoaldo ronoaldo changed the title Improve resource management for database backends for block repository Improve resource management for block repository backends Nov 30, 2023
ronoaldo added a commit to ronoaldo/mtdb that referenced this issue Nov 30, 2023
- Added sql.Rows.Close() calls when missing
- Allows the BlockRepository to be closed, and thus closing
  the underlying sql.DB connections.

Fixes minetest-go#77
BuckarooBanzay pushed a commit that referenced this issue Nov 30, 2023
- Added sql.Rows.Close() calls when missing
- Allows the BlockRepository to be closed, and thus closing
  the underlying sql.DB connections.

Fixes #77
@BuckarooBanzay
Copy link
Member

(reopening, looks like there are still 2 tasks open)

@ronoaldo
Copy link
Contributor Author

(reopening, looks like there are still 2 tasks open)

yeah! thanks! I'm going to test how Iterator() performs on a 80G/150M blocks map cleanup task and if it indeed causes resource usage issues due to a single reference to sql.Rows I'll batch them.

I have split the IteratorTo() as a new task. It is kind of a very similar impl. but may benefit from the perf. improvement if any.

ronoaldo added a commit to ronoaldo/mtdb that referenced this issue Dec 2, 2023
- Uses modernc.org/sqlite package
- Properly used row filters for Iterator
- Small updates for easy local testing
Refs minetest-go#77
@ronoaldo
Copy link
Contributor Author

ronoaldo commented Dec 2, 2023

#80 should resolve this.
Should also help with minetest-go/mapcleaner#185 because it uses a pure-go SQLite implementation.

@ronoaldo ronoaldo linked a pull request Dec 2, 2023 that will close this issue
BuckarooBanzay pushed a commit that referenced this issue Dec 3, 2023
* fixs: pure-go build, iterator for postgres
- Uses modernc.org/sqlite package
- Properly used row filters for Iterator
- Small updates for easy local testing
Refs #77

* doc: updated Iterator behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants