Skip to content

CQ message store: Don't scan for valid messages when deleting #13951

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

CQ message store: Don't scan for valid messages when deleting #13951

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions deps/rabbit/src/rabbit_msg_store.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2123,9 +2123,9 @@ truncate_file(File, Size, ThresholdTimestamp, #gc_state{ file_summary_ets = File

-spec delete_file(non_neg_integer(), gc_state()) -> ok | defer.

delete_file(File, State = #gc_state { file_summary_ets = FileSummaryEts,
file_handles_ets = FileHandlesEts,
dir = Dir }) ->
delete_file(File, #gc_state { file_summary_ets = FileSummaryEts,
file_handles_ets = FileHandlesEts,
dir = Dir }) ->
case ets:match_object(FileHandlesEts, {{'_', File}, '_'}, 1) of
{[_|_], _Cont} ->
rabbit_log:debug("Asked to delete file ~p but it has active readers. Deferring.",
Expand All @@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

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

(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

rabbitmq-server/deps/rabbit/src/rabbit_msg_store.erl

Line 2151 in 1797a6f

index_delete_object(IndexEts, Entry),
)

(Loic said its OK, so probably Im missing something)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

You were right, I have provided details in the new review. Thanks!

ok = file:delete(form_filename(Dir, filenum_to_name(File))),
true = ets:delete(FileSummaryEts, File),
rabbit_log:debug("Deleted empty file number ~tp; reclaimed ~tp bytes", [File, FileSize]),
Expand Down
Loading