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

Refactor RaftGroupMetadata to avoid keeping unnecessary TableInfo objects in memory #4354

Closed
hectorgcr opened this issue Apr 30, 2020 · 0 comments
Assignees
Labels
area/docdb YugabyteDB core features

Comments

@hectorgcr
Copy link
Contributor

Currently , RaftGroupMetadata class has a unique_ptr to a TableInfo object. The problem is that this object sometimes needs to be kept alive to avoid race conditions, so when an AlterTable or a remote bootstrap happens, a TableInfo objects is stored in a vector called old_tables just in case someone is referencing it through a raw pointer. We should redesign this to use shared_ptrs instead.

Another problem is that the RaftGroupMetadata class provides reference access to fields from the main's table TableInfo object, e.g. schema(), partition_schema(), so if we make the change described above, we also need to make sure that these methods don't return a const reference because by doing so, we won't increase the ref count of the TableInfo object, and the caller could end up with a dangling reference if the TableInfo object is suddenly destroyed. We should solve this by returning shared_ptrs to such fields from TableInfo. These fields could also be changed to be shared_ptrs, or we could construct these shared_ptrs on the fly by using the aliasing shared_ptr constructor:

template< class Y >
shared_ptr( const shared_ptr<Y>& r, element_type* ptr ) noexcept;
@hectorgcr hectorgcr self-assigned this Apr 30, 2020
@hectorgcr hectorgcr added the area/docdb YugabyteDB core features label Apr 30, 2020
hectorgcr added a commit that referenced this issue May 11, 2020
…nfo objects in memory

Summary:
Currently , `RaftGroupMetadata` class has a unique_ptr to a TableInfo object.
The problem is that this object sometimes needs to be kept alive to avoid race
conditions, so when an AlterTable or a remote bootstrap happens, a TableInfo
objects is stored in a vector called `old_tables` just in case someone is
referencing it through a raw pointer. Changing this to use shared_ptrs instead.

Another problem is that the `RaftGroupMetadata` class provides reference
access to fields from the main's table TableInfo object, e.g. `schema()`,
`partition_schema()`, so if we make the change described above, we also need
to make sure that these methods don't return a const reference because by
 doing so, we won't increase the ref count of the TableInfo object, and the caller
 could end up with a dangling reference if the TableInfo object is suddenly destroyed.
Changing this to return a shared_ptr for such fields from `RaftGroupMetadata`.

Test Plan: Unit tests

Reviewers: timur, bogdan, sergei

Reviewed By: sergei

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D8403
hectorgcr added a commit that referenced this issue May 14, 2020
Summary:
Access TableInfo object explicitly for any call that gets the IndexMap. This fixes ASAN issues created by  https://detective-gcp.dev.yugabyte.com/D8403.

Test Plan: ybd asan --cxx-test integration-tests_cassandra_cpp_driver-test --gtest_filter CppCassandraDriverTest.TestTableCreateIndexUserEnforced -n 100

Reviewers: amitanand, sergei, bogdan

Reviewed By: sergei

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D8467
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docdb YugabyteDB core features
Projects
None yet
Development

No branches or pull requests

2 participants