improve error handling for DROP TABLE

project-tsurugi/tsurugi-issues#1030

src/jogasaki/api/impl/database.cpp

@@ -980,10 +980,21 @@ status database::initialize_from_providers() {
         }
         try {
             sequence_manager_->load_id_map(tx.get());
-            sequence_manager_->register_sequences(tx.get(), tables_);
+            sequence_manager_->register_sequences(tx.get(), tables_, false);
         } catch(executor::sequence::exception& e) {
-            LOG_LP(ERROR) << "registering sequences failed:" << e.get_status() << " " << e.what();
-            return e.get_status();
+            if(e.get_status() != status::err_not_found) {
+                LOG_LP(ERROR) << "registering sequences failed:" << e.get_status() << " " << e.what();
+                return e.get_status();
+            }
+            // missing sequence entry in __sequences table
+            // The situation possibly occurs by aborting transaction used for CREATE TABLE.
+            // Dropping the table and recreating it will fix the issue.
+            // We do not raise error in the start-up here. Allow users to read/dump the data for backup or
+            // to drop the table to fix the situation.
+            LOG_LP(WARNING) << "sequence '" << e.what()
+                            << "' not found on the system table. Possibly the table definition did not "
+                               "complete successfully. Inserting new records using the sequence is likely to hit an "
+                               "error. Re-creating the table that owns the sequence may fix the issue";
         }
         if(auto res = tx->commit(); res != status::ok) {
             LOG_LP(ERROR) << "committing table schema entries failed";

src/jogasaki/executor/common/create_table.cpp

@@ -96,7 +96,8 @@ bool create_generated_sequence(
             p.min_value(),
             p.max_value(),
             p.cycle(),
-            true
+            true,
+            true // create new seq_id
         );
 //        provider.add_sequence(p);  // sequence definition is added in serializer, no need to add it here
     } catch (sequence::exception& e) {

src/jogasaki/executor/common/drop_table.cpp

@@ -76,13 +76,18 @@ bool remove_generated_sequences(
             auto def_id = s->definition_id().value();
             try {
                 if(! context.sequence_manager()->remove_sequence(def_id, context.transaction()->object().get())) {
-                    VLOG_LP(log_error) << "sequence '" << sequence_name << "' not found";
-                    context.status_code(status::err_not_found);
-                    return false;
+                    // even on error, continue clean-up as much as possible
+                    VLOG_LP(log_info) << "sequence '" << sequence_name << "' not found";
                 }
             } catch(executor::sequence::exception const& e) {
-                VLOG_LP(log_error) << "removing sequence '" << sequence_name << "' not found";
-                context.status_code(e.get_status(), e.what());
+                // unrecoverable error - transaction aborted and we cannot continue
+                VLOG_LP(log_error) << "removing sequence '" << sequence_name << "' failed";
+                set_error(
+                    context,
+                    error_code::sql_execution_exception,
+                    e.what(),
+                    e.get_status()
+                );
                 return false;
             }
         }
@@ -131,6 +136,10 @@ bool drop_table::operator()(request_context& context) const {
         return false;
     }
 
+    // note on error handling: now regular check for existence of table has passed. Going forward, if part of the table
+    // dependencies (e.g. secondary indices or sequence entry on system table) are missing, drop table should clean-up
+    // those dependencies as much as possible in order to avoid left garbage becoming a road block for other operations.
+
     std::vector<std::string> generated_sequences{};
     // drop auto-generated sequences first because accessing system table causes cc error
     if(! drop_auto_generated_sequences(context, *t, generated_sequences)) {

src/jogasaki/executor/sequence/manager.cpp

@@ -96,10 +96,14 @@ sequence* manager::register_sequence(
     sequence_value minimum_value,
     sequence_value maximum_value,
     bool enable_cycle,
-    bool save_id_map_entry
+    bool save_id_map_entry,
+    bool assign_new_seq_id_if_not_found
 ) {
     sequence_id seq_id{};
     if(sequences_.count(def_id) == 0) {
+        if(! assign_new_seq_id_if_not_found) {
+            return {};
+        }
         if(auto rc = db_->create_sequence(seq_id); rc != status::ok) {
             (void) tx->abort();
             throw_exception(exception{rc});
@@ -142,15 +146,16 @@ sequence* manager::register_sequence(
 
 void manager::register_sequences(
     kvs::transaction* tx,
-    maybe_shared_ptr<yugawara::storage::configurable_provider> const& provider
+    maybe_shared_ptr<yugawara::storage::configurable_provider> const& provider,
+    bool assign_new_seq_id_if_not_found
 ) {
     provider->each_sequence(
-        [this, &tx](
+        [this, &tx, assign_new_seq_id_if_not_found](
             std::string_view,
             std::shared_ptr<yugawara::storage::sequence const> const& entry
         ) {
             auto def_id = *entry->definition_id();
-            register_sequence(
+            if(auto p = register_sequence(
                 tx,
                 def_id,
                 entry->simple_name(),
@@ -159,8 +164,11 @@ void manager::register_sequences(
                 entry->min_value(),
                 entry->max_value(),
                 entry->cycle(),
-                false
-            );
+                false,
+                assign_new_seq_id_if_not_found
+            ); p == nullptr) {
+                throw_exception(exception{status::err_not_found, entry->simple_name()});
+            }
         }
     );
     save_id_map(tx);
@@ -200,7 +208,7 @@ bool manager::remove_sequence(
     if (sequences_.count(def_id) == 0) {
         return false;
     }
-    if (auto rc = db_->delete_sequence(sequences_[def_id].id()); rc != status::ok) {
+    if (auto rc = db_->delete_sequence(sequences_[def_id].id()); rc != status::ok && rc != status::err_not_found) {
         // status::err_not_found never comes here because the sequence is already in sequences_
         (void) tx->abort();
         throw_exception(exception{rc});

src/jogasaki/executor/sequence/manager.h

@@ -120,7 +120,8 @@ class manager {
     /**
      * @brief register the sequence properties for the definition id
      * @details using the id_map currently held, create the in-memory sequence object with the given spec.
-     * If the id_map doesn't has the given def_id, ask kvs to assign new sequence id. Optionally save the id map.
+     * If the id_map doesn't has the given def_id, ask kvs to assign new sequence id
+     * (or exit if `assign_new_seq_id_if_not_found=false`). Optionally save the id map.
      * This function is not thread-safe. Only a thread can call this member function at a time.
      * @param def_id the key to uniquely identify the sequence
      * @param name the name of the sequence
@@ -132,6 +133,8 @@ class manager {
      * min or max value (corresponding to the boundary that is went over.)
      * @param save_id_map_entry indicates whether the id_map entry for registered sequence should be saved now.
      * Set false if you are registering multiples sequences and they should be saved later.
+     * @param assign_new_seq_id_if_not_found if false, the function returns nullptr when the def_id is not found
+     * in the id_map. If true, the function creates new sequence id for such def_id.
      * @return the in-memory sequence object just registered
      * @throws sequence::exception if any error occurs, then passed transaction is aborted.
      */
@@ -144,19 +147,26 @@ class manager {
         sequence_value minimum_value = 0,
         sequence_value maximum_value = std::numeric_limits<sequence_value>::max(),
         bool enable_cycle = true,
-        bool save_id_map_entry = true
+        bool save_id_map_entry = true,
+        bool assign_new_seq_id_if_not_found = true
     );
 
     /**
      * @brief bulk registering sequences
      * @details this function retrieves sequence definitions from provider and register one by one.
      * This function is not thread-safe. Only a thread can call this member function at a time.
      * @param provider the config. provider that gives sequences definitions.
-     * @throws sequence::exception if any error occurs, then passed transaction is aborted.
+     * @param assign_new_seq_id_if_not_found if true, the function creates new sequence id for the def_id that
+     * is not on the id_map. If false, exception below is thrown for such def_id.
+     * @throws sequence::exception with status::err_not_found and what() containing sequence name if
+     * `assign_new_seq_id_if_not_found` is false and def_id is
+     * not found on id_map. The passed transaction is not touched.
+     * @throws sequence::exception if any other error occurs, then passed transaction is aborted.
      */
     void register_sequences(
         kvs::transaction* tx,
-        maybe_shared_ptr<yugawara::storage::configurable_provider> const& provider
+        maybe_shared_ptr<yugawara::storage::configurable_provider> const& provider,
+        bool assign_new_seq_id_if_not_found
     );
 
     /**

src/jogasaki/executor/wrt/fill_record_fields.cpp

@@ -69,7 +69,15 @@ status next_sequence_value(request_context& ctx, sequence_definition_id def_id,
     auto& mgr = *ctx.sequence_manager();
     auto* seq = mgr.find_sequence(def_id);
     if(seq == nullptr) {
-        throw_exception(std::logic_error{""});
+        auto rc = status::err_unknown;
+        auto msg = string_builder{} << "sequence not found def_id:" << def_id
+                                    << " possibly due to DDL failure. Recreating "
+                                       "the table may fix the situation"
+                                    << string_builder::to_string;
+        set_error(ctx, error_code::sql_execution_exception, msg, rc);
+        // system level errornous situation - table can be read, but write is not reliable until recration
+        LOG_LP(ERROR) << msg;
+        return rc;
     }
     auto ret = seq->next(*ctx.transaction()->object());
     // even if there is an error with next(), it mark the sequence as used by the tx, so call notify_updates first