Cleanup system schema checks

- Be more consistent about how we look for them

Author: villagestevers

sql/dd/impl/dictionary_impl.cc

@@ -77,6 +77,7 @@
 #include "sql/transaction.h"                    // trans_commit()
 #include "storage/perfschema/pfs_dd_version.h"  // PFS_DD_VERSION
 #include "villagesql/schema/schema_manager.h"
+#include "villagesql/schema/util.h"
 
 extern Cost_constant_cache *cost_constant_cache;  // defined in
                                                   // opt_costconstantcache.cc
@@ -331,8 +332,7 @@ bool Dictionary_impl::is_dd_table_access_allowed(bool is_dd_internal_thread,
                                                  size_t schema_length,
                                                  const char *table_name) const {
   // Lockdown access to villagesql.* tables.
-  if (strcmp(schema_name, villagesql::SchemaManager::VILLAGESQL_SCHEMA_NAME) ==
-      0) {
+  if (villagesql::is_villagesql_schema(schema_name)) {
     // Allow access for internal threads or if running a debug build and the
     // appropriate session variable is set.
     return is_dd_internal_thread ||

sql/table.cc

@@ -132,7 +132,7 @@
 #include "strxnmov.h"
 #include "template_utils.h"  // down_cast
 #include "thr_mutex.h"
-#include "villagesql/schema/schema_manager.h"
+#include "villagesql/schema/util.h"
 
 /* INFORMATION_SCHEMA name */
 LEX_CSTRING INFORMATION_SCHEMA_NAME = {STRING_WITH_LEN("information_schema")};
@@ -360,9 +360,7 @@ TABLE_CATEGORY get_table_category(const LEX_CSTRING &db,
   }
 
   // Check for VillageSQL system tables
-  if (my_strcasecmp(system_charset_info,
-                    villagesql::SchemaManager::VILLAGESQL_SCHEMA_NAME,
-                    db.str) == 0) {
+  if (villagesql::is_villagesql_schema(db.str)) {
     return TABLE_CATEGORY_VILLAGESQL_SYSTEM;
   }
 

storage/innobase/handler/ha_innodb.cc

@@ -4016,9 +4016,8 @@ static void innobase_dict_cache_reset_tables_and_tablespaces() {
 
     /* TODO: Remove follow if we have better way to identify
     DD "system table" */
-    if (db_str.compare("mysql") == 0 || table->is_dd_table ||
-        table->is_corrupted() ||
-        villagesql::is_villagesql_system_table(db_str.c_str())) {
+    if (villagesql::is_system_schema(db_str.c_str()) || table->is_dd_table ||
+        table->is_corrupted()) {
       continue;
     }
 

villagesql/schema/ADDING_SYSTEM_TABLES.md

@@ -189,7 +189,7 @@ constexpr const char *YOUR_TABLE_NAME = "your_table";
 - **Wrong initializer order** - Must match member declaration order
 - **Missing update_key()** - Must call in read_from_table() after deserializing fields
 - **Using public key fields** - Keep key components private with accessors to prevent stale keys
-- **Hardcoded table names** - Use `SchemaManager::VILLAGESQL_SCHEMA_NAME` and `SchemaManager::YOUR_TABLE_NAME` instead of string literals in Table_ref and error messages
+- **Hardcoded table names** - Use `SchemaManager::VILLAGESQL_SCHEMA_NAME` and `SchemaManager::YOUR_TABLE_NAME` instead of string literals in Table_ref and error messages. To check for system tables, use villagesql::is_system_schema() or villagesql::is_villagesql_schema().
 
 ## Usage in Code
 

villagesql/schema/util.cc

@@ -24,7 +24,7 @@ namespace villagesql {
 // that use the constants from SchemaManager to make sure they use this.
 bool is_villagesql_system_table(const TABLE *table) {
   if (!table || !table->s) return false;
-  return is_villagesql_system_table(table->s->db.str);
+  return is_villagesql_schema(table->s->db.str);
 }
 
 }  // namespace villagesql

villagesql/schema/util.h

@@ -26,11 +26,18 @@ namespace villagesql {
 
 // Check if a database name is the 'villagesql' schema. db_name must not be
 // nullptr.
-inline bool is_villagesql_system_table(const char *db_name) {
+inline bool is_villagesql_schema(const char *db_name) {
   return (my_strcasecmp(system_charset_info,
                         SchemaManager::VILLAGESQL_SCHEMA_NAME, db_name) == 0);
 }
 
+// Like the above but also considered true if this is in a mysql system schema.
+inline bool is_system_schema(const char *db_name) {
+  return (my_strcasecmp(system_charset_info, "mysql", db_name) == 0 ||
+          my_strcasecmp(system_charset_info, "sys", db_name) == 0 ||
+          is_villagesql_schema(db_name));
+}
+
 // Check if a TABLE is in the 'villagesql' schema.
 // All tables in the villagesql schema are system tables that can be accessed
 // with no_read_locking from INFORMATION_SCHEMA views.

villagesql/sql/metadata_modifier.cc

@@ -34,6 +34,7 @@
 #include "villagesql/schema/descriptor/type_descriptor.h"
 #include "villagesql/schema/schema_manager.h"
 #include "villagesql/schema/systable/extensions.h"
+#include "villagesql/schema/util.h"
 #include "villagesql/schema/victionary_client.h"
 #include "villagesql/sql/custom_vdf.h"
 #include "villagesql/sql/func_lookup.h"
@@ -720,7 +721,9 @@ bool Metadata_modifier::store(THD *thd) {
     // just need to find it in query_tables and lock it.
     Table_ref *columns_table = nullptr;
     for (Table_ref *tl = thd->lex->query_tables; tl; tl = tl->next_global) {
-      if (strcmp(tl->db, SchemaManager::VILLAGESQL_SCHEMA_NAME) == 0 &&
+      // TODO(villagesql-beta): table names should be using system charset
+      // comparison
+      if (villagesql::is_villagesql_schema(tl->db) &&
           strcmp(tl->table_name, SchemaManager::COLUMNS_TABLE_NAME) == 0) {
         columns_table = tl;
         break;

villagesql/types/util.cc

@@ -45,8 +45,8 @@
 #include "villagesql/include/error.h"
 #include "villagesql/schema/descriptor/type_context.h"
 #include "villagesql/schema/descriptor/type_descriptor.h"
-#include "villagesql/schema/schema_manager.h"
 #include "villagesql/schema/systable/custom_columns.h"
+#include "villagesql/schema/util.h"
 #include "villagesql/schema/victionary_client.h"
 
 namespace villagesql {
@@ -70,7 +70,7 @@ bool MaybeInjectCustomType(THD *thd, TABLE_SHARE &share, Field *field) {
   std::string db_name = std::string(share.db.str, share.db.length);
 
   // Skip special databases
-  if (db_name == "mysql" || db_name == SchemaManager::VILLAGESQL_SCHEMA_NAME) {
+  if (::villagesql::is_system_schema(db_name.c_str())) {
     return false;
   }