flush_job: do not roll back memtable flush on CF drop or database shutdown #126

isaac-io · 2022-08-18T20:23:18Z

In non-atomic flush the flush job code currently rolls back the memtable flush in case the CF was dropped or if the database is shutting down. This is unnecessary since it reactivates the pending flush flag on the relevant CF even though no flush should take place after CF drop or DB shutdown request, and it also causes an issue for the upcoming improvements to the WriteBufferManager immutable memory tracking (#113) because it marks the memtable memory as ready for flush again for a short period before finally getting freed when the memtable is dropped during the destruction of the dropped CF or the closing of the DB (which might upset the WBM behaviour because in the future we plan to rely on the amount of memory that isn't already being flushed for triggering flushes rather than on the amount of mutable memory alone as is done today).

Since the rollback only marks the memtable as ready for flush again and clears related flush state, there's no harm in skipping it and just letting the memtable drop in that state. Moreover, in atomic flush those cases do not trigger a rollback for flushes that already completed, so this simply matches the behaviour there.

The text was updated successfully, but these errors were encountered:

…126) Rolling back in these cases is meaningless because the memtables will be freed shortly afterwards, so making them available for flush again is useless at best. Additionally, in atomic flush, memtables that were successfully flushed aren't rolled back if a CF is dropped or a DB shutdown was requested (the shutdown case is the more relevant one for the comparison here because it aborts the flush, whereas the in the CF drop case it's simply ignored at flush result installation time). In this regard this change simply matches the behaviour there. Lastly, this might be needed for the WriteBufferManager changes in #113, since we plan to use the information about memory that can be flushed in the future for triggering flushes based on immutable memory as well, and rolling back flushes causes the memtable memory to become ready for flush again for a brief period of time until it is dropped, which might wrongly affect the WBM decisions.

…#126) Rolling back in these cases is meaningless because the memtables will be freed shortly afterwards, so making them available for flush again is useless at best. Additionally, in atomic flush, memtables that were successfully flushed aren't rolled back if a CF is dropped or a DB shutdown was requested (the shutdown case is the more relevant one for the comparison here because it aborts the flush, whereas the in the CF drop case it's simply ignored at flush result installation time). In this regard this change simply matches the behaviour there. Lastly, this might be needed for the WriteBufferManager changes in #113, since we plan to use the information about memory that can be flushed in the future for triggering flushes based on immutable memory as well, and rolling back flushes causes the memtable memory to become ready for flush again for a brief period of time until it is dropped, which might wrongly affect the WBM decisions.

This is a continuation of #126 with changes that were missed there in case a CF drop was encountered after the flush was completed but before or during manifest write. Rolling back on CF drop is meaningless because a flush is not possible in that state, and the only path forwards is for the memtables to be freed shortly afterwards, so making them available for flush again is useless at best. Additionally, this might be needed for the WriteBufferManager changes in in the future for triggering flushes based on immutable memory as well, and rolling back flushes causes the memtable memory to become ready for flush again for a brief period of time until it is dropped, which might wrongly affect the WBM decisions. Finally, the code installs a new version even if no changes are made. This is unnecessary and as such that part is moved into the version-mutating code path only. This PR also adds two regression tests, so we could rely on rollback not being performed on CF drop.