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

[ysql] Import Fix plancache refcount leak after error in ExecuteQuery. #8962

Closed
tedyu opened this issue Jun 17, 2021 · 0 comments
Closed

[ysql] Import Fix plancache refcount leak after error in ExecuteQuery. #8962

tedyu opened this issue Jun 17, 2021 · 0 comments
Assignees

Comments

@tedyu
Copy link
Contributor

tedyu commented Jun 17, 2021

Commit was 9cf163266017ec6d0e190313cce1d417cbf4a549

When stuffing a plan from the plancache into a Portal, one is
not supposed to risk throwing an error between GetCachedPlan and
PortalDefineQuery; if that happens, the plan refcount incremented
by GetCachedPlan will be leaked.  I managed to break this rule
while refactoring code in 9dbf2b7d7.  There is no visible
consequence other than some memory leakage, and since nobody is
very likely to trigger the relevant error conditions many times
in a row, it's not surprising we haven't noticed.  Nonetheless,
it's a bug, so rearrange the order of operations to remove the
hazard.

Noted on the way to looking for a better fix for bug #17053.
This mistake is pretty old, so back-patch to all supported
branches.
tedyu added a commit that referenced this issue Jun 23, 2021
…teQuery.

Summary:
Commit was 9cf163266017ec6d0e190313cce1d417cbf4a549

Commit message was:

When stuffing a plan from the plancache into a Portal, one is
not supposed to risk throwing an error between GetCachedPlan and
PortalDefineQuery; if that happens, the plan refcount incremented
by GetCachedPlan will be leaked.  I managed to break this rule
while refactoring code in 9dbf2b7d7.  There is no visible
consequence other than some memory leakage, and since nobody is
very likely to trigger the relevant error conditions many times
in a row, it's not surprising we haven't noticed.  Nonetheless,
it's a bug, so rearrange the order of operations to remove the
hazard.

Noted on the way to looking for a better fix for bug #17053.
This mistake is pretty old, so back-patch to all supported
branches.

Test Plan: Build Yugabyte DB and verify there is no test regression.

Reviewers: jason

Reviewed By: jason

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D11984
@tedyu tedyu closed this as completed Jun 23, 2021
@tedyu tedyu self-assigned this Jul 13, 2021
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

No branches or pull requests

1 participant