CQ message store: Don't scan for valid messages when deleting #13951
Conversation
Looking for valid messages before deleting a message store file was an old check for extra safety. However, it is a bottleneck under intense workload: it can take a few seconds to delete a file (a vast majority of that spent scanning for messages), while new files can be created faster than that. That leads to the disk usage growing, even if the number of messages in the queues is not. We are not aware of any situations where this check actually found valid messages in a file that was requested to be deleted. This is a well tested code path a this point, so let's just remove the extra check.
| @@ -2134,7 +2134,6 @@ delete_file(File, State = #gc_state { file_summary_ets = FileSummaryEts, | |||
| _ -> | |||
| [#file_summary{ valid_total_size = 0, | |||
| file_size = FileSize }] = ets:lookup(FileSummaryEts, File), | |||
| [] = scan_and_vacuum_message_file(File, State), | |||
There was a problem hiding this comment.
(this is a discussion that is brought over from #13940 (reply in thread), but I think this PR is a better place)
I wonder which code part calls index_delete for the index entries that point to the deleted file, if this line is removed? (and how there won't be a leak of index entries) (I mean instead of
(Loic said its OK, so probably Im missing something)
There was a problem hiding this comment.
My explanation is not as good as Loïc's but here is my logic: scan_and_vacuum_message_file executes all actions on the messages found by scan_file_for_valid_messages. The list of those found messages is later returned and matched against []. Therefore, index_delete_object is never really executed here (that we know of) - if it were, we'd see a badmatch for [] = scan_and_vacuum_message_file(File, State) and we agreed that we've never seen it.
There was a problem hiding this comment.
Let me double check, I think there used to be some deletes delayed to this function. Not sure that's true anymore. If it is we can do a quicker ets delete than going over the entire file.
There was a problem hiding this comment.
You were right, I have provided details in the new review. Thanks!
There was a problem hiding this comment.
We must handle the case where entries in the index exist but have a ref_count of 0. Basically perform the equivalent of https://github.com/rabbitmq/rabbitmq-server/pull/13951/files#diff-152a5c9ff6f63d7db77f29ba31364bc77e8ee24164c48499e7831d2b9e4f8a5cR2149-R2150
We don't need to lookup entries, a single ets:match_delete/2 call that matches on file = File and ref_count = 0 should be enough to remove everything in one go. There shouldn't be entries with a positive ref_count after that.
I agree there is no need to read anything from the file itself. But a simple But there could be a "secondary index", another ets table that maps from rdq file -> msgId, that could be used for efficient lookup. (This efficient lookup could be used during compaction as well, so the file scanning is only left for dirty recovery) |
|
It's not exactly the same. The lookup/delete is done all in ets, and it's only to be used on deletion, not on compaction. I don't know if this will be good enough, benchmarking is necessary, but at least for deletion it should be better than scanning the file. The problem with a secondary ets table is that you have to write to that table and that slows down other operations. The only real solution is a new file format where the messages in the file are always easy to find (linked list or tree based or similar, basically a format where we have either a message+link to next message location, or just a link to next message location, allowing jumping around the file to get all the entries without having to scan). The problem in the current format being that when we compact, we end up with holes that contain garbage, whereas we would want a link to the next message there. Perhaps that's doable without too many changes to the current format but there's always compatibility issues to think about. That's a heavy change and I'm busy with other things. For now I'd settle for "better". |
|
For what it's worth we don't have to run the match_delete every time a file is deleted, we only need to run it from time to time to clean up the memory index. If it's not good enough when we run it every time, we may run it every N files deleted. |
|
UPDATE: I've implemented a delayed index cleanup but it turned out to be significantly slower ( |
|
Yup, this new approach looks much more promising: #13959 My test case so far:
On 4.1/main, it takes ~30 seconds for me I will do a bit more testing |
Looking for valid messages before deleting a message store file was an old check for extra safety. However, it is a bottleneck under intense workload: it can take a few seconds to delete a file (a vast majority of that spent scanning for messages), while new files can be created faster than that. That leads to the disk usage growing, even if the number of messages in the queues is not.
We are not aware of any situations where this check actually found valid messages in a file that was requested to be deleted. This is a well tested code path a this point, so let's just remove the extra check.
References #13940 #13233 #13430.