PS-9148 feature: Dictionary caching for Masking Functions Component (8.0) #5520
Conversation
The fix for PS-9453 "percona_telemetry causes a long wait on COND_thd_list due to the absence of the root user" (commit 27468f8) partially reverted as a preparation step for cherry-picking Bug #34741098 "component::deinit() will block if calling any registry update APIs" (commit mysql/mysql-server@d39330f).
update APIs 1. A no_lock version of registry and registry_registration service is implemented which provides the same functionality without taking any lock on the registry. 2. MySQL command service is updated to use either the lock or no_lock version based on the new flag no_lock_registry added in mcs_ext. 3. The no_lock_registry flag is set to true by health monitor query thread before calling MySQL command service APIs (close, connect). Change-Id: I8ebf8f07cffb8ddc4de0f17c48c10cb15be7dad8
Re-applied the fix for PS-9453 "percona_telemetry causes a long wait on COND_thd_list due to the absence of the root user" (commit e53363d) partially reverted previously. After changes in 'cssm_begin_connect()' instead of cherry-picking from 8.0 branch the modified patch from 8.4 was taken. This is a finalization step of for cherry-picking Bug #34741098 "component::deinit() will block if calling any registry update APIs" (commit mysql/mysql-server@d39330f).
https://perconadev.atlassian.net/browse/PS-9551 Fixed problem in 'mysql_command_services_imp::set()'. When user sets the 'MYSQL_COMMAND_LOCAL_THD_HANDLE' option very early after server startup (when 'srv_session_server_is_available()' still returns false), 'service->open(nullptr, nullptr)' may return nullptr and it is unsafe to use it afterwards. Fixed by checking for nullness and returning earlier.
…ries in Command Services https://perconadev.atlassian.net/browse/PS-9537 Fixed problem with re-using the same connection created via 'mysql_command_factory->init()' / 'mysql_command_factory->connect()' in multiple calls to' mysql_command_query->query()'. The problem seems to be a regression introduced by upstream in their fix for Bug #34323788 "ASAN memory leaks reported by test_mysql_command_services_component.test" (commit mysql/mysql-server@5dc1a14). Inside 'csi_advanced_command()' 'mcs_extn->consumer_srv_data' when is not nullptr cannot be simply re-used as its `mcs_extn->data` member has already been set to nullptr by `std::exchange()` inside `csi_read_rows()`. Fixed by calling 'factory_srv->end()' and 'factory_srv->start()' in this case.
…g_functions https://perconadev.atlassian.net/browse/PS-9148 - Added caching of mysql.masking_dictionaries table content. - Implemented masking_dictionaries_flush() UDF which flushes data from the masking dictionaries table to the memory cache.
https://perconadev.atlassian.net/browse/PS-9148 The masking_functions.masking_database system variable for the masking_functions component specifies database used for data masking dictionaries.
…lugin https://perconadev.atlassian.net/browse/PS-9148 - Added component_masking.dictionaries_flush_interval_seconds system variable. - Added actual flusher thread. It periodically rereads content of dictionary table and updates in-memory cache.
…d terms https://perconadev.atlassian.net/browse/PS-9148 Introduced 'dictionary' and 'bookshelf' classes for storing terms on per-dictionary level. Reworked 'query_cache' to utilize these two new classes.
https://perconadev.atlassian.net/browse/PS-9148 Introduced 'component_sys_variable_service_tuple' class for groupping comonent system variable registration services (supposed to be used with 'primitive_singleton' class template). 'query_cache' now expects 'query_builder' and 'flusher_interval_seconds' as its constructor's parameters. Eliminates custom MySQL types (like 'ulonglong') and its includes (like 'my_inttypes.h') from the publicly facing headers. 'query_cache' is now explicitly initialized / deinitialized in the component's 'init()'' / 'deinit()'' functions via 'primitive_singleton' interface. 'query_cache' helper thread-related methods made private.
https://perconadev.atlassian.net/browse/PS-9148 As std::string_view::data() is not guaranteed to be null-terminated, it is not safe to use it in old c-functions accepting 'const char *'. Some constants converted to arrays of char 'const char buffer[]{"value"}'.
https://perconadev.atlassian.net/browse/PS-9148 'command_service_tuple' struct extended with one more member - 'field_info' service. Reworked 'query_cache' class: instead of loading terms from the database in constructor, this operation is now performed in first attempt to access one of the dictionary methods ('contains()' / 'get_random()' / 'remove()' / 'insert()'). This is done in order to overcome a limitation that does not allow 'mysql_command_query' service to be used from inside the componment initialization function. Fixed problem with 'm_dict_cache' shared pointer updated concurrently from different threads. Exceptions thrown from the cache loading function no longer escape the flusher thread. De-coupled 'sql_context' and 'bookshelf' classes: 'sql_context' now accepts a generic insertion callback that can be used to populate any type of containers. 'component_masking_functions.dictionary_operations' MTR test case extended with additional checks for flushed / unflushed dictionary cache.
https://perconadev.atlassian.net/browse/PS-9148 Both 'dictionary' and 'bookshelf' classes no longer include their own 'std::shared_mutex' to protect data. Instead, we now have a single 'std::shared_mutex' at the 'query_cache' level. The return value of the 'get_random()' method in both 'dictionary' and 'bookshelf' classes changed from 'optional_string' to 'std::string_view'. Empty (default constructed) 'std::string_view' is used as an indicator of an unsuccessful operation. 'get_random()' method in the 'query_cache' class still returns a string by value to avoid race conditions. Changed the behaviour of the 'sql_context::execute_dml()' method - it now throws when SQL errors (like "no table found", etc.) occur.
https://perconadev.atlassian.net/browse/PS-9148 Added missing my_thread_attr_t initialization.
…ry_cache https://perconadev.atlassian.net/browse/PS-9148 Threading-related functionality extracted from the 'query_cache' class into a separate 'dictionary_flusher_thread' class. This new class now accepts an instance of existing 'query_cache' class as a parameter of its constructor in a form of shared pointer. Changed the way how these two objects are now initialized / deinitialized in 'component.cpp' ('component_init()' / 'component_deinit()' functions).
https://perconadev.atlassian.net/browse/PS-9148 'dictionary_flusher_thread' class interface ('dictionary_flusher_thread.hpp') now includes no public / internal MySQL headers. Introduced internal 'thread_handler_context' class, which is supposed to be instantiated as the very first declaration of the MySQL thread handler function - it performs proper 'THD' object initialization in constructor and deinitialization in destructor. Introduced internal 'thread_attributes' class - an RAII wrapper over 'my_thread_attr_t' with proper initialization in constructor and deinitialization in destructor. Introduced internal 'jthread' class that similarly to 'std::jthread' from c++20 spawns a joinable thread in constructor and joins it in destructor. It expects only meaningful logic in a form of 'std::function<void()>' from the user and hides all the MySQL initialization / PSI registration boilerplate. It uses an instance of 'thread_attributes' class when spawns a thread. It also creates an instance of the 'thread_handler_context' class inside actual thread handler function ('jthread::raw_handler()'). This class also makes sure that no exception escapes actual thread handler function. Refactored error handling in 'component_init()'. Fixed problem with updating 'stopped_' variable (which the condition variable uses in its 'waitt_for()' method) without properly locking the mutex. Fixed instabilities in 'component_masking_functions.rpl_dictionaries_flush_interval' MTR test case.
…ql_context https://perconadev.atlassian.net/browse/PS-9148 'command_service_tuple' class extended with new 'error_info' member of type 'SERVICE_TYPE(mysql_command_error_info) *' that allows extracting MySQL error codes and messages. It is expected to be used inside 'sql_context' class methods. Reworked the way how exceptions are thrown from the 'sql_context' class methods - we now use 'raise_with_error_message()' helper method that throws an exception that incorporates MySQL client API error message extracted via added 'error_info' member of the `command_service_tuple` class. More meaningful error messages, which include underlying MySQL error descriptions, are now generated from inside the 'query_cache' class methods. Added new DBUG keywords and DEBUG_SYNC actions that allow control over Dictionary Flusher background thread actions. Added new 'component_masking_functions.flusher_thread_suspend_resume' MTR test case that checks for various race conditions between current session and Dictionary Flusher background thread. Improved 'component_masking_functions.rpl_dictionaries_flush_interval' MTR test case - pre-recorder values replaced with 'assert.inc'.
https://perconadev.atlassian.net/browse/PS-9148 'sql_context' class constructor now takes one extra parameter that allows to specify whether the user wants to set the 'MYSQL_NO_LOCK_REGISTRY' option or not. Introduced new abstract class 'basic_sql_context_builder' that is used to construct instances of the 'sql_context' class on demand. Also added two concrete implementations of this interface: 'default_sql_context_builder' and 'static_sql_context_builder'. The former just creates a new instance of the 'sql_context' class every time the 'build()' method is invoked. This implementation is used from inside the UDF function implementation methods. The latter creates an instance of the 'sql_context' class during the first call to the 'build()' method, saves it internally and returns this saved instance for each subsequent call to the 'build()' method. This implementation is used in the 'dictionary_flusher_thread'. Refactored 'query_cache' class: basic functionality that needs external 'basic_sql_context_builder' and 'query_builder' extracted into separate class 'sql_cache_core'. For convenience, added new 'sql_cache' class as a wrapper over existing 'sql_cache_code', 'basic_sql_context_builder' and 'query_builder'. The same 'sql_cache_core' can be shared between multiple instances of the `sql_cache`. This allowed to make sure that `dictionary_flusher_thread` uses a dedicated long-living instance of the 'sql_context' class (with 'MYSQL_NO_LOCK_REGISTRY' enabled) and does not cause deadlocks during 'UNINSTALL COMPONENT'. See Bug #34741098 "component::deinit() will block if calling any registry" (commit mysql/mysql-server@d39330f) for more details. As the 'sql_context' connection in the 'dictionary_flusher_thread' is now a long-living one, it is now shown in the output of the 'SHOW PROCESSLIST' statement. Modified 'count_sessions.inc' / 'wait_until_count_sessions.inc' logic inside 'component_masking_functions.flusher_thread_suspend_resume' MTR test case to reflect these changes.
…nation https://perconadev.atlassian.net/browse/PS-9148 'command_service_tuple' extended with one more member 'thread' which is used to initialize / deinitialize threads intended to be used as MySQL threads. 'sql_context' class constructor now takes one extra parameter that allows to specify whether the user wants to associate a new session (including an instance of the 'THD' class) with the calling thread. Internally it is done by setting the 'MYSQL_COMMAND_LOCAL_THD_HANDLE' option to nullptr. Also 'sql_context' now tries to open connections on behalf of the internal predefined 'mysql.session' MySQL user (instead of 'root'). Reworked 'static_sql_context_builder' class - it now creates a shared "static" instance of he 'sql_context' class inside the class constructor and passes true as its 'initialize_thread' parameter meaning an intent to associate the calling thread with this connection. Before this change, the construction was done inside the very first call to 'do_build()'. The regular ("new instance per request" ) implementation of the 'basic_sql_context_builder', 'default_sql_context_builder', now passes false as the 'initialize_thread' parameter (meaning no association with the thread needed). Significantly reworked 'dictionary_flusher_thread': - instead of composed 'query_cache' object it now expects its component 'query_cache_core' and 'query_builder' as constructor arguments. This allows to create an instance of the 'static_sql_context_builder' and 'query_cache' directly inside the thread function. - Instead of 'stopped_' boolean flag, we now have a state enumeration ('initial' 'initialization_failure', 'operational', 'stopped') - the implementation no longer uses 'std::conditional_variable' for awaiting timer events / termination requests. Instead, it just wakes up periodically (once per second) and checks it it needs to reload the cache. This is necessary to be able to respond to graceful termination requests like 'KILL CONNECTION' or shutdown closing sessions at shutdown. - added 'request_termination()' method used inside component 'deinit()' handler. - 'do_periodic_reload()' function now looks more lake a state machine performing different actions and making transitions to different states. - added new logic to wait for Sessin Server availability inside the 'do_periodic_reload()' function. Reworked 'thread_handler_context' class - it now uses 'mysql_command_thread' service to initialize / deinitialize the thread for MySQL. Various MTR test case that use dictionary functions updated with explicit granting necessary privileges on the dictionary table to the 'mysql.session'@'localhost' internal MySQL user. Added new 'component_masking_functions.flusher_thread_connection_reuse' MTR test case that checks that the same MySQL internal connection (created via 'mysql_command_xxx' services) can be used several times (without closing and re-opening) by the background flusher thread. Added new 'component_masking_functions.flusher_thread_immediate_restart' MTR test case that check for proper behavior during server shutdown immediately after installing the component. Added new 'wait_for_component_uninstall.inc' MTR include file which can be used to perform several attempts to 'UNINSTALL COMPONENT' until it succeeds or reaches the max number of attempts.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| const auto random_index{random_number(0, terms_.size() - 1U)}; | ||
| return *std::next(std::begin(terms_), random_index); |
There was a problem hiding this comment.
narrowing conversion from std::size_t (aka unsigned long) to signed type typename iterator_traits<_Node_const_iterator<basic_string<char>, true, true>>::difference_type (aka long) is implementation-defined
| sql_context_ptr build() const { return do_build(); } | ||
|
|
||
| protected: | ||
| basic_sql_context_builder(const command_service_tuple &services) |
There was a problem hiding this comment.
single-argument constructors must be marked explicit to avoid unintentional implicit conversions
| basic_sql_context_builder(const command_service_tuple &services) | |
| explicit basic_sql_context_builder(const command_service_tuple &services) |
|
|
||
| namespace masking_functions { | ||
|
|
||
| class bookshelf { |
There was a problem hiding this comment.
class bookshelf defines a non-default destructor, a copy assignment operator, a move constructor and a move assignment operator but does not define a copy constructor
|
|
||
| #include <my_dbug.h> | ||
| #include <my_sys.h> | ||
| #include <mysqld_error.h> |
There was a problem hiding this comment.
mysqld_error.h file not found
|
|
||
| #include "sql/debug_sync.h" | ||
|
|
||
| extern SERVICE_TYPE(log_builtins) * log_bi; |
There was a problem hiding this comment.
redundant log_bi declaration
| extern SERVICE_TYPE(log_builtins) * log_bi; |
| #include "sql/debug_sync.h" | ||
|
|
||
| extern SERVICE_TYPE(log_builtins) * log_bi; | ||
| extern SERVICE_TYPE(log_builtins_string) * log_bs; |
There was a problem hiding this comment.
redundant log_bs declaration
| extern SERVICE_TYPE(log_builtins_string) * log_bs; |
| using handler_function_type = std::function<void()>; | ||
|
|
||
| // passing 'handler_function' deliberately by value to move from | ||
| jthread(handler_function_type handler_function, const char *category_name, |
There was a problem hiding this comment.
parameter handler_function is unused
| jthread(handler_function_type handler_function, const char *category_name, | |
| jthread(const char *category_name, |
| return state_.exchange(state_type::stopped) != state_type::initial; | ||
| } | ||
|
|
||
| void dictionary_flusher_thread::do_periodic_reload() { |
There was a problem hiding this comment.
function do_periodic_reload has cognitive complexity of 70 (threshold 50)
| std::size_t number_of_attempts{0U}; | ||
|
|
||
| while (number_of_attempts < max_number_of_attempts && | ||
| state_ != state_type::stopped && !srv_session_server_is_available()) { |
There was a problem hiding this comment.
implicit conversion int -> bool
| state_ != state_type::stopped && !srv_session_server_is_available()) { | |
| state_ != state_type::stopped && (srv_session_server_is_available() == 0)) { |
| } else { | ||
| try { |
There was a problem hiding this comment.
do not use else after return
| } else { | |
| try { | |
| } try { |
| don't have to delete it explicitly. */ | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
do not use else after return
| } | |
| } | ||
|
|
||
| /* Pointer is stored in registry, thous we release ownership. */ | ||
| imp.release(); |
There was a problem hiding this comment.
the value returned by this function should be used
| !strcmp(imp->service_name_c_str(), | ||
| new_default_iter->second->service_name_c_str())) { |
There was a problem hiding this comment.
implicit conversion int -> bool
| !strcmp(imp->service_name_c_str(), | |
| new_default_iter->second->service_name_c_str())) { | |
| (strcmp(imp->service_name_c_str(), | |
| new_default_iter->second->service_name_c_str()) == 0)) { |
| */ | ||
| DEFINE_BOOL_METHOD(mysql_registry_no_lock_imp::acquire, | ||
| (const char *service_name, my_h_service *out_service)) { | ||
| return mysql_registry_no_lock_imp::acquire_nolock(service_name, out_service); |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return mysql_registry_no_lock_imp::acquire_nolock(service_name, out_service); | |
| return static_cast<mysql_service_status_t>(mysql_registry_no_lock_imp::acquire_nolock(service_name, out_service)); |
| mysql_registry_no_lock_imp::get_service_implementation_by_interface( | ||
| service); | ||
| if (service_implementation == nullptr) { | ||
| return true; |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return true; | |
| return 1; |
| const char *component_part = | ||
| strchr(service_implementation->name_c_str(), '.'); | ||
| if (component_part == nullptr) { | ||
| return true; |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return true; | |
| return 1; |
| } | ||
| /* Assure given service_name is not fully qualified. */ | ||
| if (strchr(service_name, '.') != nullptr) { | ||
| return true; |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return true; | |
| return 1; |
| if (mysql_registry_no_lock_imp::acquire_nolock( | ||
| service_implementation_name.c_str(), out_service)) { | ||
| /* service is not found */ | ||
| return true; |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return true; | |
| return 1; |
| /* service is not found */ | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return false; | |
| return 0; |
| return false; | ||
| } catch (...) { | ||
| } | ||
| return true; |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return true; | |
| return 1; |
| */ | ||
| DEFINE_BOOL_METHOD(mysql_registry_no_lock_imp::release, | ||
| (my_h_service service)) { | ||
| return mysql_registry_no_lock_imp::release_nolock(service); |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return mysql_registry_no_lock_imp::release_nolock(service); | |
| return static_cast<mysql_service_status_t>(mysql_registry_no_lock_imp::release_nolock(service)); |
| return mysql_registry_no_lock_imp::register_service_nolock( | ||
| service_implementation_name, ptr); |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return mysql_registry_no_lock_imp::register_service_nolock( | |
| service_implementation_name, ptr); | |
| return static_cast<mysql_service_status_t>(mysql_registry_no_lock_imp::register_service_nolock( | |
| service_implementation_name, ptr)); |
| return mysql_registry_no_lock_imp::unregister_nolock( | ||
| service_implementation_name); |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return mysql_registry_no_lock_imp::unregister_nolock( | |
| service_implementation_name); | |
| return static_cast<mysql_service_status_t>(mysql_registry_no_lock_imp::unregister_nolock( | |
| service_implementation_name)); |
| service_implementation_name); | ||
|
|
||
| if (iter == mysql_registry_no_lock_imp::service_registry.cend()) { | ||
| return true; |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return true; | |
| return 1; |
| mysql_registry_no_lock_imp::service_registry.emplace_hint( | ||
| iter, imp->service_name_c_str(), imp); | ||
|
|
||
| return false; |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return false; | |
| return 0; |
| return false; | ||
| } catch (...) { | ||
| } | ||
| return true; |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return true; | |
| return 1; |
| MYSQL *mysql = ctx->mysql; | ||
| Mysql_handle mysql_handle; | ||
| mysql_handle.mysql = mysql; | ||
| auto mcs_extn = MYSQL_COMMAND_SERVICE_EXTN(mysql); |
There was a problem hiding this comment.
auto mcs_extn can be declared as auto *mcs_extn
| auto mcs_extn = MYSQL_COMMAND_SERVICE_EXTN(mysql); | |
| auto *mcs_extn = MYSQL_COMMAND_SERVICE_EXTN(mysql); |
| MYSQL *mysql = ctx->mysql; | ||
| Mysql_handle mysql_handle; | ||
| mysql_handle.mysql = mysql; | ||
| auto mcs_extn = MYSQL_COMMAND_SERVICE_EXTN(mysql); |
There was a problem hiding this comment.
do not use C-style cast to convert between unrelated types
| MYSQL *mysql = ctx->mysql; | ||
| Mysql_handle mysql_handle; | ||
| mysql_handle.mysql = mysql; | ||
| auto mcs_extn = MYSQL_COMMAND_SERVICE_EXTN(mysql); |
There was a problem hiding this comment.
do not use reinterpret_cast
| const char *host = ctx->host; | ||
| const char *user = ctx->user; | ||
| const char *db = ctx->db; | ||
| MYSQL_THD thd; |
There was a problem hiding this comment.
variable thd is not initialized
| MYSQL_THD thd; | |
| MYSQL_THD thd = nullptr; |
| MYSQL_SESSION mysql_session = nullptr; | ||
|
|
||
| if (mysql_command_services_imp::get( | ||
| (MYSQL_H)&mysql_handle, MYSQL_NO_LOCK_REGISTRY, &no_lock_registry)) |
There was a problem hiding this comment.
implicit conversion mysql_service_status_t (aka int) -> bool
| (MYSQL_H)&mysql_handle, MYSQL_NO_LOCK_REGISTRY, &no_lock_registry)) | |
| (MYSQL_H)&mysql_handle, MYSQL_NO_LOCK_REGISTRY, &no_lock_registry) != 0) |
| MYSQL_SESSION mysql_session = nullptr; | ||
|
|
||
| if (mysql_command_services_imp::get( | ||
| (MYSQL_H)&mysql_handle, MYSQL_NO_LOCK_REGISTRY, &no_lock_registry)) |
There was a problem hiding this comment.
do not use C-style cast to convert between unrelated types
| MYSQL_SESSION mysql_session = nullptr; | ||
|
|
||
| if (mysql_command_services_imp::get( | ||
| (MYSQL_H)&mysql_handle, MYSQL_NO_LOCK_REGISTRY, &no_lock_registry)) |
There was a problem hiding this comment.
C-style casts are discouraged; use reinterpret_cast
| (MYSQL_H)&mysql_handle, MYSQL_NO_LOCK_REGISTRY, &no_lock_registry)) | |
| reinterpret_cast<MYSQL_H>(&mysql_handle), MYSQL_NO_LOCK_REGISTRY, &no_lock_registry)) |
| if (mysql_session == nullptr) return STATE_MACHINE_FAILED; | ||
| thd = mysql_session->get_thd(); | ||
| mcs_extn->is_thd_associated = false; | ||
| Security_context_handle sc; |
There was a problem hiding this comment.
variable sc is not initialized
| Security_context_handle sc; | |
| Security_context_handle sc = nullptr; |
| thd = mysql_session->get_thd(); | ||
| mcs_extn->is_thd_associated = false; | ||
| Security_context_handle sc; | ||
| if (mysql_security_context_imp::get(thd, &sc)) return STATE_MACHINE_FAILED; |
There was a problem hiding this comment.
implicit conversion mysql_service_status_t (aka int) -> bool
| if (mysql_security_context_imp::get(thd, &sc)) return STATE_MACHINE_FAILED; | |
| if (mysql_security_context_imp::get(thd, &sc) != 0) return STATE_MACHINE_FAILED; |
| mcs_extn->is_thd_associated = false; | ||
| Security_context_handle sc; | ||
| if (mysql_security_context_imp::get(thd, &sc)) return STATE_MACHINE_FAILED; | ||
| if (mysql_security_context_imp::lookup(sc, user, host, nullptr, db)) |
There was a problem hiding this comment.
implicit conversion mysql_service_status_t (aka int) -> bool
| if (mysql_security_context_imp::lookup(sc, user, host, nullptr, db)) | |
| if (mysql_security_context_imp::lookup(sc, user, host, nullptr, db) != 0) |
| mysql->thd = thd; | ||
| mcs_extn->session_svc = mysql_session; | ||
| } else { | ||
| mysql->thd = reinterpret_cast<void *>(mcs_extn->mcs_thd); |
There was a problem hiding this comment.
do not use reinterpret_cast
| mcs_extn->command_consumer_services = new mysql_command_consumer_refs(); | ||
| } | ||
| mysql_command_consumer_refs *consumer_refs = | ||
| (mysql_command_consumer_refs *)mcs_extn->command_consumer_services; |
There was a problem hiding this comment.
do not use C-style cast to convert between unrelated types
| mcs_extn->command_consumer_services = new mysql_command_consumer_refs(); | ||
| } | ||
| mysql_command_consumer_refs *consumer_refs = | ||
| (mysql_command_consumer_refs *)mcs_extn->command_consumer_services; |
There was a problem hiding this comment.
C-style casts are discouraged; use static_cast
| (mysql_command_consumer_refs *)mcs_extn->command_consumer_services; | |
| static_cast<mysql_command_consumer_refs *>(mcs_extn->command_consumer_services); |
| else if (((class mysql_command_consumer_refs *)(command_consumer_srv)) | ||
| ->factory_srv->start(&srv_ctx_h, (MYSQL_H *)&mysql_handle)) { | ||
| if (mcs_extn->consumer_srv_data != nullptr) { | ||
| ((class mysql_command_consumer_refs *)(command_consumer_srv)) |
There was a problem hiding this comment.
do not use C-style cast to convert between unrelated types
| else if (((class mysql_command_consumer_refs *)(command_consumer_srv)) | ||
| ->factory_srv->start(&srv_ctx_h, (MYSQL_H *)&mysql_handle)) { | ||
| if (mcs_extn->consumer_srv_data != nullptr) { | ||
| ((class mysql_command_consumer_refs *)(command_consumer_srv)) |
There was a problem hiding this comment.
C-style casts are discouraged; use static_cast
| ((class mysql_command_consumer_refs *)(command_consumer_srv)) | |
| (static_cast<class mysql_command_consumer_refs *>(command_consumer_srv)) |
| if (mcs_extn->consumer_srv_data != nullptr) { | ||
| ((class mysql_command_consumer_refs *)(command_consumer_srv)) | ||
| ->factory_srv->end( | ||
| reinterpret_cast<SRV_CTX_H>(mcs_extn->consumer_srv_data)); |
There was a problem hiding this comment.
do not use reinterpret_cast
| reinterpret_cast<SRV_CTX_H>(mcs_extn->consumer_srv_data)); | ||
| } | ||
| if (((class mysql_command_consumer_refs *)(command_consumer_srv)) | ||
| ->factory_srv->start(&srv_ctx_h, (MYSQL_H *)&mysql_handle)) { |
There was a problem hiding this comment.
implicit conversion mysql_service_status_t (aka int) -> bool
| ->factory_srv->start(&srv_ctx_h, (MYSQL_H *)&mysql_handle)) { | |
| ->factory_srv->start(&srv_ctx_h, (MYSQL_H *)&mysql_handle) != 0) { |
| ->factory_srv->end( | ||
| reinterpret_cast<SRV_CTX_H>(mcs_extn->consumer_srv_data)); | ||
| } | ||
| if (((class mysql_command_consumer_refs *)(command_consumer_srv)) |
There was a problem hiding this comment.
do not use C-style cast to convert between unrelated types
| ->factory_srv->end( | ||
| reinterpret_cast<SRV_CTX_H>(mcs_extn->consumer_srv_data)); | ||
| } | ||
| if (((class mysql_command_consumer_refs *)(command_consumer_srv)) |
There was a problem hiding this comment.
C-style casts are discouraged; use static_cast
| if (((class mysql_command_consumer_refs *)(command_consumer_srv)) | |
| if ((static_cast<class mysql_command_consumer_refs *>(command_consumer_srv)) |
| reinterpret_cast<SRV_CTX_H>(mcs_extn->consumer_srv_data)); | ||
| } | ||
| if (((class mysql_command_consumer_refs *)(command_consumer_srv)) | ||
| ->factory_srv->start(&srv_ctx_h, (MYSQL_H *)&mysql_handle)) { |
There was a problem hiding this comment.
do not use C-style cast to convert between unrelated types
| reinterpret_cast<SRV_CTX_H>(mcs_extn->consumer_srv_data)); | ||
| } | ||
| if (((class mysql_command_consumer_refs *)(command_consumer_srv)) | ||
| ->factory_srv->start(&srv_ctx_h, (MYSQL_H *)&mysql_handle)) { |
There was a problem hiding this comment.
C-style casts are discouraged; use reinterpret_cast
| ->factory_srv->start(&srv_ctx_h, (MYSQL_H *)&mysql_handle)) { | |
| ->factory_srv->start(&srv_ctx_h, reinterpret_cast<MYSQL_H *>(&mysql_handle))) { |
| @@ -137,6 +137,7 @@ CREATE TABLE mysql.masking_dictionaries( | |||
| Term VARCHAR(256) NOT NULL, | |||
| UNIQUE INDEX dictionary_term_idx (Dictionary, Term) | |||
| ) ENGINE = InnoDB DEFAULT CHARSET=utf8mb4; | |||
| GRANT SELECT, INSERT, UPDATE, DELETE ON mysql.masking_dictionaries TO 'mysql.session'@'localhost'; | |||
There was a problem hiding this comment.
unknown type name GRANT
| @@ -137,6 +137,7 @@ CREATE TABLE mysql.masking_dictionaries( | |||
| Term VARCHAR(256) NOT NULL, | |||
| UNIQUE INDEX dictionary_term_idx (Dictionary, Term) | |||
| ) ENGINE = InnoDB DEFAULT CHARSET=utf8mb4; | |||
| GRANT SELECT, INSERT, UPDATE, DELETE ON mysql.masking_dictionaries TO 'mysql.session'@'localhost'; | |||
There was a problem hiding this comment.
expected ; after top level declarator
| GRANT SELECT, INSERT, UPDATE, DELETE ON mysql.masking_dictionaries TO 'mysql.session'@'localhost'; | |
| GRANT SELECT, INSERT, UPDATE, DELETE; ON mysql.masking_dictionaries TO 'mysql.session'@'localhost'; |
| @@ -137,6 +137,7 @@ CREATE TABLE mysql.masking_dictionaries( | |||
| Term VARCHAR(256) NOT NULL, | |||
| UNIQUE INDEX dictionary_term_idx (Dictionary, Term) | |||
| ) ENGINE = InnoDB DEFAULT CHARSET=utf8mb4; | |||
| GRANT SELECT, INSERT, UPDATE, DELETE ON mysql.masking_dictionaries TO 'mysql.session'@'localhost'; | |||
|
|
|||
| --let $current_user = `SELECT USER()` | |||
There was a problem hiding this comment.
expected unqualified-id
| #include <mysql/components/services/component_sys_var_service.h> | ||
| #include <mysql/components/services/log_builtins.h> | ||
|
|
||
| #include <mysqld_error.h> |
There was a problem hiding this comment.
mysqld_error.h file not found
| ++iter; | ||
| return iter == mysql_registry_imp::service_registry.cend(); | ||
| return iter == mysql_registry_no_lock_imp::service_registry.cend(); |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return iter == mysql_registry_no_lock_imp::service_registry.cend(); | |
| return static_cast<mysql_service_status_t>(iter == mysql_registry_no_lock_imp::service_registry.cend()); |
| @@ -649,7 +360,7 @@ DEFINE_BOOL_METHOD(mysql_registry_imp::iterator_is_valid, | |||
| my_service_registry::const_iterator &iter = | |||
| reinterpret_cast<my_h_service_iterator_imp *>(iterator)->m_it; | |||
|
|
|||
| return iter == mysql_registry_imp::service_registry.cend(); | |||
| return iter == mysql_registry_no_lock_imp::service_registry.cend(); | |||
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return iter == mysql_registry_no_lock_imp::service_registry.cend(); | |
| return static_cast<mysql_service_status_t>(iter == mysql_registry_no_lock_imp::service_registry.cend()); |
| @@ -8,7 +8,7 @@ if (!$MASKING_FUNCTIONS_COMPONENT) { | |||
| } | |||
|
|
|||
| # | |||
| ## Check if --plugin-dir was setup for component_encryption_udf | |||
| ## Check if --plugin-dir was setup for component_masking_functions | |||
There was a problem hiding this comment.
expected unqualified-id
|
|
||
| namespace masking_functions { | ||
|
|
||
| query_cache_core::query_cache_core() : dict_cache_{}, dict_cache_mutex_{} {} |
There was a problem hiding this comment.
initializer for member dict_cache_ is redundant
| query_cache_core::query_cache_core() : dict_cache_{}, dict_cache_mutex_{} {} | |
| query_cache_core::query_cache_core() : , dict_cache_mutex_{} {} |
|
|
||
| namespace masking_functions { | ||
|
|
||
| query_cache_core::query_cache_core() : dict_cache_{}, dict_cache_mutex_{} {} |
There was a problem hiding this comment.
initializer for member dict_cache_mutex_ is redundant
| query_cache_core::query_cache_core() : dict_cache_{}, dict_cache_mutex_{} {} | |
| query_cache_core::query_cache_core() : dict_cache_{}, {} |
| mutable bookshelf_ptr dict_cache_; | ||
| mutable std::shared_mutex dict_cache_mutex_; | ||
|
|
||
| bookshelf_ptr create_dict_cache_internal( |
There was a problem hiding this comment.
method create_dict_cache_internal can be made static
| bookshelf_ptr create_dict_cache_internal( | |
| static bookshelf_ptr create_dict_cache_internal( |
|
|
||
| bookshelf_ptr create_dict_cache_internal( | ||
| sql_context &sql_ctx, const query_builder &sql_query_builder, | ||
| std::string &error_message) const; |
There was a problem hiding this comment.
method create_dict_cache_internal can be made static
| std::string &error_message) const; | |
| std::string &error_message) ; |
|
|
||
| bookshelf_ptr query_cache_core::create_dict_cache_internal( | ||
| sql_context &sql_ctx, const query_builder &sql_query_builder, | ||
| std::string &error_message) const { |
There was a problem hiding this comment.
method create_dict_cache_internal can be made static
| std::string &error_message) const { | |
| std::string &error_message) { |
| @@ -2087,6 +2088,10 @@ static bool component_infrastructure_init() { | |||
| LogErr(ERROR_LEVEL, ER_COMPONENTS_INFRASTRUCTURE_BOOTSTRAP); | |||
| return true; | |||
| } | |||
| srv_registry->acquire( | |||
| "registry.mysql_minimal_chassis_no_lock", | |||
| reinterpret_cast<my_h_service *>(&srv_registry_no_lock)); | |||
There was a problem hiding this comment.
do not use reinterpret_cast
| @@ -2192,6 +2197,7 @@ static bool component_infrastructure_deinit() { | |||
|
|
|||
| srv_registry->release(reinterpret_cast<my_h_service>( | |||
| const_cast<loader_scheme_type_t *>(scheme_file_srv))); | |||
| srv_registry->release(reinterpret_cast<my_h_service>(srv_registry_no_lock)); | |||
There was a problem hiding this comment.
do not use reinterpret_cast
| @@ -0,0 +1,35 @@ | |||
| # When flusher thread is enabled in masking functions component, | |||
There was a problem hiding this comment.
invalid preprocessing directive
| @@ -0,0 +1,35 @@ | |||
| # When flusher thread is enabled in masking functions component, | |||
| # 'UNINSTALL COMPONENT' statement is allowed to fail (in case when run too | |||
There was a problem hiding this comment.
invalid preprocessing directive
| @@ -0,0 +1,35 @@ | |||
| # When flusher thread is enabled in masking functions component, | |||
| # 'UNINSTALL COMPONENT' statement is allowed to fail (in case when run too | |||
| # early and MySQL session in the background thread has not been initialized | |||
There was a problem hiding this comment.
invalid preprocessing directive
| # When flusher thread is enabled in masking functions component, | ||
| # 'UNINSTALL COMPONENT' statement is allowed to fail (in case when run too | ||
| # early and MySQL session in the background thread has not been initialized | ||
| # yet). |
There was a problem hiding this comment.
invalid preprocessing directive
| # early and MySQL session in the background thread has not been initialized | ||
| # yet). | ||
|
|
||
| # This .inc file is supposed to make several attempts to execute |
There was a problem hiding this comment.
invalid preprocessing directive
| # yet). | ||
|
|
||
| # This .inc file is supposed to make several attempts to execute | ||
| # 'UNINSTALL COMPONENT' in a loop until this statement succeeds. |
There was a problem hiding this comment.
invalid preprocessing directive
| # This .inc file is supposed to make several attempts to execute | ||
| # 'UNINSTALL COMPONENT' in a loop until this statement succeeds. | ||
|
|
||
| --disable_query_log |
There was a problem hiding this comment.
expected unqualified-id
| --sleep 1 | ||
| } | ||
| } | ||
| if (!$counter) |
There was a problem hiding this comment.
expected unqualified-id
| { | ||
| --die Unable to uninstall component | ||
| } | ||
| if ($counter) |
There was a problem hiding this comment.
expected unqualified-id
| { | ||
| --echo Component successfully uninstalled | ||
| } | ||
| --enable_query_log |
There was a problem hiding this comment.
expected unqualified-id
|
|
||
| namespace masking_functions { | ||
|
|
||
| class static_sql_context_builder : public basic_sql_context_builder { |
There was a problem hiding this comment.
class static_sql_context_builder defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator
| @@ -1229,6 +1247,7 @@ DECLARE_STRING_UDF_AUTO(mask_uk_nin) | |||
| DECLARE_STRING_UDF_AUTO(mask_uuid) | |||
| DECLARE_STRING_UDF_AUTO(gen_blocklist) | |||
| DECLARE_STRING_UDF_AUTO(gen_dictionary) | |||
| DECLARE_STRING_UDF_AUTO(masking_dictionaries_flush) | |||
There was a problem hiding this comment.
variable initid is not initialized
| @@ -1229,6 +1247,7 @@ DECLARE_STRING_UDF_AUTO(mask_uk_nin) | |||
| DECLARE_STRING_UDF_AUTO(mask_uuid) | |||
| DECLARE_STRING_UDF_AUTO(gen_blocklist) | |||
| DECLARE_STRING_UDF_AUTO(gen_dictionary) | |||
| DECLARE_STRING_UDF_AUTO(masking_dictionaries_flush) | |||
There was a problem hiding this comment.
parameter initid is unused
| std::string_view query) { | ||
| if ((*get_services().query->query)(to_mysql_h(impl_.get()), query.data(), | ||
| bool sql_context::execute_dml(std::string_view query) { | ||
| const auto casted_impl{to_mysql_h(impl_.get())}; |
There was a problem hiding this comment.
const auto casted_impl can be declared as auto *const casted_impl
| const auto casted_impl{to_mysql_h(impl_.get())}; | |
| auto *const casted_impl{to_mysql_h(impl_.get())}; |
| void sql_context::execute_select_internal( | ||
| std::string_view query, std::size_t expected_number_of_fields, | ||
| const row_internal_callback &callback) { | ||
| const auto casted_impl{to_mysql_h(impl_.get())}; |
There was a problem hiding this comment.
const auto casted_impl can be declared as auto *const casted_impl
| const auto casted_impl{to_mysql_h(impl_.get())}; | |
| auto *const casted_impl{to_mysql_h(impl_.get())}; |
| using error_message_buffer_type = std::array<char, MYSQL_ERRMSG_SIZE>; | ||
| error_message_buffer_type error_message_buffer; | ||
| char *error_message_buffer_ptr{std::data(error_message_buffer)}; | ||
| const auto casted_impl{to_mysql_h(impl_.get())}; |
There was a problem hiding this comment.
const auto casted_impl can be declared as auto *const casted_impl
| const auto casted_impl{to_mysql_h(impl_.get())}; | |
| auto *const casted_impl{to_mysql_h(impl_.get())}; |
| static void release_services(mysql_command_consumer_refs *consumer_refs, | ||
| mysql_command_service_extn *mcs_ext, | ||
| mysql_service_registry_t *srv_registry) { | ||
| if (consumer_refs) { |
There was a problem hiding this comment.
implicit conversion mysql_command_consumer_refs * -> bool
| if (consumer_refs) { | |
| if (consumer_refs != nullptr) { |
| mysql_command_service_extn *mcs_ext, | ||
| mysql_service_registry_t *srv_registry) { | ||
| if (consumer_refs) { | ||
| if (consumer_refs->factory_srv) { |
There was a problem hiding this comment.
implicit conversion const mysql_service_mysql_text_consumer_factory_v1_t * (aka const s_mysql_mysql_text_consumer_factory_v1 *) -> bool
| if (consumer_refs->factory_srv) { | |
| if (consumer_refs->factory_srv != nullptr) { |
| /* This service call is used to free the memory, the allocation | ||
| was happened through factory_srv->start() service api. */ | ||
| consumer_refs->factory_srv->end( | ||
| reinterpret_cast<SRV_CTX_H>(mcs_ext->consumer_srv_data)); |
There was a problem hiding this comment.
do not use reinterpret_cast
| was happened through factory_srv->start() service api. */ | ||
| consumer_refs->factory_srv->end( | ||
| reinterpret_cast<SRV_CTX_H>(mcs_ext->consumer_srv_data)); | ||
| srv_registry->release(reinterpret_cast<my_h_service>( |
There was a problem hiding this comment.
do not use reinterpret_cast
| consumer_refs->factory_srv->end( | ||
| reinterpret_cast<SRV_CTX_H>(mcs_ext->consumer_srv_data)); | ||
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_factory_v1) *>( |
There was a problem hiding this comment.
do not use const_cast
| const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_factory_v1) *>( | ||
| consumer_refs->factory_srv))); | ||
| } | ||
| if (consumer_refs->metadata_srv) |
There was a problem hiding this comment.
implicit conversion const mysql_service_mysql_text_consumer_metadata_v1_t * (aka const s_mysql_mysql_text_consumer_metadata_v1 *) -> bool
| if (consumer_refs->metadata_srv) | |
| if (consumer_refs->metadata_srv != nullptr) |
| consumer_refs->factory_srv))); | ||
| } | ||
| if (consumer_refs->metadata_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( |
There was a problem hiding this comment.
do not use reinterpret_cast
| } | ||
| if (consumer_refs->metadata_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_metadata_v1) *>( |
There was a problem hiding this comment.
do not use const_cast
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_metadata_v1) *>( | ||
| consumer_refs->metadata_srv))); | ||
| if (consumer_refs->row_factory_srv) |
There was a problem hiding this comment.
implicit conversion const mysql_service_mysql_text_consumer_row_factory_v1_t * (aka const s_mysql_mysql_text_consumer_row_factory_v1 *) -> bool
| if (consumer_refs->row_factory_srv) | |
| if (consumer_refs->row_factory_srv != nullptr) |
| const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_metadata_v1) *>( | ||
| consumer_refs->metadata_srv))); | ||
| if (consumer_refs->row_factory_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( |
There was a problem hiding this comment.
do not use reinterpret_cast
| consumer_refs->metadata_srv))); | ||
| if (consumer_refs->row_factory_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST( |
There was a problem hiding this comment.
do not use const_cast
| const_cast<SERVICE_TYPE_NO_CONST( | ||
| mysql_text_consumer_row_factory_v1) *>( | ||
| consumer_refs->row_factory_srv))); | ||
| if (consumer_refs->error_srv) |
There was a problem hiding this comment.
implicit conversion const mysql_service_mysql_text_consumer_error_v1_t * (aka const s_mysql_mysql_text_consumer_error_v1 *) -> bool
| if (consumer_refs->error_srv) | |
| if (consumer_refs->error_srv != nullptr) |
| mysql_text_consumer_row_factory_v1) *>( | ||
| consumer_refs->row_factory_srv))); | ||
| if (consumer_refs->error_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( |
There was a problem hiding this comment.
do not use reinterpret_cast
| consumer_refs->row_factory_srv))); | ||
| if (consumer_refs->error_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_error_v1) *>( |
There was a problem hiding this comment.
do not use const_cast
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_error_v1) *>( | ||
| consumer_refs->error_srv))); | ||
| if (consumer_refs->get_null_srv) |
There was a problem hiding this comment.
implicit conversion const mysql_service_mysql_text_consumer_get_null_v1_t * (aka const s_mysql_mysql_text_consumer_get_null_v1 *) -> bool
| if (consumer_refs->get_null_srv) | |
| if (consumer_refs->get_null_srv != nullptr) |
| const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_error_v1) *>( | ||
| consumer_refs->error_srv))); | ||
| if (consumer_refs->get_null_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( |
There was a problem hiding this comment.
do not use reinterpret_cast
| consumer_refs->error_srv))); | ||
| if (consumer_refs->get_null_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_get_null_v1) *>( |
There was a problem hiding this comment.
do not use const_cast
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_get_null_v1) *>( | ||
| consumer_refs->get_null_srv))); | ||
| if (consumer_refs->get_integer_srv) |
There was a problem hiding this comment.
implicit conversion const mysql_service_mysql_text_consumer_get_integer_v1_t * (aka const s_mysql_mysql_text_consumer_get_integer_v1 *) -> bool
| if (consumer_refs->get_integer_srv) | |
| if (consumer_refs->get_integer_srv != nullptr) |
| const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_get_null_v1) *>( | ||
| consumer_refs->get_null_srv))); | ||
| if (consumer_refs->get_integer_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( |
There was a problem hiding this comment.
do not use reinterpret_cast
| consumer_refs->get_null_srv))); | ||
| if (consumer_refs->get_integer_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST( |
There was a problem hiding this comment.
do not use const_cast
| const_cast<SERVICE_TYPE_NO_CONST( | ||
| mysql_text_consumer_get_integer_v1) *>( | ||
| consumer_refs->get_integer_srv))); | ||
| if (consumer_refs->get_longlong_srv) |
There was a problem hiding this comment.
implicit conversion const mysql_service_mysql_text_consumer_get_longlong_v1_t * (aka const s_mysql_mysql_text_consumer_get_longlong_v1 *) -> bool
| if (consumer_refs->get_longlong_srv) | |
| if (consumer_refs->get_longlong_srv != nullptr) |
| mysql_text_consumer_get_integer_v1) *>( | ||
| consumer_refs->get_integer_srv))); | ||
| if (consumer_refs->get_longlong_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( |
There was a problem hiding this comment.
do not use reinterpret_cast
| consumer_refs->get_integer_srv))); | ||
| if (consumer_refs->get_longlong_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST( |
There was a problem hiding this comment.
do not use const_cast
| const_cast<SERVICE_TYPE_NO_CONST( | ||
| mysql_text_consumer_get_longlong_v1) *>( | ||
| consumer_refs->get_longlong_srv))); | ||
| if (consumer_refs->get_decimal_srv) |
There was a problem hiding this comment.
implicit conversion const mysql_service_mysql_text_consumer_get_decimal_v1_t * (aka const s_mysql_mysql_text_consumer_get_decimal_v1 *) -> bool
| if (consumer_refs->get_decimal_srv) | |
| if (consumer_refs->get_decimal_srv != nullptr) |
| mysql_text_consumer_get_longlong_v1) *>( | ||
| consumer_refs->get_longlong_srv))); | ||
| if (consumer_refs->get_decimal_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( |
There was a problem hiding this comment.
do not use reinterpret_cast
| consumer_refs->get_longlong_srv))); | ||
| if (consumer_refs->get_decimal_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST( |
There was a problem hiding this comment.
do not use const_cast
| const_cast<SERVICE_TYPE_NO_CONST( | ||
| mysql_text_consumer_get_decimal_v1) *>( | ||
| consumer_refs->get_decimal_srv))); | ||
| if (consumer_refs->get_double_srv) |
There was a problem hiding this comment.
implicit conversion const mysql_service_mysql_text_consumer_get_double_v1_t * (aka const s_mysql_mysql_text_consumer_get_double_v1 *) -> bool
| if (consumer_refs->get_double_srv) | |
| if (consumer_refs->get_double_srv != nullptr) |
| mysql_text_consumer_get_decimal_v1) *>( | ||
| consumer_refs->get_decimal_srv))); | ||
| if (consumer_refs->get_double_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( |
There was a problem hiding this comment.
do not use reinterpret_cast
| consumer_refs->get_decimal_srv))); | ||
| if (consumer_refs->get_double_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST( |
There was a problem hiding this comment.
do not use const_cast
| const_cast<SERVICE_TYPE_NO_CONST( | ||
| mysql_text_consumer_get_double_v1) *>( | ||
| consumer_refs->get_double_srv))); | ||
| if (consumer_refs->get_date_time_srv) |
There was a problem hiding this comment.
implicit conversion const mysql_service_mysql_text_consumer_get_date_time_v1_t * (aka const s_mysql_mysql_text_consumer_get_date_time_v1 *) -> bool
| if (consumer_refs->get_date_time_srv) | |
| if (consumer_refs->get_date_time_srv != nullptr) |
| mysql_text_consumer_get_double_v1) *>( | ||
| consumer_refs->get_double_srv))); | ||
| if (consumer_refs->get_date_time_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( |
There was a problem hiding this comment.
do not use reinterpret_cast
| consumer_refs->get_double_srv))); | ||
| if (consumer_refs->get_date_time_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST( |
There was a problem hiding this comment.
do not use const_cast
| const_cast<SERVICE_TYPE_NO_CONST( | ||
| mysql_text_consumer_get_date_time_v1) *>( | ||
| consumer_refs->get_date_time_srv))); | ||
| if (consumer_refs->get_string_srv) |
There was a problem hiding this comment.
implicit conversion const mysql_service_mysql_text_consumer_get_string_v1_t * (aka const s_mysql_mysql_text_consumer_get_string_v1 *) -> bool
| if (consumer_refs->get_string_srv) | |
| if (consumer_refs->get_string_srv != nullptr) |
| mysql_text_consumer_get_date_time_v1) *>( | ||
| consumer_refs->get_date_time_srv))); | ||
| if (consumer_refs->get_string_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( |
There was a problem hiding this comment.
do not use reinterpret_cast
| consumer_refs->get_date_time_srv))); | ||
| if (consumer_refs->get_string_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST( |
There was a problem hiding this comment.
do not use const_cast
| const_cast<SERVICE_TYPE_NO_CONST( | ||
| mysql_text_consumer_get_string_v1) *>( | ||
| consumer_refs->get_string_srv))); | ||
| if (consumer_refs->client_capabilities_srv) |
There was a problem hiding this comment.
implicit conversion const mysql_service_mysql_text_consumer_client_capabilities_v1_t * (aka const s_mysql_mysql_text_consumer_client_capabilities_v1 *) -> bool
| if (consumer_refs->client_capabilities_srv) | |
| if (consumer_refs->client_capabilities_srv != nullptr) |
| mysql_text_consumer_get_string_v1) *>( | ||
| consumer_refs->get_string_srv))); | ||
| if (consumer_refs->client_capabilities_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( |
There was a problem hiding this comment.
do not use reinterpret_cast
| consumer_refs->get_string_srv))); | ||
| if (consumer_refs->client_capabilities_srv) | ||
| srv_registry->release(reinterpret_cast<my_h_service>( | ||
| const_cast<SERVICE_TYPE_NO_CONST( |
There was a problem hiding this comment.
do not use const_cast
| @@ -176,76 +251,14 @@ DEFINE_BOOL_METHOD(mysql_command_services_imp::close, (MYSQL_H mysql_h)) { | |||
| auto mcs_ext = MYSQL_COMMAND_SERVICE_EXTN(mysql); | |||
| mysql_command_consumer_refs *consumer_refs = | |||
| (mysql_command_consumer_refs *)(mcs_ext->command_consumer_services); | |||
| bool no_lock_registry = false; | |||
| if (get(mysql_h, MYSQL_NO_LOCK_REGISTRY, &no_lock_registry)) return true; | |||
There was a problem hiding this comment.
implicit conversion mysql_service_status_t (aka int) -> bool
| if (get(mysql_h, MYSQL_NO_LOCK_REGISTRY, &no_lock_registry)) return true; | |
| if (get(mysql_h, MYSQL_NO_LOCK_REGISTRY, &no_lock_registry) != 0) return true; |
| @@ -176,76 +251,14 @@ DEFINE_BOOL_METHOD(mysql_command_services_imp::close, (MYSQL_H mysql_h)) { | |||
| auto mcs_ext = MYSQL_COMMAND_SERVICE_EXTN(mysql); | |||
| mysql_command_consumer_refs *consumer_refs = | |||
| (mysql_command_consumer_refs *)(mcs_ext->command_consumer_services); | |||
| bool no_lock_registry = false; | |||
| if (get(mysql_h, MYSQL_NO_LOCK_REGISTRY, &no_lock_registry)) return true; | |||
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| if (get(mysql_h, MYSQL_NO_LOCK_REGISTRY, &no_lock_registry)) return true; | |
| if (get(mysql_h, MYSQL_NO_LOCK_REGISTRY, &no_lock_registry)) return 1; |
| @@ -643,6 +660,8 @@ DEFINE_BOOL_METHOD(mysql_command_services_imp::set, | |||
| mysql_session = service->open(nullptr, nullptr); | |||
| else | |||
| return true; | |||
| if (mysql_session == nullptr) | |||
| return true; | |||
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return true; | |
| return 1; |
| @@ -693,6 +712,9 @@ DEFINE_BOOL_METHOD(mysql_command_services_imp::set, | |||
| mcs_ext->mcs_tcpip_port = *static_cast<const int *>(arg); | |||
| } | |||
| break; | |||
| case MYSQL_NO_LOCK_REGISTRY: | |||
| mcs_ext->no_lock_registry = static_cast<const bool *>(arg); | |||
There was a problem hiding this comment.
implicit conversion const bool * -> bool
| mcs_ext->no_lock_registry = static_cast<const bool *>(arg); | |
| mcs_ext->no_lock_registry = (static_cast<const bool *>(arg) != nullptr); |
| if (mysql_get_option(m_handle->mysql, | ||
| static_cast<enum mysql_option>(option), arg) != 0) | ||
| return true; | ||
| auto mcs_ext = MYSQL_COMMAND_SERVICE_EXTN(m_handle->mysql); |
There was a problem hiding this comment.
auto mcs_ext can be declared as auto *mcs_ext
| auto mcs_ext = MYSQL_COMMAND_SERVICE_EXTN(m_handle->mysql); | |
| auto *mcs_ext = MYSQL_COMMAND_SERVICE_EXTN(m_handle->mysql); |
| if (mysql_get_option(m_handle->mysql, | ||
| static_cast<enum mysql_option>(option), arg) != 0) | ||
| return true; | ||
| auto mcs_ext = MYSQL_COMMAND_SERVICE_EXTN(m_handle->mysql); |
There was a problem hiding this comment.
do not use C-style cast to convert between unrelated types
| if (mysql_get_option(m_handle->mysql, | ||
| static_cast<enum mysql_option>(option), arg) != 0) | ||
| return true; | ||
| auto mcs_ext = MYSQL_COMMAND_SERVICE_EXTN(m_handle->mysql); |
There was a problem hiding this comment.
do not use reinterpret_cast
| static_cast<enum mysql_option>(option), arg) != 0) | ||
| return true; | ||
| auto mcs_ext = MYSQL_COMMAND_SERVICE_EXTN(m_handle->mysql); | ||
| if (m_handle == nullptr || !arg) return true; |
There was a problem hiding this comment.
implicit conversion const void * -> bool
| if (m_handle == nullptr || !arg) return true; | |
| if (m_handle == nullptr || (arg == nullptr)) return true; |
| static_cast<enum mysql_option>(option), arg) != 0) | ||
| return true; | ||
| auto mcs_ext = MYSQL_COMMAND_SERVICE_EXTN(m_handle->mysql); | ||
| if (m_handle == nullptr || !arg) return true; |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| if (m_handle == nullptr || !arg) return true; | |
| if (m_handle == nullptr || !arg) return 1; |
| if (m_handle == nullptr || !arg) return true; | ||
| switch (option) { | ||
| case MYSQL_NO_LOCK_REGISTRY: | ||
| *(const_cast<bool *>(static_cast<const bool *>(arg))) = |
There was a problem hiding this comment.
do not use const_cast
| default: | ||
| if (mysql_get_option(m_handle->mysql, | ||
| static_cast<enum mysql_option>(option), arg) != 0) | ||
| return true; |
There was a problem hiding this comment.
implicit conversion bool -> mysql_service_status_t (aka int)
| return true; | |
| return 1; |
| @@ -32,10 +41,20 @@ | |||
|
|
|||
| #include <mysqld_error.h> | |||
There was a problem hiding this comment.
mysqld_error.h file not found
| @@ -174,10 +279,15 @@ BEGIN_COMPONENT_REQUIRES(CURRENT_COMPONENT_NAME) | |||
| REQUIRES_SERVICE(mysql_string_substr), | |||
| REQUIRES_SERVICE(mysql_string_compare), | |||
|
|
|||
| REQUIRES_PSI_THREAD_SERVICE, | |||
There was a problem hiding this comment.
do not use const_cast
| REQUIRES_SERVICE(mysql_command_query), | ||
| REQUIRES_SERVICE(mysql_command_query_result), | ||
| REQUIRES_SERVICE(mysql_command_field_info), |
There was a problem hiding this comment.
do not use const_cast
| REQUIRES_SERVICE(mysql_command_options), | ||
| REQUIRES_SERVICE(mysql_command_factory), | ||
| REQUIRES_SERVICE(mysql_command_error_info), |
There was a problem hiding this comment.
do not use const_cast
| REQUIRES_SERVICE(mysql_command_options), | ||
| REQUIRES_SERVICE(mysql_command_factory), | ||
| REQUIRES_SERVICE(mysql_command_error_info), | ||
| REQUIRES_SERVICE(mysql_command_thread), |
There was a problem hiding this comment.
do not use const_cast
| @@ -187,6 +297,8 @@ BEGIN_COMPONENT_REQUIRES(CURRENT_COMPONENT_NAME) | |||
| REQUIRES_SERVICE(mysql_current_thread_reader), | |||
| REQUIRES_SERVICE(mysql_thd_security_context), | |||
| REQUIRES_SERVICE(global_grants_check), | |||
| REQUIRES_SERVICE(component_sys_variable_register), | |||
There was a problem hiding this comment.
do not use const_cast
| @@ -187,6 +297,8 @@ BEGIN_COMPONENT_REQUIRES(CURRENT_COMPONENT_NAME) | |||
| REQUIRES_SERVICE(mysql_current_thread_reader), | |||
| REQUIRES_SERVICE(mysql_thd_security_context), | |||
| REQUIRES_SERVICE(global_grants_check), | |||
| REQUIRES_SERVICE(component_sys_variable_register), | |||
| REQUIRES_SERVICE(component_sys_variable_unregister), | |||
There was a problem hiding this comment.
do not use const_cast
| @@ -110,7 +122,9 @@ mysql_runtime_error_imp::emit END_SERVICE_IMPLEMENTATION(); | |||
|
|
|||
| BEGIN_COMPONENT_PROVIDES(mysql_minimal_chassis) | |||
| PROVIDES_SERVICE(mysql_minimal_chassis, registry), | |||
| PROVIDES_SERVICE(mysql_minimal_chassis_no_lock, registry), | |||
There was a problem hiding this comment.
do not use const_cast
| PROVIDES_SERVICE(mysql_minimal_chassis, registry_registration), | ||
| PROVIDES_SERVICE(mysql_minimal_chassis_no_lock, registry_registration), |
There was a problem hiding this comment.
do not use const_cast
|
|
||
| if (!sresult) { | ||
| return {std::string{cs_term.get_buffer()}}; | ||
| return cs_term_escaped; |
There was a problem hiding this comment.
So if term is not present in the dictionary our UDF returns its escaped version? And if it is present, we return some non-escaped random replacement term from the dictionary? This difference escaped - non-escaped sounds suspicious to me. Is this intentional change or a bug?
|
|
||
| namespace masking_functions { | ||
|
|
||
| class basic_sql_context_builder { |
There was a problem hiding this comment.
It took me some time to figure out class hierarchy for basic_sql_context_builder and its descendants. Perhaps it would be easier if use more standard "abstract_sql_context_builder" for this class, and perhaps add comments describing this class as well as its descendants? What do you think?
| throw std::invalid_argument{ | ||
| "Wrong argument list: masking_dictionaries_flush()"}; | ||
|
|
||
| ctx.mark_result_nullable(true); |
There was a problem hiding this comment.
I think it is worth to add a comment that will explain why this function is nullable/in which cases it can return NULL.
| @@ -960,41 +958,35 @@ class gen_blocklist_impl { | |||
| if (ctx.is_arg_null(0)) return std::nullopt; | |||
|
|
|||
| const auto cs_term = make_charset_string_from_arg(ctx, 0); | |||
| const auto cs_dict_a = make_charset_string_from_arg(ctx, 1); | |||
| const auto cs_dict_b = make_charset_string_from_arg(ctx, 2); | |||
| const auto cs_term_escaped = escape_string(cs_term); | |||
There was a problem hiding this comment.
After a bit more thinking. IMO it is conceptually wrong to do escaping of dictionary and term names on this level (i.e. in UDF implementation). It is something which belongs to the level interacting with SQL. For example, if we will decide to change the implementation and start using prepared statements on the level below, such escaping on a high-level will become unnecessary and will only harm. So I'd like to propose move escaping back to query_builder.cpp layer.
What do you think?
https://perconadev.atlassian.net/browse/PS-9148