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

add c-api to get default cf handle #12514

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

979357361
Copy link

@979357361 979357361 commented Apr 6, 2024

rocksdb_batched_multi_get_cf has performance improvement than normal multi_get, however it needs a cf_handle arg, so add a C-API to get and destroy the default cf_handle, as many user only use the default cf.

Fixes #12316

@ajkr ajkr changed the title add c-api to get default cf handle, solve issue #12316 add c-api to get default cf handle Apr 9, 2024
@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Comment on lines +1151 to +1152
void rocksdb_default_column_family_handle_destroy(
rocksdb_column_family_handle_t* handle) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's kind of confusing because (1) it has "destroy" in the name but doesn't go through our regular destroy path (DestroyColumnFamilyHandle() or ~ColumnFamilyHandle()); and (2) it has "default" in the name but operates on any rocksdb_column_family_handle_t*.

What do you think about simply not offering such an API, and let the user call delete handle instead?

Copy link
Author

Choose a reason for hiding this comment

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

The reason I add this different api is that, I observed that "delete handle" api will destroy the cf together with the handle, for other cf this behavior is rational because except for the handle, user has no other way to get and work on it, free the handle should also free the cf, too.
But for the default cf, things have a little change, only the new batched multi_get api need the handle arg, what if I finish the call and just want to free the memory of default cf_handle? Maybe another api name is more suitable, what do you think?
Or we can modify the batched multi_get api, if the cf_handle arg is NULL, actions will go to the default cf by default? Maybe adding a new api "rocksdb_batched_multi_get" which use the default cf is more direct, as all other APIs have two version -- with or without "_cf" ending, except for batched multi_get.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok I see the ambiguity of the term "destroy" in our C API is preexisting. How about this as the API change?

struct rocksdb_column_family_handle_t {
  ColumnFamilyHandle* rep;
  bool immortal;  // <-- New field
};

extern ROCKSDB_LIBRARY_API rocksdb_column_family_handle_t*
    rocksdb_get_default_column_family_handle(rocksdb_t* db);  // <-- New function
  • rocksdb_get_default_column_family_handle() will set immortal == true
  • Other creations of rocksdb_column_family_handle_t, where rep is not obtained by DefaultColumnFamilyHandle(), will set immortal == false
  • rocksdb_column_family_handle_destroy() will delete rep only when !immortal

Copy link
Author

Choose a reason for hiding this comment

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

OK, that seems rational, thank you.
Can I expect to see this new API in the next release?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we will be able to include it in the 9.2 release as long as we land it by Friday this week.

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

Successfully merging this pull request may close these issues.

How can I get the default CF_handle by C API?
3 participants