Merge pull request #1354 from spacejam/tyler_copy_error

Make Error Copy

Author: spacejam

.github/workflows/test.yml

@@ -98,11 +98,12 @@ jobs:
     - name: Cache target
       uses: actions/cache@v2
       env:
-        cache-name: cache-stress2-target-and-lockfile
+        cache-name: cache-stress2-asan-target-and-lockfile
       with:
         path: |
           benchmarks/stress2/target
           benchmarks/stress2/Cargo.lock
+          ~/.rustup
         key: ${{ runner.os }}-${{ env.cache-name }}-${{ hashFiles('**/Cargo.toml') }}
     - name: burn in
       run: |
@@ -111,8 +112,14 @@ jobs:
         ulimit -c unlimited
         echo "$PWD/core-dumps/corefile-%e-%p-%t" | sudo tee /proc/sys/kernel/core_pattern
         mkdir core-dumps
+        rustup toolchain install nightly
+        rustup toolchain install nightly --component rust-src
+        rustup update
         rm -rf default.sled || true
-        cargo run --release -- --duration=60
+        export RUSTFLAGS="-Z sanitizer=address"
+        export ASAN_OPTIONS="detect_odr_violation=0"
+        cargo +nightly build --release --target x86_64-unknown-linux-gnu
+        target/x86_64-unknown-linux-gnu/release/stress2 --duration=240
         rm -rf default.sled
     - name: print backtraces with gdb
       if: ${{ failure() }}

CHANGELOG.md

@@ -23,7 +23,6 @@
 
 ## Breaking Changes
 
-* #1349 The "measure_allocs" feature has been removed.
 * #1135 The "no_metrics" anti-feature has been replaced with
   the "metrics" positive feature.
 * #1178 the `Event` enum has become a unified struct that allows
@@ -52,6 +51,9 @@
 * #1314 `Subscriber::next_timeout` now requires a mutable self
   reference.
 * #1337 Bump MSRV to 1.48.
+* #1349 The "measure_allocs" feature has been removed.
+* #1354 `Error` has been modified to be Copy, removing all
+  heap-allocated variants.
 
 
 ## Bug Fixes

src/config.rs

@@ -68,8 +68,7 @@ impl StorageParameters {
                 return Err(Error::Unsupported(
                     "failed to open database that may \
                      have been created using a sled version \
-                     earlier than 0.29"
-                        .to_string(),
+                     earlier than 0.29",
                 ));
             };
             let mut split = line.split(": ").map(String::from);
@@ -282,7 +281,7 @@ impl Inner {
 macro_rules! supported {
     ($cond:expr, $msg:expr) => {
         if !$cond {
-            return Err(Error::Unsupported($msg.to_owned()));
+            return Err(Error::Unsupported($msg));
         }
     };
 }
@@ -550,15 +549,11 @@ impl Config {
                     file.try_lock_exclusive()
                 };
 
-            if let Err(e) = try_lock {
-                return Err(Error::Io(io::Error::new(
+            if try_lock.is_err() {
+                return Err(Error::Io(
                     ErrorKind::Other,
-                    format!(
-                        "could not acquire lock on {:?}: {:?}",
-                        self.db_path().to_string_lossy(),
-                        e
-                    ),
-                )));
+                    "could not acquire database file lock",
+                ));
             }
         }
 
@@ -568,28 +563,27 @@ impl Config {
     fn verify_config(&self) -> Result<()> {
         match self.read_config() {
             Ok(Some(old)) => {
-                supported!(
-                    self.use_compression == old.use_compression,
-                    format!(
-                        "cannot change compression values across restarts. \
-                         old value of use_compression loaded from disk: {}, \
-                         currently set value: {}.",
-                        old.use_compression, self.use_compression,
-                    )
-                );
+                if self.use_compression {
+                    supported!(
+                        old.use_compression,
+                        "cannot change compression configuration across restarts. \
+                        this database was created without compression enabled."
+                    );
+                } else {
+                    supported!(
+                        !old.use_compression,
+                        "cannot change compression configuration across restarts. \
+                        this database was created with compression enabled."
+                    );
+                }
 
                 supported!(
                     self.segment_size == old.segment_size,
-                    format!(
-                        "cannot change the io buffer size across restarts. \
-                         please change it back to {}",
-                        old.segment_size
-                    )
+                    "cannot change the io buffer size across restarts."
                 );
 
-                supported!(
-                    self.version == old.version,
-                    format!(
+                if self.version != old.version {
+                    error!(
                         "This database was created using \
                          pagecache version {}.{}, but our pagecache \
                          version is {}.{}. Please perform an upgrade \
@@ -599,8 +593,13 @@ impl Config {
                         old.version.1,
                         self.version.0,
                         self.version.1,
-                    )
-                );
+                    );
+                    supported!(
+                        self.version == old.version,
+                        "The stored database must use a compatible sled version.
+                        See error log for more details."
+                    );
+                }
                 Ok(())
             }
             Ok(None) => self.write_config(),
@@ -697,7 +696,7 @@ impl Config {
         } else {
             #[allow(unsafe_code)]
             unsafe {
-                Err(ge.deref().clone())
+                Err(*ge.deref())
             }
         }
     }

src/db.rs

@@ -146,9 +146,7 @@ impl Db {
     pub fn drop_tree<V: AsRef<[u8]>>(&self, name: V) -> Result<bool> {
         let name_ref = name.as_ref();
         if name_ref == DEFAULT_TREE_ID {
-            return Err(Error::Unsupported(
-                "cannot remove the core structures".into(),
-            ));
+            return Err(Error::Unsupported("cannot remove the default tree"));
         }
         trace!("dropping tree {:?}", name_ref,);
 

src/lib.rs

@@ -162,6 +162,7 @@
         // clippy::wildcard_enum_match_arm,
     )
 )]
+#![allow(clippy::comparison_chain)]
 
 macro_rules! io_fail {
     ($config:expr, $e:expr) => {

src/meta.rs

@@ -54,7 +54,7 @@ where
                     merge_operator: RwLock::new(None),
                 })));
             }
