Follow-up from review on PR #554 #572
Conversation
Typos and stale / inaccurate comments.
by making it a newtype for UnspentCredits rather than Credits, so when unwrapping one layer its still compatible with other credit values and we can perform operations.
We do now check that we do not do too much merging work. But we could still supply excess credits leading to unexpected leftover credits. Add TODOs in the right places about asserting there are no leftover credits. We can't assert this yet because there are in fact leftovers because we do not spread out merging work fully, in the case that runs are less than worst case in size.
These are not concurrency safe. The merging runs could complete during snapshot persistence leading to use after free. Add TODOs to explain this and to suggest a solution.
And remove a stale comment about non-zero cases and replace it by an assertion.
dcoutts
requested review from
jorisdral,
mheinzel,
recursion-ninja and
wenkokke
as code owners
| --TODO: assert leftoverCredits == 0 | ||
| -- to assert that we did not finished the merge too early, | ||
| -- and thus have spread the work out evenly. |
There was a problem hiding this comment.
I realise that you are changing the credit scaling in #571, so maybe this comment does not apply anymore, but I'll post it anyway:
I don't think this assertion will hold in the presence of exceptions, even if we fix credit scaling (a priori) to precisely add up to the a priori merging debt.
Say a table needs n updates until the next flush. We do some updates but an exception happens, so we roll back the database internal state to a consistent state. This means undoing changes to the write buffer for example, but not undoing the credits we supplied to merging runs already. Afterwards, the table still needs n updates until the next flush, but some or all of its merging runs already have an elevated count of supplied credits. This means that supplyCredits during normal operation can also return leftover credits.
Moreover, if multiple tables concurrently supply credits, we'll in general try to supply even more credits, leading to more leftovers.
If we really want to assert this, maybe we have to:
(i) ensure that scaleCreditsForMerge accounts for the elevated counts of supplied credits
(ii) undo supplied credits
Both options sound like they would be rather complex to do in a concurrent setting
| --TODO: Run.fromMutable (used in Merge.complete) claims not to be | ||
| -- exception safe so we should probably be using the resource registry | ||
| -- and test for exception safety. |
There was a problem hiding this comment.
Makes sense. We could use modifyWithActionRegistry or something similar to guarantee that the result of Run.fromMutable is put in the mergeVar.
I'm hoping we can make Run.fromMutable and Merge.complete exception safe without passing a registry into it, so that we don't have to have a registry argument in too many places, but I haven't tried that yet
Additional post-merge PR review.