-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
levels: Compaction incorrectly drops delete markers #1422
Conversation
fd89894 ("Compaction: Expired keys and delete markers are never purged") revealed a bug in the compaction logic that leads to delete markers being incorrectly dropped during compaction. During compaction, `*levelsController.compactBuildTables` decides to drop a delete key if there is no overlap with levels lower than the bottom layer of the compaction definition. It does that by checking only the `top` table, thinking that proving that the `top` table doesn't overlap is sufficient to prove that the `bottom` table doesn't. Unfortunately, this is not the case. Not in general, and even less in the case of `DropPrefix()` where we run a same-level compaction and `top` is empty. The faulty condition was introduced way back when in e597fb7 ("Discard key versions during compaction").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ashish-goswami, @manishrjain, and @NamanJain8)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @ashish-goswami, @manishrjain, and @NamanJain8)
@@ -484,11 +484,8 @@ func (s *levelsController) compactBuildTables( | |||
botTables := cd.bot | |||
|
|||
// Check overlap of the top level with the levels which are not being |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jarifibrahim the comment needs to be fixed?
fd89894 ("Compaction: Expired keys and delete markers are never purged") revealed a bug in the compaction logic that leads to delete markers being incorrectly dropped during compaction. During compaction, `*levelsController.compactBuildTables` decides to drop a delete key if there is no overlap with levels lower than the bottom layer of the compaction definition. It does that by checking only the `top` table, thinking that proving that the `top` table doesn't overlap is sufficient to prove that the `bottom` table doesn't. Unfortunately, this is not the case. Not in general, and even less in the case of `DropPrefix()` where we run a same-level compaction and `top` is empty. The faulty condition was introduced way back when in e597fb7 ("Discard key versions during compaction").
fd89894 ("Compaction: Expired keys and delete markers are never purged") revealed a bug in the compaction logic that leads to delete markers being incorrectly dropped during compaction. During compaction, `*levelsController.compactBuildTables` decides to drop a delete key if there is no overlap with levels lower than the bottom layer of the compaction definition. It does that by checking only the `top` table, thinking that proving that the `top` table doesn't overlap is sufficient to prove that the `bottom` table doesn't. Unfortunately, this is not the case. Not in general, and even less in the case of `DropPrefix()` where we run a same-level compaction and `top` is empty. The faulty condition was introduced way back when in e597fb7 ("Discard key versions during compaction"). (cherry picked from commit 5b90893)
fd89894 ("Compaction: Expired keys and delete markers are never purged") revealed a bug in the compaction logic that leads to delete markers being incorrectly dropped during compaction. During compaction, `*levelsController.compactBuildTables` decides to drop a delete key if there is no overlap with levels lower than the bottom layer of the compaction definition. It does that by checking only the `top` table, thinking that proving that the `top` table doesn't overlap is sufficient to prove that the `bottom` table doesn't. Unfortunately, this is not the case. Not in general, and even less in the case of `DropPrefix()` where we run a same-level compaction and `top` is empty. The faulty condition was introduced way back when in e597fb7 ("Discard key versions during compaction"). (cherry picked from commit 5b90893)
fd89894 ("Compaction: Expired keys and delete markers are never purged") revealed a bug in the compaction logic that leads to delete markers being incorrectly dropped during compaction. During compaction, `*levelsController.compactBuildTables` decides to drop a delete key if there is no overlap with levels lower than the bottom layer of the compaction definition. It does that by checking only the `top` table, thinking that proving that the `top` table doesn't overlap is sufficient to prove that the `bottom` table doesn't. Unfortunately, this is not the case. Not in general, and even less in the case of `DropPrefix()` where we run a same-level compaction and `top` is empty. The faulty condition was introduced way back when in e597fb7 ("Discard key versions during compaction"). (cherry picked from commit 5b90893) Co-authored-by: Damien Tournoud <damien@platform.sh>
fd89894 ("Compaction: Expired keys and delete markers are never purged") revealed a bug in the compaction logic that leads to delete markers being incorrectly dropped during compaction. During compaction, `*levelsController.compactBuildTables` decides to drop a delete key if there is no overlap with levels lower than the bottom layer of the compaction definition. It does that by checking only the `top` table, thinking that proving that the `top` table doesn't overlap is sufficient to prove that the `bottom` table doesn't. Unfortunately, this is not the case. Not in general, and even less in the case of `DropPrefix()` where we run a same-level compaction and `top` is empty. The faulty condition was introduced way back when in e597fb7 ("Discard key versions during compaction").
) (dgraph-io#1507) fd89894 ("Compaction: Expired keys and delete markers are never purged") revealed a bug in the compaction logic that leads to delete markers being incorrectly dropped during compaction. During compaction, `*levelsController.compactBuildTables` decides to drop a delete key if there is no overlap with levels lower than the bottom layer of the compaction definition. It does that by checking only the `top` table, thinking that proving that the `top` table doesn't overlap is sufficient to prove that the `bottom` table doesn't. Unfortunately, this is not the case. Not in general, and even less in the case of `DropPrefix()` where we run a same-level compaction and `top` is empty. The faulty condition was introduced way back when in e597fb7 ("Discard key versions during compaction"). (cherry picked from commit 5b90893) Co-authored-by: Damien Tournoud <damien@platform.sh>
This took me a ridiculous amount of time to track down.
fd89894 ("Compaction: Expired keys and delete markers are never purged") revealed a bug in the compaction logic that leads to delete markers being incorrectly dropped during compaction (i.e. deleted keys are reappearing).
During compaction,
*levelsController.compactBuildTables
decides to drop a delete key if there is no overlap with levels lower than the bottom layer of the compaction definition.It does that by checking only the
top
table, thinking that proving that thetop
table doesn't overlap is sufficient to prove that thebottom
table doesn't. Unfortunately, this is not the case. Not in general, and even less in the case ofDropPrefix()
where we run a same-level compaction andtop
is empty.The faulty condition was introduced way back when in e597fb7 ("Discard key versions during compaction").
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)