diff --git a/rocksdb/columnfamily/cfopts.nim b/rocksdb/columnfamily/cfopts.nim index f73c8f6..d32b431 100644 --- a/rocksdb/columnfamily/cfopts.nim +++ b/rocksdb/columnfamily/cfopts.nim @@ -11,12 +11,13 @@ import ../lib/librocksdb, ../internal/utils, ../options/tableopts +export tableopts + type SlicetransformPtr* = ptr rocksdb_slicetransform_t SlicetransformRef* = ref object cPtr: SlicetransformPtr - autoClose*: bool # if true then close will be called when it's parent is closed ColFamilyOptionsPtr* = ptr rocksdb_options_t @@ -25,7 +26,6 @@ type # type - CF options are a subset of rocksdb_options_t - when in doubt, check: # https://github.com/facebook/rocksdb/blob/b8c9a2576af6a1d0ffcfbb517dfcb7e7037bd460/include/rocksdb/options.h#L66 cPtr: ColFamilyOptionsPtr - sliceTransform: SlicetransformRef tableOpts: TableOptionsRef autoClose*: bool # if true then close will be called when the database is closed @@ -40,11 +40,8 @@ type xpressCompression = rocksdb_xpress_compression zstdCompression = rocksdb_zstd_compression -proc createFixedPrefix*(value: int, autoClose = false): SlicetransformRef = - SlicetransformRef( - cPtr: rocksdb_slicetransform_create_fixed_prefix(value.csize_t), - autoClose: autoClose, - ) +proc createFixedPrefix*(value: int): SlicetransformRef = + SlicetransformRef(cPtr: rocksdb_slicetransform_create_fixed_prefix(value.csize_t)) proc isClosed*(s: SlicetransformRef): bool {.inline.} = s.cPtr.isNil() @@ -73,7 +70,6 @@ proc close*(cfOpts: ColFamilyOptionsRef) = rocksdb_options_destroy(cfOpts.cPtr) cfOpts.cPtr = nil - autoCloseNonNil(cfOpts.sliceTransform) autoCloseNonNil(cfOpts.tableOpts) template opt(nname, ntyp, ctyp: untyped) = @@ -135,11 +131,14 @@ proc defaultColFamilyOptions*(autoClose = false): ColFamilyOptionsRef = proc `setPrefixExtractor`*(cfOpts: ColFamilyOptionsRef, value: SlicetransformRef) = doAssert not cfOpts.isClosed() - doAssert cfOpts.sliceTransform.isNil() - # don't allow overwriting an existing sliceTransform which could leak memory + # Destroys the existing slice transform if there is one attached to the column + # family options and takes ownership of the passed slice transform. After this call, + # the ColFamilyOptionsRef is responsible for cleaning up the policy when it is no + # longer needed so we set the filter policy to nil so that isClosed() will return true + # and prevent the filter policy from being double freed which was causing a seg fault. rocksdb_options_set_prefix_extractor(cfOpts.cPtr, value.cPtr) - cfOpts.sliceTransform = value + value.cPtr = nil proc `blockBasedTableFactory=`*( cfOpts: ColFamilyOptionsRef, tableOpts: TableOptionsRef diff --git a/rocksdb/options/cache.nim b/rocksdb/options/cache.nim index 2024852..b266149 100644 --- a/rocksdb/options/cache.nim +++ b/rocksdb/options/cache.nim @@ -10,6 +10,9 @@ type proc cacheCreateLRU*(size: int, autoClose = false): CacheRef = CacheRef(cPtr: rocksdb_cache_create_lru(size.csize_t), autoClose: autoClose) +proc isClosed*(cache: CacheRef): bool = + isNil(cache.cPtr) + proc close*(cache: CacheRef) = if cache.cPtr != nil: rocksdb_cache_destroy(cache.cPtr) diff --git a/rocksdb/options/tableopts.nim b/rocksdb/options/tableopts.nim index c942b01..1d91b29 100644 --- a/rocksdb/options/tableopts.nim +++ b/rocksdb/options/tableopts.nim @@ -1,5 +1,7 @@ import ../lib/librocksdb, ../internal/utils, ./cache +export cache + type # TODO might eventually wrap this TableOptionsPtr* = ptr rocksdb_block_based_table_options_t diff --git a/tests/columnfamily/test_cfopts.nim b/tests/columnfamily/test_cfopts.nim index 28d25a3..5b5d2d6 100644 --- a/tests/columnfamily/test_cfopts.nim +++ b/tests/columnfamily/test_cfopts.nim @@ -20,3 +20,45 @@ suite "ColFamilyOptionsRef Tests": check cfOpts.isClosed() cfOpts.close() check cfOpts.isClosed() + + test "Test auto close enabled": + let + cfOpts = defaultColFamilyOptions() + tableOpts = defaultTableOptions(autoClose = true) + sliceTransform = createFixedPrefix(1000) + + cfOpts.blockBasedTableFactory = tableOpts + cfOpts.setPrefixExtractor(sliceTransform) + + check: + cfOpts.isClosed() == false + tableOpts.isClosed() == false + sliceTransform.isClosed() == true # closed because tableopts takes ownership + + cfOpts.close() + + check: + cfOpts.isClosed() == true + tableOpts.isClosed() == true + sliceTransform.isClosed() == true + + test "Test auto close disabled": + let + cfOpts = defaultColFamilyOptions() + tableOpts = defaultTableOptions(autoClose = false) + sliceTransform = createFixedPrefix(1000) + + cfOpts.blockBasedTableFactory = tableOpts + cfOpts.setPrefixExtractor(sliceTransform) + + check: + cfOpts.isClosed() == false + tableOpts.isClosed() == false + sliceTransform.isClosed() == true # closed because tableopts takes ownership + + cfOpts.close() + + check: + cfOpts.isClosed() == true + tableOpts.isClosed() == false + sliceTransform.isClosed() == true diff --git a/tests/options/test_dbopts.nim b/tests/options/test_dbopts.nim index b0f9513..c1fb261 100644 --- a/tests/options/test_dbopts.nim +++ b/tests/options/test_dbopts.nim @@ -13,7 +13,7 @@ import unittest2, ../../rocksdb/options/dbopts suite "DbOptionsRef Tests": test "Test newDbOptions": - var dbOpts = newDbOptions() + let dbOpts = newDbOptions() check not dbOpts.cPtr.isNil() @@ -28,10 +28,27 @@ suite "DbOptionsRef Tests": dbOpts.close() test "Test close": - var dbOpts = defaultDbOptions() + let dbOpts = defaultDbOptions() check not dbOpts.isClosed() dbOpts.close() check dbOpts.isClosed() dbOpts.close() check dbOpts.isClosed() + + test "Test auto close enabled": + let + dbOpts = defaultDbOptions() + cache = cacheCreateLRU(1000, autoClose = true) + + dbOpts.rowCache = cache + + check: + dbOpts.isClosed() == false + cache.isClosed() == false + + dbOpts.close() + + check: + dbOpts.isClosed() == true + cache.isClosed() == true diff --git a/tests/options/test_tableopts.nim b/tests/options/test_tableopts.nim new file mode 100644 index 0000000..076fbf2 --- /dev/null +++ b/tests/options/test_tableopts.nim @@ -0,0 +1,80 @@ +# Nim-RocksDB +# Copyright 2024 Status Research & Development GmbH +# Licensed under either of +# +# * Apache License, version 2.0, ([LICENSE-APACHE](LICENSE-APACHE) or http://www.apache.org/licenses/LICENSE-2.0) +# * GPL license, version 2.0, ([LICENSE-GPLv2](LICENSE-GPLv2) or https://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html) +# +# at your option. This file may not be copied, modified, or distributed except according to those terms. + +{.used.} + +import unittest2, ../../rocksdb/options/tableopts + +suite "TableOptionsRef Tests": + test "Test createTableOptions": + let tableOpts = createTableOptions() + + check not tableOpts.cPtr.isNil() + + tableOpts.close() + + test "Test defaultTableOptions": + let tableOpts = defaultTableOptions() + + check not tableOpts.cPtr.isNil() + + tableOpts.close() + + test "Test close": + let tableOpts = defaultTableOptions() + + check not tableOpts.isClosed() + tableOpts.close() + check tableOpts.isClosed() + tableOpts.close() + check tableOpts.isClosed() + + test "Test auto close enabled": + # TODO: enable filter policy once creating updated DLL build + let + tableOpts = defaultTableOptions() + cache = cacheCreateLRU(1000, autoClose = true) + # filter = createRibbon(9.9, autoClose = true) + + tableOpts.blockCache = cache + # tableOpts.filterPolicy = filter + + check: + tableOpts.isClosed() == false + cache.isClosed() == false + # filter.isClosed() == true # closed because tableopts takes ownership + + tableOpts.close() + + check: + tableOpts.isClosed() == true + cache.isClosed() == true + # filter.isClosed() == true + + test "Test auto close disabled": + # TODO: enable filter policy once creating updated DLL build + let + tableOpts = defaultTableOptions() + cache = cacheCreateLRU(1000, autoClose = false) + # filter = createRibbon(9.9, autoClose = true) + + tableOpts.blockCache = cache + # tableOpts.filterPolicy = filter + + check: + tableOpts.isClosed() == false + cache.isClosed() == false + # filter.isClosed() == true # closed because tableopts takes ownership + + tableOpts.close() + + check: + tableOpts.isClosed() == true + cache.isClosed() == false + # filter.isClosed() == true diff --git a/tests/test_all.nim b/tests/test_all.nim index f236e72..3b6287a 100644 --- a/tests/test_all.nim +++ b/tests/test_all.nim @@ -17,6 +17,7 @@ import ./options/test_dbopts, ./options/test_readopts, ./options/test_writeopts, + ./options/test_tableopts, ./transactions/test_txdbopts, ./transactions/test_txopts, ./test_backup, diff --git a/tests/test_backup.nim b/tests/test_backup.nim index b2ba55a..4e50650 100644 --- a/tests/test_backup.nim +++ b/tests/test_backup.nim @@ -59,3 +59,33 @@ suite "BackupEngineRef Tests": check engine.isClosed() engine.close() check engine.isClosed() + + test "Test auto close enabled": + let + backupOpts = defaultBackupEngineOptions(autoClose = true) + backupEngine = openBackupEngine(dbPath, backupOpts).get() + + check: + backupOpts.isClosed() == false + backupEngine.isClosed() == false + + backupEngine.close() + + check: + backupOpts.isClosed() == true + backupEngine.isClosed() == true + + test "Test auto close disabled": + let + backupOpts = defaultBackupEngineOptions(autoClose = false) + backupEngine = openBackupEngine(dbPath, backupOpts).get() + + check: + backupOpts.isClosed() == false + backupEngine.isClosed() == false + + backupEngine.close() + + check: + backupOpts.isClosed() == false + backupEngine.isClosed() == true diff --git a/tests/test_rocksdb.nim b/tests/test_rocksdb.nim index 2d58052..160609a 100644 --- a/tests/test_rocksdb.nim +++ b/tests/test_rocksdb.nim @@ -380,7 +380,7 @@ suite "RocksDbRef Tests": test "Test auto close disabled": let - dbPath = mkdtemp() / "autoclose-enabled" + dbPath = mkdtemp() / "autoclose-disabled" dbOpts = defaultDbOptions(autoClose = false) readOpts = defaultReadOptions(autoClose = false) writeOpts = defaultWriteOptions(autoClose = false) diff --git a/tests/test_sstfilewriter.nim b/tests/test_sstfilewriter.nim index f21e197..187affe 100644 --- a/tests/test_sstfilewriter.nim +++ b/tests/test_sstfilewriter.nim @@ -86,3 +86,33 @@ suite "SstFileWriterRef Tests": check writer.isClosed() writer.close() check writer.isClosed() + + test "Test auto close enabled": + let + dbOpts = defaultDbOptions(autoClose = true) + writer = openSstFileWriter(sstFilePath, dbOpts).get() + + check: + dbOpts.isClosed() == false + writer.isClosed() == false + + writer.close() + + check: + dbOpts.isClosed() == true + writer.isClosed() == true + + test "Test auto close disabled": + let + dbOpts = defaultDbOptions(autoClose = false) + writer = openSstFileWriter(sstFilePath, dbOpts).get() + + check: + dbOpts.isClosed() == false + writer.isClosed() == false + + writer.close() + + check: + dbOpts.isClosed() == false + writer.isClosed() == true diff --git a/tests/test_transactiondb.nim b/tests/test_transactiondb.nim index c991c73..2ae0132 100644 --- a/tests/test_transactiondb.nim +++ b/tests/test_transactiondb.nim @@ -202,21 +202,96 @@ suite "TransactionDbRef Tests": tx2.close() check tx2.isClosed() - test "Test auto close": + test "Test auto close enabled": let - dbPath = mkdtemp() / "autoclose" - dbOpts = defaultDbOptions(autoClose = false) + dbPath = mkdtemp() / "autoclose-enabled" + dbOpts = defaultDbOptions(autoClose = true) txDbOpts = defaultTransactionDbOptions(autoClose = true) - db = openTransactionDb(dbPath, dbOpts, txDbOpts).get() + columnFamilies = + @[ + initColFamilyDescriptor(CF_DEFAULT, defaultColFamilyOptions(autoClose = true)) + ] + db = openTransactionDb(dbPath, dbOpts, txDbOpts, columnFamilies).get() check: dbOpts.isClosed() == false txDbOpts.isClosed() == false + columnFamilies[0].isClosed() == false db.isClosed() == false db.close() check: - dbOpts.isClosed() == false + dbOpts.isClosed() == true txDbOpts.isClosed() == true + columnFamilies[0].isClosed() == true + db.isClosed() == true + + test "Test auto close enabled": + let + dbPath = mkdtemp() / "autoclose-disabled" + dbOpts = defaultDbOptions(autoClose = false) + txDbOpts = defaultTransactionDbOptions(autoClose = false) + columnFamilies = + @[ + initColFamilyDescriptor( + CF_DEFAULT, defaultColFamilyOptions(autoClose = false) + ) + ] + db = openTransactionDb(dbPath, dbOpts, txDbOpts, columnFamilies).get() + + check: + dbOpts.isClosed() == false + txDbOpts.isClosed() == false + columnFamilies[0].isClosed() == false + db.isClosed() == false + + db.close() + + check: + dbOpts.isClosed() == false + txDbOpts.isClosed() == false + columnFamilies[0].isClosed() == false db.isClosed() == true + + test "Test auto close tx enabled": + let + readOpts = defaultReadOptions(autoClose = true) + writeOpts = defaultWriteOptions(autoClose = true) + txOpts = defaultTransactionOptions(autoClose = true) + tx = db.beginTransaction(readOpts, writeOpts, txOpts) + + check: + readOpts.isClosed() == false + writeOpts.isClosed() == false + txOpts.isClosed() == false + tx.isClosed() == false + + tx.close() + + check: + readOpts.isClosed() == true + writeOpts.isClosed() == true + txOpts.isClosed() == true + tx.isClosed() == true + + test "Test auto close tx disabled": + let + readOpts = defaultReadOptions(autoClose = false) + writeOpts = defaultWriteOptions(autoClose = false) + txOpts = defaultTransactionOptions(autoClose = false) + tx = db.beginTransaction(readOpts, writeOpts, txOpts) + + check: + readOpts.isClosed() == false + writeOpts.isClosed() == false + txOpts.isClosed() == false + tx.isClosed() == false + + tx.close() + + check: + readOpts.isClosed() == false + writeOpts.isClosed() == false + txOpts.isClosed() == false + tx.isClosed() == true