Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential OOM in PopFrontDDL Method of ddlPullerImpl #11793

Open
wlwilliamx opened this issue Nov 25, 2024 · 2 comments · May be fixed by #11794
Open

Potential OOM in PopFrontDDL Method of ddlPullerImpl #11793

wlwilliamx opened this issue Nov 25, 2024 · 2 comments · May be fixed by #11794
Labels
area/ticdc Issues or PRs related to TiCDC. impact/oom severity/moderate type/bug The issue is confirmed as a bug.

Comments

@wlwilliamx
Copy link
Contributor

What did you do?

Problem

In the PopFrontDDL method of the ddlPullerImpl struct, the operation h.pendingDDLJobs = h.pendingDDLJobs[1:] is used to remove the first element from the pendingDDLJobs slice. However, this can lead to memory retention issues due to the behavior of Go slices:

  • Slices in Go are references to the underlying array. When an element is removed by slicing, the reference to the element still exists in the underlying array, even though it is no longer part of the slice.
  • As a result, the removed element (in this case, the first element of pendingDDLJobs) cannot be garbage collected, leading to unnecessary memory usage if the element is no longer needed and holds significant memory.

The problematic section of the code is as follows:

func (h *ddlPullerImpl) PopFrontDDL() (uint64, *timodel.Job) {
    h.mu.Lock()
    defer h.mu.Unlock()
    if len(h.pendingDDLJobs) == 0 {
        return atomic.LoadUint64(&h.resolvedTS), nil
    }
    job := h.pendingDDLJobs[0]
    h.pendingDDLJobs = h.pendingDDLJobs[1:] // Memory retention issue here
    return job.BinlogInfo.FinishedTS, job
}

Proposed Solution

To resolve this issue, the reference to the removed element should be explicitly set to nil before updating the slice. This will break the reference to the element, allowing the garbage collector to reclaim the memory.

Updated code:

func (h *ddlPullerImpl) PopFrontDDL() (uint64, *timodel.Job) {
    h.mu.Lock()
    defer h.mu.Unlock()
    if len(h.pendingDDLJobs) == 0 {
        return atomic.LoadUint64(&h.resolvedTS), nil
    }
    job := h.pendingDDLJobs[0]
    h.pendingDDLJobs[0] = nil // Clear reference to allow GC
    h.pendingDDLJobs = h.pendingDDLJobs[1:]
    return job.BinlogInfo.FinishedTS, job
}

This issue is not a memory leak but a memory retention problem due to Go’s slice mechanics. The fix should improve the memory efficiency of the system.

What did you expect to see?

No memory retention problem.

What did you see instead?

Memory retention problem in PopFrontDDL method.

Versions of the cluster

TiCDC version (execute cdc version):

(paste TiCDC version here)
@wlwilliamx wlwilliamx added area/ticdc Issues or PRs related to TiCDC. type/bug The issue is confirmed as a bug. labels Nov 25, 2024
@fubinzh
Copy link

fubinzh commented Nov 29, 2024

/severity moderate

@wlwilliamx
Copy link
Contributor Author

/affects 6.5 7.1 7.5 8.1 8.5

@wlwilliamx wlwilliamx changed the title Potential Memory Retention in PopFrontDDL Method of ddlPullerImpl Potential OOM in PopFrontDDL Method of ddlPullerImpl Dec 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ticdc Issues or PRs related to TiCDC. impact/oom severity/moderate type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants