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

fix: enforce load limits when loading snapshot #4136

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Conversation

romange
Copy link
Collaborator

@romange romange commented Nov 15, 2024

Prevent loading snapshots with used memory higher than max memory limit.

  1. Store the used memory metadata only inside the summary file
  2. Load the summary file before loading anything else, and if the used-memory is higher, abort the load.

@romange romange requested a review from adiholden November 16, 2024 09:12
@adiholden
Copy link
Collaborator

What about dfly load append command?
you will not flush the database before you load the snapshot file.
If we want to enforce this we need to check current_used_memory + snapshot used memory < max_memory_limit

@romange
Copy link
Collaborator Author

romange commented Nov 17, 2024

This command is not interesting as is only used for migrations which are ad hoc at this point.

Prevent loading snapshots with used memory higher than max memory limit.

1. Store the used memory metadata only inside the summary file
2. Load the summary file before loading anything else, and if the used-memory is higher,
   abort the load.

Signed-off-by: Roman Gershman <[email protected]>
@@ -646,4 +646,13 @@ TEST_F(RdbTest, LoadHugeStream) {
ASSERT_EQ(2000, CheckedInt({"xlen", "test:0"}));
}

TEST_F(RdbTest, SnapshotTooBig) {
// Run({"debug", "populate", "10000", "foo", "1000"});
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: you can remove the lines in comment

@romange romange merged commit 0e7ae34 into main Nov 20, 2024
12 checks passed
@romange romange deleted the EnfoceLoadLimits branch November 20, 2024 04:12
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.

2 participants