diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fd366b4..8cfdbd48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ All changes in this project will be noted in this file. -## Version 0.8.0 +## Unreleased ### Additions @@ -19,6 +19,12 @@ All changes in this project will be noted in this file. - Similary, all `inspect` queries have been changed - Entities are now of the form `space.model` instead of `ks:tbl` +## Version 0.7.6 + +### Fixes + +- Fixed erroneous removal of `auth` system table during tree cleanup (see #276) + ## Version 0.7.5 ### Additions diff --git a/server/src/storage/v1/interface.rs b/server/src/storage/v1/interface.rs index 2eb65bbb..c3777482 100644 --- a/server/src/storage/v1/interface.rs +++ b/server/src/storage/v1/interface.rs @@ -26,6 +26,8 @@ //! Interfaces with the file system +use std::collections::HashMap; + use { crate::{ corestore::memstore::Memstore, @@ -86,57 +88,60 @@ pub fn create_tree_fresh(target: &T, memroot: &Memstore) -> Io /// throughout the lifecycle of the server pub fn cleanup_tree(memroot: &Memstore) -> IoResult<()> { if registry::get_cleanup_tripswitch().is_tripped() { + log::info!("We're cleaning up ..."); // only run a cleanup if someone tripped the switch // hashset because the fs itself will not allow duplicate entries // the keyspaces directory will contain the PRELOAD file, but we'll just // remove it from the list let mut dir_keyspaces: HashSet = read_dir_to_col!(DIR_KSROOT); dir_keyspaces.remove("PRELOAD"); - let our_keyspaces: HashSet = memroot + let our_keyspaces: HashMap> = memroot .keyspaces .iter() - .map(|kv| unsafe { kv.key().as_str() }.to_owned()) + .map(|kv| { + let ksid = unsafe { kv.key().as_str() }.to_owned(); + let tables: HashSet = kv + .value() + .tables + .iter() + .map(|tbl| unsafe { tbl.key().as_str() }.to_owned()) + .collect(); + (ksid, tables) + }) .collect(); + // these are the folders that we need to remove; plonk the deleted keyspaces first - for folder in dir_keyspaces.difference(&our_keyspaces) { + let keyspaces_to_remove: Vec<&String> = dir_keyspaces + .iter() + .filter(|ksname| !our_keyspaces.contains_key(ksname.as_str())) + .collect(); + for folder in keyspaces_to_remove { let ks_path = concat_str!(DIR_KSROOT, "/", folder); fs::remove_dir_all(ks_path)?; } - // now remove the tables - for keyspace in memroot.keyspaces.iter() { - let ks_path = unsafe { concat_str!(DIR_KSROOT, "/", keyspace.key().as_str()) }; + + // HACK(@ohsayan): Due to the nature of how system tables are stored in v1, we need to get rid of this + // ensuring that system tables don't end up being removed (since no system tables are actually + // purged at this time) + let mut our_keyspaces = our_keyspaces; + our_keyspaces.remove("system").unwrap(); + let our_keyspaces = our_keyspaces; + + // now remove the dropped tables + for (keyspace, tables) in our_keyspaces { + let ks_path = concat_str!(DIR_KSROOT, "/", keyspace.as_str()); + // read what is present in the tables directory let mut dir_tbls: HashSet = read_dir_to_col!(&ks_path); // in the list of directories we collected, remove PARTMAP because we should NOT // delete it dir_tbls.remove("PARTMAP"); - let our_tbls: HashSet = keyspace - .value() - .tables - .iter() - .map(|v| unsafe { v.key().as_str() }.to_owned()) - .collect(); - for old_file in dir_tbls.difference(&our_tbls) { - let fpath = concat_path!(&ks_path, old_file); + // find what tables we should remove + let tables_to_remove = dir_tbls.difference(&tables); + for removed_table in tables_to_remove { + let fpath = concat_path!(&ks_path, removed_table); fs::remove_file(&fpath)?; } } - // now plonk the data files - for keyspace in memroot.keyspaces.iter() { - let ks_path = unsafe { concat_str!(DIR_KSROOT, "/", keyspace.key().as_str()) }; - let dir_tbls: HashSet = read_dir_to_col!(&ks_path); - let our_tbls: HashSet = keyspace - .value() - .tables - .iter() - .map(|v| unsafe { v.key().as_str() }.to_owned()) - .collect(); - for old_file in dir_tbls.difference(&our_tbls) { - if old_file != "PARTMAP" { - // plonk this data file; we don't need it anymore - fs::remove_file(concat_path!(&ks_path, old_file))?; - } - } - } } Ok(()) } diff --git a/server/src/tests/issue_tests.rs b/server/src/tests/issue_tests.rs new file mode 100644 index 00000000..71d7d49a --- /dev/null +++ b/server/src/tests/issue_tests.rs @@ -0,0 +1,52 @@ +/* + * Created on Fri Aug 12 2022 + * + * This file is a part of Skytable + * Skytable (formerly known as TerrabaseDB or Skybase) is a free and open-source + * NoSQL database written by Sayan Nandan ("the Author") with the + * vision to provide flexibility in data modelling without compromising + * on performance, queryability or scalability. + * + * Copyright (c) 2022, Sayan Nandan + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * +*/ + +mod issue_276 { + // Refer to the issue here: https://github.com/skytable/skytable/issues/276 + // Gist of issue: the auth table was cleaned up because the dummy ks in Memstore does not have + // the system tables + use skytable::{Element, Query, RespCode}; + #[sky_macros::dbtest_func(port = 2005, auth_testuser = true, skip_if_cfg = "persist-suite")] + async fn first_run() { + // create the space + let r: Element = con + .run_query(Query::from("create space please_do_not_vanish")) + .await + .unwrap(); + assert_eq!(r, Element::RespCode(RespCode::Okay)); + // drop the space + let r: Element = con + .run_query(Query::from("drop space please_do_not_vanish")) + .await + .unwrap(); + assert_eq!(r, Element::RespCode(RespCode::Okay)); + } + #[sky_macros::dbtest_func(port = 2005, auth_testuser = true, run_if_cfg = "persist-suite")] + async fn second_run() { + // this function is just a dummy fn; if, as described in #276, the auth data is indeed + // lost, then the server will simply fail to start up + } +} diff --git a/server/src/tests/mod.rs b/server/src/tests/mod.rs index d91d53f8..b40608df 100644 --- a/server/src/tests/mod.rs +++ b/server/src/tests/mod.rs @@ -38,6 +38,7 @@ mod kvengine_list; mod persist; mod pipeline; mod snapshot; +mod issue_tests; mod tls { use skytable::{query, Element};