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

Fix memory leak in ao_truncate_replay #11210

Merged
merged 1 commit into from Nov 25, 2020
Merged

Conversation

hidva
Copy link
Contributor

@hidva hidva commented Nov 24, 2020

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

Copy link
Contributor

@ashwinstar ashwinstar left a comment

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);
Copy link
Contributor

@ashwinstar ashwinstar Nov 24, 2020

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.

Copy link
Contributor Author

@hidva hidva Nov 25, 2020

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
@hidva
Copy link
Contributor Author

hidva commented Nov 25, 2020

Thanks for finding the issue and PR. Just curious to learn how did you uncover this to be a problem?

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.

@kainwen-zz kainwen-zz merged commit 9bb8bce into greenplum-db:master Nov 25, 2020
1 of 3 checks passed
@kainwen-zz
Copy link

kainwen-zz commented Nov 25, 2020

Nice job.

Thanks for your contribution. Pushed to master.

@ashwinstar
Copy link
Contributor

ashwinstar commented Nov 25, 2020

Backported the commit to 6X_STABLE via fa7444b

@ashwinstar
Copy link
Contributor

ashwinstar commented Nov 25, 2020

Thanks for finding the issue and PR. Just curious to learn how did you uncover this to be a problem?

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.

I enjoyed reading your blog on this @hidva (I used google translate to read it in English). Thanks for code and blog contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak in ao_truncate_replay
3 participants