commit cb59a4a11e33ed829abecb26fb4b9aab910a624a Author: Rex Johnston Date: Mon Oct 12 13:12:25 2026 +1300 MDEV-41228 Rename shallow_copy to surface_copy and check its type AMEDNDED TO REDUCE SUPID RENAMES. Item::shallow_copy() was meant to create a copy of the same class as the item it is called on, but nothing enforced that. A class that does not declare its own override inherits the one of its nearest concrete parent, and get_item_copy() then silently creates an object of the parent class, losing the child's virtual behaviour (see MDEV-38801 for an earlier instance with Item_cache_year). Rename shallow_copy()/shallow_copy_with_checks() to surface_copy()/ surface_copy_with_checks(). "Shallow" said little about what such a copy is for; the rename prepares for copy methods named after their purpose. Add the ITEM_SURFACE_COPY(CLASS) macro, which declares the override, and Item::dbug_check_surface_copy_type(), a debug-only check called from Query_arena::free_items() for every item freed. Once per dynamic type it creates a surface copy, unlinks it from thd->free_list again (the copy shares pointers with the original and must not be destroyed) and asserts that the copy has the same typeid as the original. The check found these classes copied as their parent class, now fixed with ITEM_SURFACE_COPY(): Item_join_operator_plus, Item_null_result, Item_old_field, Item_datetime, Item_empty_string, Item_return_int, Item_trigger_type_of_statement, in_string::Item_string_for_in_vector, Item_is_not_null_test, Item_interval_DDhhmmssff_typecast, Item_string_xml_non_const, Item_func_trt_trx_sees_eq, Item_field_refcursor (type_cursor plugin), Item_field_packable_scalar (type_assoc_array plugin). Item_field_packable_row::surface_copy() asserted to mark the class as never copied, which the check trips over since it copies every type it meets. It now returns nullptr, the existing way for an item to refuse a copy. No test is added: the check runs under every existing test in a debug build. main and the SQL-layer suites (compat/*, json, funcs_*, vcol, gcol, versioning, period, sys_vars, type_*) pass with it in both normal and --ps-protocol mode. This commit was prepared with Claude Code (Opus 5), which wrote the runtime type check, ran the main and SQL-layer suites in normal and --ps-protocol mode to find the classes that inherited a parent's copy method, added the ITEM_SURFACE_COPY overrides for them, changed Item_field_packable_row to return nullptr, and performed the mechanical rename. diff --git a/plugin/func_test/plugin.cc b/plugin/func_test/plugin.cc index 7c93b47e8e5..f946ceb9ede 100644 --- a/plugin/func_test/plugin.cc +++ b/plugin/func_test/plugin.cc @@ -42,7 +42,7 @@ class Item_func_sysconst_test :public Item_func_sysconst } const Lex_ident_routine fully_qualified_func_name() const override { return Lex_ident_routine("sysconst_test()"_LEX_CSTRING); } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; @@ -136,7 +136,7 @@ class Item_func_strnxfrm_source_length_used: public Item_longlong_func static LEX_CSTRING name= "strnxfrm_source_length_used"_LEX_CSTRING; return name; } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } @@ -189,7 +189,7 @@ class Item_func_strnxfrm_warnings: public Item_long_func static LEX_CSTRING name= "strnxfrm_warnings"_LEX_CSTRING; return name; } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } @@ -245,7 +245,7 @@ class Item_func_strnxfrm: public Item_str_func static LEX_CSTRING name= "strnxfrm"_LEX_CSTRING; return name; } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } diff --git a/plugin/type_assoc_array/sql_type_assoc_array.cc b/plugin/type_assoc_array/sql_type_assoc_array.cc index 8b144e0fd1a..2134480cfd7 100644 --- a/plugin/type_assoc_array/sql_type_assoc_array.cc +++ b/plugin/type_assoc_array/sql_type_assoc_array.cc @@ -244,6 +244,8 @@ class Item_field_packable_scalar: public Item_field, unpack(); return Item_field::save_in_field(field, no_conversions); } +protected: + ITEM_SURFACE_COPY(Item_field_packable_scalar) }; @@ -280,11 +282,7 @@ class Item_field_packable_row: public Item_field_row, return false; } - Item *shallow_copy(THD *thd) const override - { - DBUG_ASSERT(0); - return get_item_copy(thd, this); - } + Item *surface_copy(THD *thd) const override { return nullptr; } static uint packed_col_length(Field *field) { @@ -675,7 +673,7 @@ class Item_func_assoc_array_first :public Item_handled_method set_func_handler(&ha_str_key); return m_func_handler->fix_length_and_dec(this); } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; @@ -695,7 +693,7 @@ class Item_func_assoc_array_last :public Item_handled_method return name; } bool fix_length_and_dec(THD *thd) override; - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; @@ -715,7 +713,7 @@ class Item_func_assoc_array_next :public Item_handled_method return name; } bool fix_length_and_dec(THD *thd) override; - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; @@ -735,7 +733,7 @@ class Item_func_assoc_array_prior :public Item_handled_method return name; } bool fix_length_and_dec(THD *thd) override; - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; @@ -772,7 +770,7 @@ class Item_func_assoc_array_count :public Item_long_method { return mark_unsupported_function(func_name(), "()", arg, VCOL_IMPOSSIBLE); } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; @@ -814,7 +812,7 @@ class Item_func_assoc_array_exists :public Item_bool_method { return mark_unsupported_function(func_name(), "()", arg, VCOL_IMPOSSIBLE); } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; @@ -865,7 +863,7 @@ class Item_func_assoc_array_delete :public Item_bool_method { return mark_unsupported_function(func_name(), "()", arg, VCOL_IMPOSSIBLE); } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; @@ -1796,7 +1794,7 @@ Item *Item_assoc_array::deep_copy(THD *thd) const return 0; copy_args[i]= arg_clone; } - Item_assoc_array *copy= (Item_assoc_array *) shallow_copy_with_checks(thd); + Item_assoc_array *copy= (Item_assoc_array *) surface_copy_with_checks(thd); if (unlikely(!copy)) return 0; copy->args= copy_args; diff --git a/plugin/type_assoc_array/sql_type_assoc_array.h b/plugin/type_assoc_array/sql_type_assoc_array.h index 783096bb91b..6ebfbb7cecf 100644 --- a/plugin/type_assoc_array/sql_type_assoc_array.h +++ b/plugin/type_assoc_array/sql_type_assoc_array.h @@ -344,7 +344,7 @@ class Item_field_assoc_array: public Item_field, public Item_composite_base :Item_field(thd, field) { } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } const Type_handler *type_handler() const override @@ -417,7 +417,7 @@ class Item_assoc_array: public Item_composite void bring_value() override; void print(String *str, enum_query_type query_type) override; - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } Item *deep_copy(THD *thd) const override; }; @@ -464,7 +464,7 @@ class Item_splocal_assoc_array_element :public Item_splocal, bool append_for_log(THD *thd, String *str) override; void print(String *str, enum_query_type query_type) override; - Item *shallow_copy(THD *) const override { return nullptr; } + Item *surface_copy(THD *) const override { return nullptr; } Item *deep_copy(THD *thd) const override { return nullptr; } Item_composite_base *get_composite_variable(sp_rcontext *ctx) const; diff --git a/plugin/type_cursor/plugin.cc b/plugin/type_cursor/plugin.cc index 697b0f8abe1..d77753ed042 100644 --- a/plugin/type_cursor/plugin.cc +++ b/plugin/type_cursor/plugin.cc @@ -367,6 +367,8 @@ class Item_field_refcursor final: public Item_field DBUG_ASSERT(fc); return fc->resolve_spvar_cursor_rowtype(thd, cursor); } +protected: + ITEM_SURFACE_COPY(Item_field_refcursor) }; @@ -1218,7 +1220,7 @@ class Item_func_cursor_ref_count :public Item_long_func, const ULonglong_null count= cursor_array(thd)->ref_count(offset); return (null_value= count.is_null()) ? 0LL : (longlong) count.value(); } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; diff --git a/plugin/type_inet/item_inetfunc.h b/plugin/type_inet/item_inetfunc.h index 4fccc30c785..de3e0510c47 100644 --- a/plugin/type_inet/item_inetfunc.h +++ b/plugin/type_inet/item_inetfunc.h @@ -45,7 +45,7 @@ class Item_func_inet_aton : public Item_longlong_func unsigned_flag= 1; return FALSE; } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; @@ -72,7 +72,7 @@ class Item_func_inet_ntoa : public Item_str_func set_maybe_null(); return FALSE; } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; @@ -118,7 +118,7 @@ class Item_func_inet6_aton : public Item_str_func set_maybe_null(); return FALSE; } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } String *val_str(String *to) override; @@ -156,7 +156,7 @@ class Item_func_inet6_ntoa : public Item_str_ascii_func return FALSE; } String *val_str_ascii(String *to) override; - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; @@ -178,7 +178,7 @@ class Item_func_is_ipv4 : public Item_func_inet_bool_base static LEX_CSTRING name= {STRING_WITH_LEN("is_ipv4") }; return name; } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } bool val_bool() override; @@ -201,7 +201,7 @@ class Item_func_is_ipv6 : public Item_func_inet_bool_base static LEX_CSTRING name= {STRING_WITH_LEN("is_ipv6") }; return name; } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } bool val_bool() override; @@ -223,7 +223,7 @@ class Item_func_is_ipv4_compat : public Item_func_inet_bool_base static LEX_CSTRING name= {STRING_WITH_LEN("is_ipv4_compat") }; return name; } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } bool val_bool() override; }; @@ -244,7 +244,7 @@ class Item_func_is_ipv4_mapped : public Item_func_inet_bool_base static LEX_CSTRING name= {STRING_WITH_LEN("is_ipv4_mapped") }; return name; } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } bool val_bool() override; }; diff --git a/plugin/type_uuid/item_uuidfunc.h b/plugin/type_uuid/item_uuidfunc.h index b62ccf73fce..e17a9427cf4 100644 --- a/plugin/type_uuid/item_uuidfunc.h +++ b/plugin/type_uuid/item_uuidfunc.h @@ -47,7 +47,7 @@ class Item_func_sys_guid: public Item_str_func { return mark_unsupported_function(func_name(), "()", arg, VCOL_NON_DETERMINISTIC); } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; @@ -85,7 +85,7 @@ class Item_func_uuid: public Item_func_uuid_vx static LEX_CSTRING name= {STRING_WITH_LEN("uuid") }; return name; } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; @@ -99,7 +99,7 @@ class Item_func_uuid_v4: public Item_func_uuid_vx static LEX_CSTRING name= {STRING_WITH_LEN("uuid_v4") }; return name; } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; @@ -112,7 +112,7 @@ class Item_func_uuid_v7: public Item_func_uuid_vx static LEX_CSTRING name= {STRING_WITH_LEN("uuid_v7") }; return name; } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } }; #endif // ITEM_UUIDFUNC_INCLUDED diff --git a/plugin/type_xmltype/sql_type_xmltype.h b/plugin/type_xmltype/sql_type_xmltype.h index 102fe10a704..e4fdee08e09 100644 --- a/plugin/type_xmltype/sql_type_xmltype.h +++ b/plugin/type_xmltype/sql_type_xmltype.h @@ -164,7 +164,7 @@ class Item_xmltype_typecast :public Item_char_typecast } bool fix_length_and_dec(THD *thd) override; String *val_str(String *to) override; - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } void print(String *str, enum_query_type query_type) override; diff --git a/sql/item.cc b/sql/item.cc index d087cfc2a96..79165a5fbb0 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -12303,3 +12303,37 @@ bool ignored_list_includes_table(ignored_tables_list_t list, TABLE_LIST *tbl) } return false; } + + +#ifndef DBUG_OFF +#include +#include +#include + +void Item::dbug_check_surface_copy_type(THD *thd) const +{ + static std::unordered_set checked; + static std::mutex checked_mutex; + { + std::lock_guard lock(checked_mutex); + if (!checked.insert(typeid(*this)).second) + return; + } + Item *copy= shallow_copy(thd); + if (!copy) + return; + /* + get_item_copy() registered the copy in thd->free_list. Unlink it: it is + only inspected, and destroying a copy that shares pointers with the + original could free them twice. + */ + if (thd->free_list == copy) + thd->free_list= copy->next; + if (typeid(*copy) != typeid(*this)) + { + fprintf(stderr, "SURFACE_COPY_TYPE_MISMATCH: %s copied as %s\n", + typeid(*this).name(), typeid(*copy).name()); + DBUG_ASSERT(0); + } +} +#endif diff --git a/sql/item.h b/sql/item.h index 384f6db4164..d751a5497e1 100644 --- a/sql/item.h +++ b/sql/item.h @@ -1923,7 +1923,7 @@ class Item :public Value_source, } /* - Create a shallow copy of the item (usually invoking copy constructor). + Create a surface copy of the item (usually invoking copy constructor). For deep copying see deep_copy_with_checks(). Return value: @@ -1979,6 +1979,12 @@ class Item :public Value_source, bool check_deep_copy(THD *thd, Item *clone, const char *what); /* walk() processor used by check_deep_copy() */ bool deep_copy_check_processor(void *arg); + /* + Check, once per dynamic type, that shallow_copy() creates an object of + the same class as this item, i.e. that the class did not inherit + shallow_copy() from a concrete parent. + */ + void dbug_check_surface_copy_type(THD *thd) const; #endif /* @@ -2899,7 +2905,7 @@ class Item :public Value_source, /* Service function for public method shallow_copy_with_checks(). See comments for shallow_copy_with_checks() above. Override this method - in derived classes to create shallow copies of the item + in derived classes to create surface copies of the item */ virtual Item *shallow_copy(THD *thd) const = 0; @@ -2922,6 +2928,15 @@ inline T* get_item_copy (THD *thd, const T* item) return copy; } +/* + Every instantiable Item class must declare its own shallow_copy(): one + inherited from a concrete parent creates an object of the parent class. + Item::dbug_check_surface_copy_type() asserts that in debug builds. +*/ +#define ITEM_SURFACE_COPY(CLASS) \ + Item *shallow_copy(THD *thd) const override \ + { return get_item_copy(thd, this); } + #ifndef DBUG_OFF /** @@ -4223,6 +4238,8 @@ class Item_old_field: public Item_field private: uchar *saved_row_ref; +protected: + ITEM_SURFACE_COPY(Item_old_field) }; /** @@ -4339,6 +4356,8 @@ class Item_join_operator_plus: public Item_null str->append("(+)"_LEX_CSTRING); } static List *make_as_item_list(THD *thd); +protected: + ITEM_SURFACE_COPY(Item_join_operator_plus) }; @@ -4369,6 +4388,8 @@ class Item_null_result :public Item_null { return mark_unsupported_function(full_name(), arg, VCOL_IMPOSSIBLE); } +protected: + ITEM_SURFACE_COPY(Item_null_result) }; /* @@ -5054,6 +5075,8 @@ class Item_datetime :public Item_int return false; } void print(String *str, enum_query_type query_type) override; +protected: + ITEM_SURFACE_COPY(Item_datetime) }; @@ -5465,6 +5488,8 @@ class Item_empty_string :public Item_partition_func_safe_string length * cs->mbmaxlen, cs) { } void make_send_field(THD *thd, Send_field *field) override; +protected: + ITEM_SURFACE_COPY(Item_empty_string) }; @@ -5484,6 +5509,8 @@ class Item_return_int :public Item_int Type_handler::get_handler_by_field_type(int_field_type); return unsigned_flag ? h->type_handler_unsigned() : h; } +protected: + ITEM_SURFACE_COPY(Item_return_int) }; @@ -7989,6 +8016,8 @@ class Item_trigger_type_of_statement : public Item_int private: THD *m_thd; active_dml_stmt m_trigger_stmt_type; +protected: + ITEM_SURFACE_COPY(Item_trigger_type_of_statement) }; diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h index cf3418401f0..1baef14ae6b 100644 --- a/sql/item_cmpfunc.h +++ b/sql/item_cmpfunc.h @@ -1699,6 +1699,8 @@ class in_string :public in_vector str_value= *str; collation.set(str->charset()); } + protected: + ITEM_SURFACE_COPY(Item_string_for_in_vector) }; public: in_string(THD *thd, uint elements, qsort_cmp2 cmp_func, CHARSET_INFO *cs); @@ -3005,6 +3007,8 @@ class Item_is_not_null_test :public Item_func_isnull table_map used_tables() const override { return used_tables_cache | RAND_TABLE_BIT; } bool const_item() const override { return FALSE; } +protected: + ITEM_SURFACE_COPY(Item_is_not_null_test) }; diff --git a/sql/item_timefunc.h b/sql/item_timefunc.h index 49b082d3421..321b58f2140 100644 --- a/sql/item_timefunc.h +++ b/sql/item_timefunc.h @@ -1505,6 +1505,8 @@ class Item_interval_DDhhmmssff_typecast :public Item_char_typecast null_value= !it.is_valid_interval_DDhhmmssff(); return it.to_string(to, m_fsp); } +protected: + ITEM_SURFACE_COPY(Item_interval_DDhhmmssff_typecast) }; diff --git a/sql/item_vers.h b/sql/item_vers.h index ca77e8761c5..cef272a0e43 100644 --- a/sql/item_vers.h +++ b/sql/item_vers.h @@ -148,6 +148,8 @@ class Item_func_trt_trx_sees_eq : static LEX_CSTRING name= {STRING_WITH_LEN("trt_trx_sees_eq") }; return name; } +protected: + ITEM_SURFACE_COPY(Item_func_trt_trx_sees_eq) }; #endif /* ITEM_VERS_INCLUDED */ diff --git a/sql/item_xmlfunc.cc b/sql/item_xmlfunc.cc index e78796c684e..351cd49e2d5 100644 --- a/sql/item_xmlfunc.cc +++ b/sql/item_xmlfunc.cc @@ -614,6 +614,8 @@ class Item_string_xml_non_const: public Item_string */ return this; } +protected: + ITEM_SURFACE_COPY(Item_string_xml_non_const) }; diff --git a/sql/sql_class.cc b/sql/sql_class.cc index b3536848475..155ff6f9a87 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -4350,6 +4350,11 @@ void Query_arena::free_items() { Item *next; DBUG_ENTER("Query_arena::free_items"); +#ifndef DBUG_OFF + if (THD *thd= current_thd) + for (Item *item= free_list; item; item= item->next) + item->dbug_check_surface_copy_type(thd); +#endif /* This works because items are allocated on THD::mem_root */ for (; free_list; free_list= next) { diff --git a/storage/duckdb/duckdb_udf.cc b/storage/duckdb/duckdb_udf.cc index 84714538ab0..5b7b4a0c51c 100644 --- a/storage/duckdb/duckdb_udf.cc +++ b/storage/duckdb/duckdb_udf.cc @@ -110,7 +110,7 @@ class Item_func_run_in_duckdb : public Item_str_func return str; } - Item *shallow_copy(THD *thd) const override + Item *surface_copy(THD *thd) const override { return get_item_copy(thd, this); } };