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

PostgreSQL - Simple Composer for reduced memory usage #1037

Closed
wants to merge 0 commits into from

Conversation

krnaveen14
Copy link
Contributor

@krnaveen14 krnaveen14 commented Jul 14, 2021

Database name

PostgreSQL

Pull request description

Simple Composer for reduced memory usage on instances with > 100k files

  • Does not track metadata of files backed up (Files and TarFileSets)
  • Does not support WALG_DELTA_MAX_STEPS, WALG_DELTA_ORIGIN, WALG_USE_REVERSE_UNPACK, WALG_SKIP_REDUNDANT_TARS, catchup-push

Describe what this PR fix

Similar to Issue #738 , WAL-G consumes huge memory on backing up instance with millions of files. This is due to tracking the metadata of each file, maintaining the list of files in each TarFileSet and finally marshalling the data structure to text for creating sentinel.json file. Even though PR #740 tries to address the marshalling part by streaming / pipe it, significant memory usage on BackupSentinelDto data structure still persists.

Based on the BackupSentinelDto usage, it is needed only for configurations such as DELTA, INCREMENTAL, RATING_COMPOSER, catchup-push etc... Since we don't use any of the above mentioned configs, Files and TarFileSets data in BackupSentinelDto simply unused.

To reduce the memory usage of WAL-G on instances with millions of files and to make it available & configurable for the community as whole, SimpleTarBallComposer is implemented which does not track any of the file metadata, list of files in each TarFileSet and the list of TarFileSets itself.

Simple Composer is supported on both local and remote backups.

Usage : wal-g backup-push [path] --simple-composer or WALG_USE_SIMPLE_COMPOSER=true environment variable

Please provide steps to reproduce

Create 500+ Databases with 1000+ Tables in each Database (not necessary to insert any data). Execute WAL-G backup-push to see the memory usage of wal-g gradually increases over time and failing with OOM after exhausting available memory (10GB in our case).

@krnaveen14 krnaveen14 requested a review from a team as a code owner July 14, 2021 11:31
@krnaveen14 krnaveen14 force-pushed the simple-composer branch 3 times, most recently from 7dd53be to c382c7a Compare July 14, 2021 12:27
@krnaveen14
Copy link
Contributor Author

@usernamedt waiting on approval for test execution and expecting code review suggestions

Copy link
Member

@usernamedt usernamedt left a comment

Choose a reason for hiding this comment

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

Hi!

This is definitely a cool feature, but it needs some adjustments.

docs/PostgreSQL.md Outdated Show resolved Hide resolved
internal/databases/postgres/backup_push_handler.go Outdated Show resolved Hide resolved
internal/databases/postgres/simple_tar_ball_composer.go Outdated Show resolved Hide resolved
internal/databases/postgres/tar_ball_composer.go Outdated Show resolved Hide resolved
Copy link
Member

@usernamedt usernamedt left a comment

Choose a reason for hiding this comment

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

Hi!

Looks like this unit test is failing:

2021-08-17T11:25:48.0366780Z === RUN   TestGetFilesToUnwrap_UtilityFiles
2021-08-17T11:25:48.0367313Z     backup_test.go:49: 
2021-08-17T11:25:48.0367713Z         	Error Trace:	backup_test.go:49
2021-08-17T11:25:48.0378206Z         	Error:      	Not equal: 
2021-08-17T11:25:48.0378808Z         	            	expected: map[string]bool{"/global/pg_control":true, "backup_label":true, "tablespace_map":true}
2021-08-17T11:25:48.0379442Z         	            	actual  : map[string]bool(nil)
2021-08-17T11:25:48.0379793Z         	            	
2021-08-17T11:25:48.0380100Z         	            	Diff:
2021-08-17T11:25:48.0380664Z         	            	--- Expected
2021-08-17T11:25:48.0381016Z         	            	+++ Actual
2021-08-17T11:25:48.0381447Z         	            	@@ -1,6 +1,2 @@
2021-08-17T11:25:48.0381949Z         	            	-(map[string]bool) (len=3) {
2021-08-17T11:25:48.0382567Z         	            	- (string) (len=18) "/global/pg_control": (bool) true,
2021-08-17T11:25:48.0383204Z         	            	- (string) (len=12) "backup_label": (bool) true,
2021-08-17T11:25:48.0383852Z         	            	- (string) (len=14) "tablespace_map": (bool) true
2021-08-17T11:25:48.0384349Z         	            	-}
2021-08-17T11:25:48.0384701Z         	            	+(map[string]bool) <nil>
2021-08-17T11:25:48.0385035Z         	            	 
2021-08-17T11:25:48.0385506Z         	Test:       	TestGetFilesToUnwrap_UtilityFiles
2021-08-17T11:25:48.0386267Z --- FAIL: TestGetFilesToUnwrap_UtilityFiles (0.00s)
2021-08-17T11:25:48.0386909Z === RUN   TestGetFilesToUnwrap_NoMoreFiles
2021-08-17T11:25:48.0387661Z --- PASS: TestGetFilesToUnwrap_NoMoreFiles (0.00s)

Copy link
Member

@usernamedt usernamedt left a comment

Choose a reason for hiding this comment

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

Seems OK, but need to change the flag

internal/databases/postgres/tar_file_sets.go Outdated Show resolved Hide resolved
internal/databases/postgres/bundle_files.go Outdated Show resolved Hide resolved
cmd/pg/backup_push.go Outdated Show resolved Hide resolved
@krnaveen14
Copy link
Contributor Author

Closing this in favor new PR #1101

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.

None yet

2 participants