box: don't delete inprogress files during normal GC #6555
Conversation
9951e43 to
9b89d26
Compare
| @@ -372,6 +372,7 @@ memtx_engine_end_recovery(struct engine *engine) | |||
| if (space_foreach(memtx_build_secondary_keys, memtx) != 0) | |||
| return -1; | |||
| } | |||
| xdir_collect_inprogress(&memtx->snap_dir); | |||
There was a problem hiding this comment.
AFAIU this was moved from memtx_engine_end_recovery in the commit you mentioned, because memtx_engine_end_recovery happens too early. With your patch applied the inprogress files left by the main node won't be removed by the hot_standby node.
There was a problem hiding this comment.
I mean xdir_collect_inprogress should be called somewhere around these lines: https://github.com/tarantool/tarantool/blob/master/src/box/box.cc#L3441-L3449
So I propose to move it to recovery_finalize or wal_enable. Both functions seem suitable to me.
Or maybe extract xdir_collect_inprogress and call it explicitly.
There was a problem hiding this comment.
BTW, the commit you mention (5aa243d) also added engine_collect_garbage to gc_init: 5aa243d#diff-0bcbae7cd0018b8d3a9816c5ed98dc346f51946ea7be03730fff798c603c2da2R116
This seems useless, since gc_init is called even before engine_init.
I propose you remove that extraneous engine_collect_garbage call as well.
There was a problem hiding this comment.
The commit you mention also added engine_collect_garbage to gc_init. I propose you remove that extraneous engine_collect_garbage call as well.
Good catch. Looks like it was added there to make GC tests pass. This is not needed now. Removed. Thanks!
There was a problem hiding this comment.
With your patch applied the inprogress files left by the main node won't be removed by the hot_standby node.
Right, we shouldn't run xdir_collect_garbage until the end of the hot standby mode, because we risk deleting files that are still in use by the master instance. Thanks for catching this!
I moved engine_end_recovery back to its old place - after hot standby completes - and introduced a new engine callback for memtx to build its secondary keys before hot standby - engine_begin_hot_standby. PTAL.
9b89d26 to
05cda47
Compare
05cda47 to
2f5da36
Compare
*.inprogress files shouldn't be deleted during normal GC, because they may be in use. E.g. GC may happen to run while WAL is rotating xlog; if GC removes the transient xlog.inprogress file created by WAL (see xdir_create_xlog), WAL will fail to rename it and abort the current transaction. Initially, inprogress files were removed only after recovery. For this reason, xdir_collect_inprogress, which is used for deleting inprogress files, accesses FS directly, without COIO. This was changed by commit 5aa243d ("recovery: build secondary index in hot standby mode") which moved xdir_collect_inprogress from memtx_engine_end_recovery to memtx_engine_collect_garbage so that a hot standby instance doesn't delete inprogress files of the master instance by mistake. To fix this issue, let's move xdir_collect_inprogress back where it belongs, to engine_end_recovery, and introduce a new callback for memtx to build its secondary keys before entering the hot standby mode - engine_begin_hot_standby. Let's also remove engine_collect_garbage from gc_init, which was added there by the aforementioned commit, probably to make tests pass. The bug was reported by the vinyl/deferred_delete test (tarantool#5089). Closes tarantool#6554
2f5da36 to
16b7b65
Compare
|
This got to be part of 1.10 |
1.10 isn't affected, because it doesn't have the patch with hot standby instance building secondary indices. |
*.inprogress files shouldn't be deleted during normal GC, because they
may be in use. E.g. GC may happen to run while WAL is rotating xlog;
if GC removes the transient xlog.inprogress file created by WAL (see
xdir_create_xlog), WAL will fail to rename it and abort the current
transaction.
Initially, inprogress files were removed only after recovery. For this
reason, xdir_collect_inprogress, which is used for deleting inprogress
files, accesses FS directly, without COIO. This was changed by commit
5aa243d ("recovery: build secondary
index in hot standby mode") which moved xdir_collect_inprogress from
memtx_engine_end_recovery to memtx_engine_collect_garbage so that a hot
standby instance doesn't delete inprogress files of the master instance
by mistake.
To fix this issue, let's move xdir_collect_inprogress back where it
belongs, to engine_end_recovery, and introduce a new callback for memtx
to build its secondary keys before entering the hot standby mode -
engine_begin_hot_standby.
Let's also remove engine_collect_garbage from gc_init, which was added
there by the aforementioned commit, probably to make tests pass.
The bug was reported by the vinyl/deferred_delete test (#5089).
Closes #6554