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

Backups: Improve commands to better handle large index dumps #3140

Closed
dsuresh-ap opened this issue Jan 22, 2023 · 7 comments
Closed

Backups: Improve commands to better handle large index dumps #3140

dsuresh-ap opened this issue Jan 22, 2023 · 7 comments
Assignees
Labels
bug Something isn't working priority Supported by early sponsors or popular demand released Available in the stable release

Comments

@dsuresh-ap
Copy link

dsuresh-ap commented Jan 22, 2023

It appears that if the SQL db is too large, the backup command silently fails when writing the buffer to string.

I have 2 photoprism containers, the one with a 1.17GB DB fails to output the dump while the 260MB DB dumps successfully.

Looking at the logic and the run time of the command, it looks like this block is not running properly. I suspect its due to the size of my DB and maybe out.String() on either 162 (both file and stdout doesn't work) or 165 is reaching some limitation. Clearly the logic is dropping off somewhere between line 152 and 169...

macOS 13.1 M1
Docker 4.15.0 (4 CPUs, 6GB RAM allocated)
photoprism:latest
MariaDB:10.10

Full context and details in the discussion.
Originally posted by @dsuresh-ap in #772 (comment)

@lastzero
Copy link
Member

Can you also verify if this could be a macOS / Apple Silicon specific issue? From what I know, Docker services are running inside a VM there, so the effective "hardware" specs might be different.

@dsuresh-ap
Copy link
Author

Yes I tried this on a MacBook Pro running Intel on a dev photoprism build and was able to reproduce. Looks like in this case I do see a log for Killed. Now that I reproduced it maybe I can fix it if Go supports byte streams or something similar.

\u@:\w$ ./photoprism backup -i
INFO[2023-01-23T06:29:55Z] config: case-insensitive file system detected 
DEBU[2023-01-23T06:29:55Z] config: running on 'Intel(R) Core(TM) i7-7920HQ CPU @ 3.10GHz', 4.1 GB memory detected 
DEBU[2023-01-23T06:29:55Z] settings: loaded from /go/src/github.com/photoprism/photoprism/storage/config/settings.yml 
INFO[2023-01-23T06:29:56Z] PhotoPrism® needs your support!              
INFO[2023-01-23T06:29:56Z] Visit https://photoprism.app/membership to learn more. 
DEBU[2023-01-23T06:29:56Z] config: successfully initialized [53.533589ms] 
INFO[2023-01-23T06:29:56Z] writing SQL dump to /go/src/github.com/photoprism/photoprism/storage/backup/mysql/2023-01-23.sql 
Killed

@lastzero
Copy link
Member

What is your memory limit for Docker? Note this is a separate setting in Docker Desktop. It's not your total system memory.

@dsuresh-ap
Copy link
Author

The Docker memory limit on my Mac mini it was set to 6GB, on this MacBook Pro it was set to 4GB. I raised my MBP limit to 8GB and it worked but still think it's an issue because the plain mysqldump command works fine, the issue is when the output goes through the Golang ByteBuffer.

@lastzero
Copy link
Member

It seems possible that the command requires twice the amount of memory due to the conversion or output handling.

lastzero added a commit that referenced this issue Apr 1, 2023
See also #3301, #3311, and #3298.

Signed-off-by: Michael Mayer <michael@photoprism.app>
@lastzero lastzero changed the title Refactor sql dump output not to run into size limits Backup: Refactor commands to handle large index dumps Apr 1, 2023
@lastzero lastzero changed the title Backup: Refactor commands to handle large index dumps Backups: Improve commands to better handle large index dumps Apr 1, 2023
@lastzero lastzero self-assigned this Apr 1, 2023
@lastzero lastzero added bug Something isn't working important please-test Ready for acceptance test labels Apr 1, 2023
@lastzero
Copy link
Member

lastzero commented Apr 1, 2023

From my understanding, these changes should write the sql dump directly to a file or to stdout, so no buffer is needed.

I've started a new development preview build for you to test this. Any help is greatly appreciated, as it allows our team to work on other issues that our community is waiting for. Thank you very much!

@lastzero
Copy link
Member

lastzero commented Apr 1, 2023

This is ready for testing. We appreciate your help!

seeschloss pushed a commit to seeschloss/photoprism that referenced this issue Apr 10, 2023
@graciousgrey graciousgrey added tested Changes have been tested successfully and removed please-test Ready for acceptance test labels Apr 24, 2023
@graciousgrey graciousgrey added released Available in the stable release and removed help wanted Well suited for external contributors! tested Changes have been tested successfully labels May 3, 2023
@lastzero lastzero added the priority Supported by early sponsors or popular demand label Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority Supported by early sponsors or popular demand released Available in the stable release
Projects
Status: Release 🌈
Development

No branches or pull requests

3 participants