From cac474e0c93125056cf1d2cd9d76f1d5b5902ed4 Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Mon, 20 Nov 2023 00:08:33 -0600 Subject: [PATCH 1/7] Allow Blockstore to auto-detect and open unknown columns --- ledger/src/blockstore_db.rs | 47 +++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/ledger/src/blockstore_db.rs b/ledger/src/blockstore_db.rs index 87b26ce4d5b1a9..1f5108ed080036 100644 --- a/ledger/src/blockstore_db.rs +++ b/ledger/src/blockstore_db.rs @@ -35,7 +35,7 @@ use { }, solana_storage_proto::convert::generated, std::{ - collections::HashMap, + collections::{HashMap, HashSet}, ffi::{CStr, CString}, fs, marker::PhantomData, @@ -419,7 +419,12 @@ impl Rocks { } let oldest_slot = OldestSlot::default(); let column_options = options.column_options.clone(); - let cf_descriptors = Self::cf_descriptors(&options, &oldest_slot); + let detected_cfs = DB::list_cf(&Options::default(), path) + .map_err(|err| { + warn!("Unable to detect Rocks colums: {err:?}"); + }) + .unwrap_or_default(); + let cf_descriptors = Self::cf_descriptors(&options, &oldest_slot, &detected_cfs); // Open the database let db = match access_type { @@ -454,15 +459,25 @@ impl Rocks { Ok(rocks) } + /// Create the column family (CF) descriptors necessary to open the database. + /// + /// In order to open a RocksDB database with Primary access, all columns must be opened. So, + /// in addition to creating descriptors for all of the expected columns, also create + /// descriptors for columns that were discovered but are otherwise unknown to the software. + /// + /// One case where columns could be unknown is if a RocksDB database is modified with a newer + /// software version that adds a new column, and then also opened with an older version that + /// did not have knowledge of that new column. fn cf_descriptors( options: &BlockstoreOptions, oldest_slot: &OldestSlot, + detected_cfs: &[String], ) -> Vec { use columns::*; let (cf_descriptor_shred_data, cf_descriptor_shred_code) = new_cf_descriptor_pair_shreds::(options, oldest_slot); - vec![ + let mut cf_descriptors = vec![ new_cf_descriptor::(options, oldest_slot), new_cf_descriptor::(options, oldest_slot), new_cf_descriptor::(options, oldest_slot), @@ -484,7 +499,29 @@ impl Rocks { new_cf_descriptor::(options, oldest_slot), new_cf_descriptor::(options, oldest_slot), new_cf_descriptor::(options, oldest_slot), - ] + ]; + + let known_cfs: HashSet<_> = cf_descriptors + .iter() + .map(|cf_descriptor| cf_descriptor.name().to_string()) + .collect(); + detected_cfs.iter().for_each(|cf_name| { + if known_cfs.get(cf_name.as_str()).is_none() { + info!("Detected unknown column {cf_name}, opening column with basic options"); + // This version of the software was unaware of the column, so + // it fair to assume that we will not attempt to read or write + // the column. So, set some bare bones settings to avoid + // using extra resources on this unknown column. + let mut options = Options::default(); + // Lower the default to avoid unnecessary allocations + options.set_write_buffer_size(1024 * 1024); + // Disable compactions to avoid any modifications to the column + options.set_disable_auto_compactions(true); + cf_descriptors.push(ColumnFamilyDescriptor::new(cf_name, options)); + } + }); + + cf_descriptors } fn columns() -> Vec<&'static str> { @@ -2224,7 +2261,7 @@ pub mod tests { // should update both lists. assert_eq!( Rocks::columns().len(), - Rocks::cf_descriptors(&options, &oldest_slot).len() + Rocks::cf_descriptors(&options, &oldest_slot, &[]).len() ); } From a88e1656f8756703ddd10ba7019325625ef53fa1 Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Mon, 20 Nov 2023 01:33:59 -0600 Subject: [PATCH 2/7] Add unit test --- ledger/src/blockstore_db.rs | 45 ++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/ledger/src/blockstore_db.rs b/ledger/src/blockstore_db.rs index 1f5108ed080036..67510f77198749 100644 --- a/ledger/src/blockstore_db.rs +++ b/ledger/src/blockstore_db.rs @@ -2201,7 +2201,7 @@ fn should_enable_compression() -> bool { #[cfg(test)] pub mod tests { - use {super::*, crate::blockstore_db::columns::ShredData}; + use {super::*, crate::blockstore_db::columns::ShredData, tempfile::tempdir}; #[test] fn test_compaction_filter() { @@ -2286,6 +2286,49 @@ pub mod tests { assert!(!should_enable_cf_compaction("something else")); } + #[test] + fn test_open_unknown_columns() { + solana_logger::setup(); + + let temp_dir = tempdir().unwrap(); + let db_path = temp_dir.path(); + + // Open with Primary to create the new database + { + let options = BlockstoreOptions { + access_type: AccessType::Primary, + enforce_ulimit_nofile: false, + ..BlockstoreOptions::default() + }; + let mut rocks = Rocks::open(db_path, options).unwrap(); + + // Introduce a new column that will not be known + rocks + .db + .create_cf("new_column", &Options::default()) + .unwrap(); + } + + // Opening with either Secondary or Primary access should succeed, + // even though the Rocks code is unaware of "new_column" + { + let options = BlockstoreOptions { + access_type: AccessType::Secondary, + enforce_ulimit_nofile: false, + ..BlockstoreOptions::default() + }; + let _ = Rocks::open(db_path, options).unwrap(); + } + { + let options = BlockstoreOptions { + access_type: AccessType::Primary, + enforce_ulimit_nofile: false, + ..BlockstoreOptions::default() + }; + let _ = Rocks::open(db_path, options).unwrap(); + } + } + impl LedgerColumn where C: ColumnIndexDeprecation + ProtobufColumn + ColumnName, From da319ec1290ca2924dd187e06fb26755abb876de Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Mon, 20 Nov 2023 01:38:08 -0600 Subject: [PATCH 3/7] Avoid creating descriptor for default column --- ledger/src/blockstore_db.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ledger/src/blockstore_db.rs b/ledger/src/blockstore_db.rs index 67510f77198749..cd921264ca4e86 100644 --- a/ledger/src/blockstore_db.rs +++ b/ledger/src/blockstore_db.rs @@ -501,9 +501,13 @@ impl Rocks { new_cf_descriptor::(options, oldest_slot), ]; + // The default column is handled automatically, we don't need to create + // a descriptor for it + const DEFAULT_COLUMN_NAME: &str = "default"; let known_cfs: HashSet<_> = cf_descriptors .iter() .map(|cf_descriptor| cf_descriptor.name().to_string()) + .chain(std::iter::once(DEFAULT_COLUMN_NAME.to_string())) .collect(); detected_cfs.iter().for_each(|cf_name| { if known_cfs.get(cf_name.as_str()).is_none() { From 3dd1aa3a1d40006740a4f0866010a0e99011a309 Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Mon, 27 Nov 2023 14:00:17 -0600 Subject: [PATCH 4/7] Typo fixes --- ledger/src/blockstore_db.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ledger/src/blockstore_db.rs b/ledger/src/blockstore_db.rs index cd921264ca4e86..812c968175e0fe 100644 --- a/ledger/src/blockstore_db.rs +++ b/ledger/src/blockstore_db.rs @@ -421,7 +421,7 @@ impl Rocks { let column_options = options.column_options.clone(); let detected_cfs = DB::list_cf(&Options::default(), path) .map_err(|err| { - warn!("Unable to detect Rocks colums: {err:?}"); + warn!("Unable to detect Rocks columns: {err:?}"); }) .unwrap_or_default(); let cf_descriptors = Self::cf_descriptors(&options, &oldest_slot, &detected_cfs); @@ -513,8 +513,8 @@ impl Rocks { if known_cfs.get(cf_name.as_str()).is_none() { info!("Detected unknown column {cf_name}, opening column with basic options"); // This version of the software was unaware of the column, so - // it fair to assume that we will not attempt to read or write - // the column. So, set some bare bones settings to avoid + // it is fair to assume that we will not attempt to read or + // write the column. So, set some bare bones settings to avoid // using extra resources on this unknown column. let mut options = Options::default(); // Lower the default to avoid unnecessary allocations From 5dd20cd3a986a458c6ce50b4eeac8c2502a8d738 Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Mon, 27 Nov 2023 15:11:18 -0600 Subject: [PATCH 5/7] Only detect missing columns when access type is Primary We can open a rocksdb instance with a subset of the columns when using Secondary access. --- ledger/src/blockstore_db.rs | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/ledger/src/blockstore_db.rs b/ledger/src/blockstore_db.rs index 812c968175e0fe..c68a42698e86b0 100644 --- a/ledger/src/blockstore_db.rs +++ b/ledger/src/blockstore_db.rs @@ -419,12 +419,7 @@ impl Rocks { } let oldest_slot = OldestSlot::default(); let column_options = options.column_options.clone(); - let detected_cfs = DB::list_cf(&Options::default(), path) - .map_err(|err| { - warn!("Unable to detect Rocks columns: {err:?}"); - }) - .unwrap_or_default(); - let cf_descriptors = Self::cf_descriptors(&options, &oldest_slot, &detected_cfs); + let cf_descriptors = Self::cf_descriptors(path, &options, &oldest_slot)?; // Open the database let db = match access_type { @@ -469,10 +464,10 @@ impl Rocks { /// software version that adds a new column, and then also opened with an older version that /// did not have knowledge of that new column. fn cf_descriptors( + path: &Path, options: &BlockstoreOptions, oldest_slot: &OldestSlot, - detected_cfs: &[String], - ) -> Vec { + ) -> Result> { use columns::*; let (cf_descriptor_shred_data, cf_descriptor_shred_code) = @@ -501,6 +496,20 @@ impl Rocks { new_cf_descriptor::(options, oldest_slot), ]; + // If the access type is Secondary, we don't need to open all of the + // columns so we can just return immediately. + match options.access_type { + AccessType::Secondary => { + return Ok(cf_descriptors); + } + AccessType::Primary | AccessType::PrimaryForMaintenance => {} + } + + let detected_cfs = DB::list_cf(&Options::default(), path) + .map_err(|err| { + warn!("Unable to detect Rocks columns: {err:?}"); + }) + .unwrap_or_default(); // The default column is handled automatically, we don't need to create // a descriptor for it const DEFAULT_COLUMN_NAME: &str = "default"; @@ -525,7 +534,7 @@ impl Rocks { } }); - cf_descriptors + Ok(cf_descriptors) } fn columns() -> Vec<&'static str> { @@ -2205,7 +2214,9 @@ fn should_enable_compression() -> bool { #[cfg(test)] pub mod tests { - use {super::*, crate::blockstore_db::columns::ShredData, tempfile::tempdir}; + use { + super::*, crate::blockstore_db::columns::ShredData, std::path::PathBuf, tempfile::tempdir, + }; #[test] fn test_compaction_filter() { @@ -2258,6 +2269,7 @@ pub mod tests { #[test] fn test_cf_names_and_descriptors_equal_length() { + let path = PathBuf::default(); let options = BlockstoreOptions::default(); let oldest_slot = OldestSlot::default(); // The names and descriptors don't need to be in the same order for our use cases; @@ -2265,7 +2277,9 @@ pub mod tests { // should update both lists. assert_eq!( Rocks::columns().len(), - Rocks::cf_descriptors(&options, &oldest_slot, &[]).len() + Rocks::cf_descriptors(&path, &options, &oldest_slot) + .unwrap() + .len() ); } From 022d49710be1f37192cf1889e435dc3650c1f79d Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Mon, 27 Nov 2023 20:57:09 -0600 Subject: [PATCH 6/7] Remove superfluous Result<_> --- ledger/src/blockstore_db.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/ledger/src/blockstore_db.rs b/ledger/src/blockstore_db.rs index c68a42698e86b0..0ed9a19a0f5aaa 100644 --- a/ledger/src/blockstore_db.rs +++ b/ledger/src/blockstore_db.rs @@ -419,7 +419,7 @@ impl Rocks { } let oldest_slot = OldestSlot::default(); let column_options = options.column_options.clone(); - let cf_descriptors = Self::cf_descriptors(path, &options, &oldest_slot)?; + let cf_descriptors = Self::cf_descriptors(path, &options, &oldest_slot); // Open the database let db = match access_type { @@ -467,7 +467,7 @@ impl Rocks { path: &Path, options: &BlockstoreOptions, oldest_slot: &OldestSlot, - ) -> Result> { + ) -> Vec { use columns::*; let (cf_descriptor_shred_data, cf_descriptor_shred_code) = @@ -500,7 +500,7 @@ impl Rocks { // columns so we can just return immediately. match options.access_type { AccessType::Secondary => { - return Ok(cf_descriptors); + return cf_descriptors; } AccessType::Primary | AccessType::PrimaryForMaintenance => {} } @@ -534,7 +534,7 @@ impl Rocks { } }); - Ok(cf_descriptors) + cf_descriptors } fn columns() -> Vec<&'static str> { @@ -2277,9 +2277,7 @@ pub mod tests { // should update both lists. assert_eq!( Rocks::columns().len(), - Rocks::cf_descriptors(&path, &options, &oldest_slot) - .unwrap() - .len() + Rocks::cf_descriptors(&path, &options, &oldest_slot).len() ); } From b5e379744fd50075c605f5426fc50e216bfd179a Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Mon, 27 Nov 2023 21:02:52 -0600 Subject: [PATCH 7/7] Replace .unwrap_or_default() with explicit match statement --- ledger/src/blockstore_db.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/ledger/src/blockstore_db.rs b/ledger/src/blockstore_db.rs index 0ed9a19a0f5aaa..18ba491ea34bd1 100644 --- a/ledger/src/blockstore_db.rs +++ b/ledger/src/blockstore_db.rs @@ -505,11 +505,16 @@ impl Rocks { AccessType::Primary | AccessType::PrimaryForMaintenance => {} } - let detected_cfs = DB::list_cf(&Options::default(), path) - .map_err(|err| { + // Attempt to detect the column families that are present. It is not a + // fatal error if we cannot, for example, if the Blockstore is brand + // new and will be created by the call to Rocks::open(). + let detected_cfs = match DB::list_cf(&Options::default(), path) { + Ok(detected_cfs) => detected_cfs, + Err(err) => { warn!("Unable to detect Rocks columns: {err:?}"); - }) - .unwrap_or_default(); + vec![] + } + }; // The default column is handled automatically, we don't need to create // a descriptor for it const DEFAULT_COLUMN_NAME: &str = "default";