I'd rather tried to get rid of all allocations (as much as possible) in the OvercommitMemoryTracker. Because that code will work under high memory pressure and that could be an issue when we are trying to allocate more memory when we have to free it for another query.

@nikitamikhaylov The problem here is that we have logging inside of the OvercommitTracker (for debug purposes). And logging may lead to the allocation of the memory. I can't get rid of these allocations (if we remove logs from here, I don't know how to debug it).
We definitely shouldn't have in call stack something like: OvercommitTracker -> Logging -> OvercommitTracker. This PR tries to do it correctly.

@novikd, there's another problem and it's not only about logging. When some code does memory allocations it may throw exceptions like MEMORY_LIMIT_EXCEEDED/CANNOT_ALLOCATE_MEMORY/std::bad_alloc/etc from unexpected places. It may cause obscure and hard-to-debug issues on high memory pressure if that code is not exception-safe. For example:

Looks like the line 68 may allocate memory and therefore throw (at least it's not clear from the code why it cannot). In this case changes to allow_release and required_memory will not be undone. Consider using DENY_ALLOCATIONS_IN_SCOPE or NOEXCEPT_SCOPE macros here to avoid such issues.

@tavplubix @nikitamikhaylov I removed usage of unordered_map in OvercommitTracker, and now we have no allocations there. But at some point, we'd like to have a FIFO order for thread releasing, which will highly likely require memory allocations.

and now we have no allocations there

Let's add DENY_ALLOCATIONS_IN_SCOPE just to be sure

Isn't logging itself doing allocations when formatting strings?

Isn't logging itself doing allocations when formatting strings?

It is. We can use something like

DENY_ALLOCATIONS_IN_SCOPE;
doCriticalStuff();
{
    ALLOW_ALLOCATIONS_IN_SCOPE;
    LOG_INFO(log, "It can do allocations");
}
continueDoingCriticalStuff();

Or a bit more paranoid:

DENY_ALLOCATIONS_IN_SCOPE;
doCriticalStuff();
{
    NOEXCEPT_SCOPE;
    /// logging is done in try-catch, so we don't expect any exceptions here
    ALLOW_ALLOCATIONS_IN_SCOPE;
    LOG_INFO(log, "It can do allocations");
}
continueDoingCriticalStuff();

Maybe do it in the macro, like this:

  #define LOG_DEBUG_SAFE(...)                       \
    do {                                            \
        OvercommitTrackerBlockerInThread blocker;   \
        try                                         \ 
        {                                           \
            ALLOW_ALLOCATIONS_IN_SCOPE;             \
            LOG_DEBUG(__VA_ARGS__);                 \
        }                                           \
        catch (std::badalloc &)                     \
        {                                           \
             fprintf(stderr, "Allocation failed when writing to log in OvercommitTracker\n");   \
        }                                           \
    } while (false)

@Mergifyio update

update

✅ Branch has been successfully updated

@Mergifyio update

update

✅ Branch has been successfully updated

PullRequestCI / TestsBugfixCheck (pull_request) Failing after 27s — TestsBugfixCheck

Ok. No tests added.