VersionBuilder::Rep::LoadTableHandlers() makes preloaded files permanent in table cache

We have been very late in upgrading from RocksDB 5.x to 6.x. Only did so last month. This bug was part of the 6.0.0 (2019-02-19) release. The comments within LoadTableHandlers() are:

// If the table cache is not 1/4 full, we pin the table handle to
// file metadata to avoid the cache read costs when reading the file.
// The downside of pinning those files is that LRU won't be followed
// for those files. This doesn't matter much because if number of files
// of the DB excceeds table cache capacity, eventually no table reader
// will be pinned and LRU will be followed.

Sadly, the last sentence is incorrect. As released, the pinning done within this function is permanent. The extra cache handle creates an extra reference count within the cache object. Nothing but shutdown will release the cache object. I have provided a diff below that corrects the cache management:

diff --git a/db/version_builder.cc b/db/version_builder.cc

index 44229eefc..72165689f 100644

--- a/db/version_builder.cc

+++ b/db/version_builder.cc

@@ -969,11 +969,17 @@ class VersionBuilder::Rep {

             true /* record_read_stats */,

             internal_stats->GetFileReadHist(level), false, level,

             prefetch_index_and_filter_in_cache, max_file_size_for_l0_meta_pin);


         if (file_meta->table_reader_handle != nullptr) {

           // Load table_reader

+        if (always_load) {  // CompactedDB usage ... especially "make check"

               file_meta->fd.table_reader = table_cache_->GetTableReaderFromHandle(

               file_meta->table_reader_handle);

+       } else {

+            table_cache_->ReleaseHandle(file_meta->table_reader_handle);

+            file_meta->table_reader_handle = nullptr;

+        }

         }

       }

     });

This is Saturday. I will dig around to find a related unit test to extend and test for the extra reference. Will do this Sunday or Monday.

I suggest there are two alternative fixes:
- take my change as is
- add a new Options::disable-preload-pinning if the current behavior is actually desired.

Would appreciate comments about the alternatives before I start a PR.

Background: This bug has an extreme impact in our environment. We dynamically tune memory allocations between table readers, write buffers, and block caches. Our code opens with a large max_open_files: 524238. When memory starts to get tight we adjust that number down to reduce memory used by table readers. A test case this week pushed max_open_files down to 409. LoadTableHandlers() had already pinned more than 409 files. Many subsequent read operations became a new file opens, more than 2,000 per 10 second interval. Performance tanked.