seg: set stale files not visible until recalcVisibleFiles done

[stevemilk]

No description provided.

[AskAlexSharov]

FYI: seems i trying something similar: #12380

[stevemilk]

FYI: seems i trying something similar: #12380

@AskAlexSharov Got it. one thing special in this PR is i try to keep closeAndRemoveFiles thread safe without lock, not sure if it's within your next step 's plan, so explain a bit:

I add an indicator stale in visibleSegment struct. After recalcVisibleSegments, unused old visible segments will be marked as stale. closeAndRemoveFiles happens only when refcount==0 && stale && src.canDelete. This can thread safe without lock because new rotx will be unable to read stale visible Segments.

[AskAlexSharov]

I don’t understand how it can be called from multiple goroutines.

In my head 2parts
1:
// recalcVisibleFiles
do skip CanDelete==true
// means new RoTx can’t see it

2:
newValue := refcnt.AtomicDecrement()
If newValue == 0 {
closeAndDel()
}

Atomics guarantee that all goroutines (and there can’t be new readers because point 1).

[stevemilk]

what I'm thinking here is exactly point1, canDelete==true seems not enough.

assuming two consecutive readers read segX during merge:

1. merge step1: integrateMergedDirtyFiles marks segX as canDelete = true
2. rotx_1 read, refCnt=1
3. rotx_1 closing, refCnt=0 and canDelete = true
4. rotx_2 read, refCnt=1, because recalcVisibleFiles has not been called yet, segX is still visible
5. rotx_1 closeAndRemove segX
6. rotx_2 access nil segX
7. merge step2: recalcVisibleFiles, segX is removed from visible list

[AskAlexSharov]

rotx_1 read, refCnt=1 and rotx_2 read, refCnt=1 - "read atomics" it's kind-of anti-pattern. Need do "compare_and_swap" things on them: we do leftReaders := idx.readAheadRefcnt.Add(-1)

merge itself is also reader Aggregator.mergeLoopStep does aggTx := a.BeginFilesRo()
(i'm not sure if RoSnapshots follow this practice - i need re-check)

You right - we can make sure that we set canDelete.Store(true) after recalcVisibleFiles:

a.integrateMergedDirtyFiles(outs, in) // can don't set `canDelete`
a.recalcVisibleFiles(a.DirtyFilesEndTxNumMinimax()) 
a.cleanAfterMerge(in) // set `canDelete`

(it happening inside a.BeginFilesRo() section)

[stevemilk]

thanks, you're correct about merge, i will work on point3 later

[AskAlexSharov]

Auite a lot of changes landed in: #12339
need somehow merge main