-            Err(Error::CollectionNotFound(_)) => {}
+            Err(Error::CollectionNotFound) => {}
             Err(other) => return Err(other),
         }
 

src/pagecache/iobuf.rs

@@ -1200,7 +1200,7 @@ pub(in crate::pagecache) fn maybe_seal_and_write_iobuf(
         match iobufs.with_sa(|sa| sa.next(next_lsn)) {
             Ok(ret) => ret,
             Err(e) => {
-                iobufs.set_global_error(e.clone());
+                iobufs.set_global_error(e);
                 return Err(e);
             }
         }

src/pagecache/iterator.rs

@@ -447,7 +447,7 @@ fn check_contiguity_in_unstable_tail(
     };
 
     // run the iterator to completion
-    while let Some(_) = iter.next() {}
+    for _ in &mut iter {}
 
     // `cur_lsn` is set to the beginning
     // of the next message

src/pagecache/logger.rs

@@ -118,7 +118,7 @@ impl Log {
         );
 
         if let Err(e) = &ret {
-            self.iobufs.set_global_error(e.clone());
+            self.iobufs.set_global_error(*e);
         }
 
         ret
@@ -169,7 +169,7 @@ impl Log {
         let ret = self.reserve_inner(log_kind, pid, item, None, guard);
 
         if let Err(e) = &ret {
-            self.iobufs.set_global_error(e.clone());
+            self.iobufs.set_global_error(*e);
         }
 
         ret

src/pagecache/mod.rs

@@ -840,7 +840,7 @@ impl PageCache {
             // changing.
             let ts = old.ts() + 1;
 
-            let cache_info = CacheInfo { lsn, pointer, ts };
+            let cache_info = CacheInfo { ts, lsn, pointer };
 
             let mut new_cache_infos =
                 Vec::with_capacity(old.cache_infos.len() + 1);
@@ -944,13 +944,13 @@ impl PageCache {
             'inner: loop {
                 let pg_view = self.inner.get(pid, &guard);
 
-                match &*pg_view.cache_infos {
-                    &[_single_cache_info] => {
+                match *pg_view.cache_infos {
+                    [_single_cache_info] => {
                         let page_state = pg_view.to_page_state();
                         page_states.push(page_state);
                         break 'inner;
                     }
-                    &[_first_of_several, ..] => {
+                    [_first_of_several, ..] => {
                         // If a page has multiple disk locations,
                         // rewrite it to a single one before storing
                         // a single 8-byte pointer to its cold location.
@@ -965,7 +965,7 @@ impl PageCache {
                         }
                         continue 'inner;
                     }
-                    &[] => {
+                    [] => {
                         // there is a benign race with the thread
                         // that is allocating this page. the allocating
                         // thread has not yet written the new page to disk,
@@ -1661,8 +1661,7 @@ impl PageCacheInner {
                 "{:?}",
                 Error::ReportableBug(
                     "failed to retrieve META page \
-                 which should always be present"
-                        .into(),
+                     which should always be present"
                 )
             )
         }
@@ -1684,8 +1683,7 @@ impl PageCacheInner {
                 "{:?}",
                 Error::ReportableBug(
                     "failed to retrieve counter page \
-                 which should always be present"
-                        .into(),
+                     which should always be present"
                 )
             )
         }
@@ -1904,7 +1902,7 @@ impl PageCacheInner {
         if let Some(root) = m.get_root(name) {
             Ok(root)
         } else {
-            Err(Error::CollectionNotFound(name.into()))
+            Err(Error::CollectionNotFound)
         }
     }
 
@@ -1948,8 +1946,7 @@ impl PageCacheInner {
                 Err(None) => {
                     return Err(Error::ReportableBug(
                         "replacing the META page has failed because \
-                         the pagecache does not think it currently exists."
-                            .into(),
+                         the pagecache does not think it currently exists.",
                     ));
                 }
             }
@@ -2088,10 +2085,10 @@ impl PageCacheInner {
 
         // TODO this feels racy, test it better?
         if let Update::Free = update {
-            Err(Error::ReportableBug(format!(
-                "non-link/replace found in pull of pid {}",
-                pid
-            )))
+            error!("non-link/replace found in pull of pid {}", pid);
+            Err(Error::ReportableBug(
+                "non-link/replace found in pull of page fragments",
+            ))
         } else {
             Ok(update)
         }