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

sql: fix leak of prepared statements #33529

Conversation

andreimatei
Copy link
Contributor

The connExPrepStmtsAccessor.DeleteAll() method was not properly implemented and
"leaked" the memory accounting for prepared statements. This could lead
to crashes, like showed below.

I've looked at the various open issues around memory leak crashes, but
unfortunately I think it's unlikely this fixes those. Anyway, I'll
backport.

root@127.0.0.1:53405/defaultdb> begin; select 1;
root@127.0.0.1:53405/defaultdb OPEN> prepare x as select 1;
root@127.0.0.1:53405/defaultdb OPEN> deallocate all;
root@127.0.0.1:53405/defaultdb OPEN> commit;
root@127.0.0.1:53405/defaultdb> ^D
panic: session: unexpected 10240 leftover bytes

Release note (bug fix): Fix a memory leak around DEALLOCATE and DISCARD
statements that could result in crashes with the "unexpected
leftover bytes" message.

@andreimatei andreimatei requested review from knz and a team January 5, 2019 18:48
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

The connExPrepStmtsAccessor.DeleteAll() method was not properly implemented and
"leaked" the memory accounting for  prepared statements. This could lead
to crashes, like showed below.

I've looked at the various open issues around memory leak crashes, but
unfortunately I think it's unlikely this fixes those. Anyway, I'll
backport.

root@127.0.0.1:53405/defaultdb> begin; select 1;
root@127.0.0.1:53405/defaultdb  OPEN> prepare x as select 1;
root@127.0.0.1:53405/defaultdb  OPEN> deallocate all;
root@127.0.0.1:53405/defaultdb  OPEN> commit;
root@127.0.0.1:53405/defaultdb> ^D
panic: session: unexpected 10240 leftover bytes

Release note (bug fix): Fix a memory leak around DEALLOCATE and DISCARD
statements that could result in crashes with the "unexpected <amount>
leftover bytes" message.
Copy link
Contributor Author

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it turns out that this patch is actually broken. The patch resets one namespace to the empty namespace, but that's not something you can do. Given the current code, it's only correct to reset prepStmtsNamespace to prepStmtsNamespaceAtTxnRewindPos and vice-versa; it's not correct to reset one of those guys to some other namespace that comes from thin air cause then the reference tracking that we attempt to do gets broken. This is all undocumented and I should just be ashamed of myself for writing all this code around the namespaces the way I did.
The proper fix is #33423 which introduces reference counting for prepared statements. In conjunction with that patch, this one works properly, so I'll just fold this into #33423 (which already included this commit, but I'll squash them into one because it's now clear that they're not independent commits).

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@andreimatei andreimatei deleted the sql.prep-stmts-fix-delete-all branch January 13, 2019 18:01
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.

None yet

3 participants