db_bench: fix for issue #290

[andy-byers]

Changed db_bench so that when using multiple databases (num_multi_db > 1) we only destroy databases that were used for the current group (databases specified in dbs_to_use).

[udi-speedb]

Hi @andy-byers,
Thanks a lot for working on this issue and submitting the PR.
Your PR failed our checks due to a minor technical issue. Did you get an email from GitHub with a notification about it?
Anyway, the way to fix that is to run the command:
make format
This command runs automatic code formatting checks that make sure the code conforms to certain coding conventions, and proposes to "fix" your code. You can just allow the tool to automatically fix your code and push your update.
If anything is unclear or you need assistance of any kind, please let us know.
Thanks

[andy-byers]

Hey @udi-speedb, thanks for the feedback. I'll run make format and push again.

[andy-byers]

I figured I would explain some of my reasoning behind these changes. First, in OpenAllDbs(), we will only open databases in dbs_ that were not already opened (both their db and opt_txn_db members are nullptr). Then, in DestroyUsedDbs() (renamed from DestroyAllDbs()), we destroy the databases whose indices are in db_idxs_to_use. Instead of calling the free function DeleteDbs() which clears dbs_, we call the DeleteDbs() member of the specific DBWithColumnFamilies instances we want to delete, which makes sure both the db and opt_txn_db members are set to nullptr. Note that FLAGS_num_multi_db won't ever change between groups, so we won't leak opened but unused databases in OpenAllDbs(). Also, the free DeleteDBs() is run in the benchmark class destructor, so all databases should be cleaned up at the end. Let me know if there are any questions or concerns.

On a side note, it doesn't look like there are any tests in db_bench_tool_test.cc that exercise the -num_multi_db or -dbs_to_use options. I'd be happy to write some if that is deemed necessary. Thanks!

[udi-speedb]

Please wrap the reference to opt_txn_db with #ifndef ROCKSDB_LITE / #endif

[andy-byers]

I totally missed that, thank you. As for your comment below, you're right, the db member needs to be nulled out unconditionally. Then the check for opt_txn_db can be removed.

[udi-speedb]

I totally missed that, thank you. As for your comment below, you're right, the db member needs to be nulled out unconditionally. Then the check for opt_txn_db can be removed.

However, please do not change that as part of this PR. This is a RocksDB issue (=> upstreamable from our standpoint) and, therefore, should be handled in a separate issue (dbs_to_use_ is a Speedb only addition).
I will open a separate issue for that.

[andy-byers]

Sounds good. I just pushed a fix for this change. Does that look okay? It should be sufficient until the other issue is fixed, at which point the code can be simplified as discussed.

[udi-speedb]

Sounds good. I just pushed a fix for this change. Does that look okay? It should be sufficient until the other issue is fixed, at which point the code can be simplified as discussed.

Thanks. I only now thought about it, but couldn't you just replace the following block of code:

#ifndef ROCKSDB_LITE
        if (FLAGS_optimistic_transaction_db) {
          if (dbs_[i].opt_txn_db) {
            continue;
          }
        } else if (dbs_[i].db) {
          continue;
        }
#else   // ROCKSDB_LITE
        if (dbs_[i].db) {
          continue;
        }
#endif  // ROCKSDB_LITE

With this:

if (dbs_[i].db) {
  continue;
}

[udi-speedb]

Thanks. I understand.

I have opened #374 to fix the RocksDB issue (nullifying db_)

Please let me know if you would you like to add tests as part of this PR. If not, I will approve it as is. I can open another issue to add the tests.

[andy-byers]

I was wondering, do you know how the benchmark tests are built and run? db_bench_tool_test is provided as a target in the root Makefile, but isn't mentioned anywhere else that I could find. I built it and ran it (from main) with no arguments, and it actually trips some assertions. Is it supposed to be run with some arguments?

I think adding those tests might be a bit more complicated than I originally thought, and would probably be better off in a different issue.

[udi-speedb]

I am not familiar with db_bench_test_tool so I can't help you there. I will look at it in the near future to get to know it.
Thanks so much for working on this issue.
Please squash your commits and also put the issue number in parenthesis in the commit message (#290). I will then approve it.

[andy-byers]

I had to force push, but I got everything squashed and fixed the commit message.

Thank you for your help! I'll continue to look into the benchmark tests and file an issue if necessary.

[udi-speedb]

Thanks so much again for working on this.
Approving the PR.

[udi-speedb]

@andy-byers Thanks for fixing the formatting issue and for the detailed explanation of your changes.
Other than the minor comment I have given, it looks good to me.
Unrelated to your changes, but something I have come across during the review, and would like to hear your opinion about, is the code in DBWithColumnFamilies::DeleteDBs():

#ifndef ROCKSDB_LITE    
   if (opt_txn_db) {    
      delete opt_txn_db;    
      opt_txn_db = nullptr;    
    } else {    
      delete db;    
      db = nullptr;    
    }    
#else    

Shouldn't the db member be unconditionally set to nullptr (even when opt_txn_db != nullptr)?

[udi-speedb]

@andy-byers As for your proposal to add tests in db_bench_tool_test.cc, that would be great.
If you are willing to do so, would you like to add them as part of this PR or do you prefer that we open a separate issue for those?