WL#7771: Make sure errors are properly handled in DD API
Affects: Server-8.0 — Status: Complete
DD API implementation is being developed gradually. There are many transitional changes, it is not reasonable to take care of all possible errors for them, as they will be changed eventually anyway. This WL is to finalize error handling once the implementation is more or less stabilized. All the DD API users needs this. Stabilizing error handling avoids many hard to debug error scenarios. User Documentation ================== No user-visible changes. No user documentation required.
Functional requirement: * Make sure DD methods do not ignore error reported by Raw_* classes. * Make DD API signature changes to return 'true' if there is some unexpected error, and 'false' if operation succeeds. * Make sure error is propagated up, when there is a error reading/writing dd::Properties key-value pair. * Try to handle kill flag during DD operations. We need Identify scenarios when a DD operation should not be allowed if kill flag is set. There are some DD operations that might still required to be executed even after kill flag is set, we need to take appropriate actions. * TODO Write error message to server log. We should use different error code/message when we get error from DD API framework, something like, => "Data-dictionary error: '%original error message here'". Non functional requirement: * Add doxygen comments for functions that return unexpected errors. * Use DBUG_ENTER() and DBUG_RETURN() for important functions.
The problem is that some of the DD API's and DD API internal methods ignore the SE errors. This is mainly due to past assumption that where made, that the operations on DD API would not fail. The action required to fix the problem is to visit most of DD API methods, study the return code path and make sure we do not ignore any error raised from SE. There are following cases to handle, C1) DD method ignores error reported by Raw_* classes. The signature of DD methods needs to be changed. E.g:, Raw_record::find_record Raw_record::find_last_record Raw_record::prepare_record_for_update Raw_record::open_record_set Raw_record_set::next Dictionary_impl::get_object_by_id Dictionary_impl::get_object_by_key Dictionary_impl::restore_object_from_record Dictionary_object_table::restore_object_from_record Transaction::get_object Transaction::get_charset Transaction::get_collation Transaction::get_schema Transaction::get_tablespace Transaction::fetch_table_by_se_private_key Transaction::fetch_schemas Transaction::get_tables_max_se_private_id Schema::get_abstract_table Schema::get_table Schema::fetch_table_components Schema::fetch_tables Schema::fetch_views Tables::max_se_private_id dd_schema_exists dd_table_exists C2) DD method should assert() if something fails. We should never hit such failures. We mostly fix some other part of source when we hit these asserts. However we have one case, E.g., dd::Transaction::open_tables() is not expected to fail and we now assert if it fails. C3) DD method should fail (returns 'true') and it requires to call my_error(). E.g: Following are the two new error message added. * Report error to user if the state of DD object is not valid. ER_INVALID_DD_OBJECT eng "%s dictionary object is invalid. (%.192s)" * Report error to user if the DD object could not be stored in DD table. ER_UPDATING_DD_TABLE eng "Failed to update %.192s dictionary object." C4) DD method invokes SE API. SE API fails and does not report error. MySQL server needs to invoke my_error()/Handler::print_error(). E.g: This is most of DD classes Raw_* would need to do this, after calling SE API's. C5) DD method is forced to check thd->is_error() and take decisions if to continue or stop. This should never happen. The high level idea is that a function / operation must always return the error status. In other words, the DD operations must not return meaningful data, but only error status. The error status semantic must be common with the rest of the server. That is: - true means an error happened; - false means successful execution. The meaningful data must be returned as out-parameters. For example: bool get_object_by_id(..., Object *&obj); bool is_exists(..., bool &is_exists); In case operations need to return more than error flag (i.e. error code or something like that), the return type must not be C/C++ primitive type (int, long, ...). It must be a dedicated user type (like Error_code). The following alternatives have been considered: 1. Return meaningful data, store the error status inside some context (e.g. transaction context). Object *get_object_by_id(Transaction *trx, ...); if (trx->is_error()) ... The main downside of this approach is that it's not clear from the operation declaration if it returns error or not. Moreover, the operations prototype can change in the future and there is no way to do compiler-time check that all errors are handled. Practically, that means that either the error will not be checked, or there will be too many [unnecessary] error checking. 2. Return meaningful data, store the error status in a special error context passed to the operation. Object *get_object_by_id(Transaction *trx, ..., Error_ctx &err); if (err.is_error()) ... If we were to have complex error contexts, that would be fine. Having this complexity for just an error flag seems to be over the top. 3. C++ exceptions. The idea was that we could use C++ exceptions within the DD, but provide exception-free API.
Copyright (c) 2000, 2020, Oracle Corporation and/or its affiliates. All rights reserved.