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
Fix memory leak in ao_truncate_replay #11210
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding the issue and PR. Just curious to learn how did you uncover this to be a problem?
@@ -140,6 +140,7 @@ ao_truncate_replay(XLogReaderState *record) | |||
snprintf(path, MAXPGPATH, "%s/%u", dbPath, xlrec->target.node.relNode); | |||
else | |||
snprintf(path, MAXPGPATH, "%s/%u.%u", dbPath, xlrec->target.node.relNode, xlrec->target.segment_filenum); | |||
pfree(dbPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if we add dbPath=NULL;
after pfree to make sure no code logic tries to use it after free. Will add that and commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
The value returned by GetDatabasePath is palloc'd, and CurrentMemoryContext is TopMemoryContext when we enter ao_truncate_replay(), so we should do a pfree. Fix greenplum-db#11202
The detailed story is described in https://blog.hidva.com/2020/11/24/memory-leak/ (but written in Chinese). The simple version is that we receive an alert that tells us the used memory of some nodes has been slowly rising in the past 30 days. |
Nice job. Thanks for your contribution. Pushed to master. |
Backported the commit to 6X_STABLE via fa7444b |
I enjoyed reading your blog on this @hidva (I used google translate to read it in English). Thanks for code and blog contribution. |
The value returned by GetDatabasePath is palloc'd, and
CurrentMemoryContext is TopMemoryContext when we enter
ao_truncate_replay(), so we should do a pfree.
Fix #11202