[POC] Prototyping - MySQL User Defined Types - #707
Conversation
Can now add complex numbers.
Run untrusted pull request builds with restricted permissions against validated revisions, and publish statuses and labels only from trusted workflows that revalidate the repository, workflow run, PR head, and ordering. Replace the custom review client with the pinned OpenAI Codex Action, bound its input to a validated PR diff, pin third-party actions, and add dependency maintenance for GitHub Actions. Retry a failed or empty Codex review once after a delay with a configurable fallback model while preserving the same read-only isolation boundary and structured output contract. Publish structured Codex findings as one commit-bound GitHub review. Validate each file and right-side line range against the current diff, keep unanchored findings in the summary, prevent duplicate reviews, and revalidate both reviewed revisions before posting. Warm trusted Boost and ccache entries, align the MTR compiler cache with the GCC build, shard MTR suites across runners, run tests in parallel with bounded retries, and retain diagnostics. Safely reset head-scoped CI state, standardize labels, and remove the obsolete OCA checkbox. Require both the OCA Verified label and a current trusted approval before adding Integrate. Revalidate both conditions around label publication and remove Integrate if either condition no longer holds. Temporarily disable parallel-run failures tracked by Bug#39882117 and restore the required restart and expected output for the buffer-pool-load MTR. Change-Id: I7393e75cab3afa172a99237337c26e3974f955fa
There was a problem hiding this comment.
Codex PR review
Overall: patch is incorrect (99% confidence)
The change introduces server-crash paths, stale callbacks after component unload, missing authorization and replication handling, unbounded runtime leaks, and discards the type definition itself.
7 finding(s) were posted inline.
| return true; | ||
| } | ||
|
|
||
| if (dd::create_udt_type(thd, *existing_schema, type_name)) { |
There was a problem hiding this comment.
P1: [P1] Enforce privileges before storing the type
This is the point where persistent dictionary state is created, but execute() performs no access check first. Any authenticated user able to issue the statement can therefore create types in schemas where they have no DDL privileges. Check the appropriate schema-level privilege before acquiring locks or modifying the dictionary.
Confidence: 99%
| return true; | ||
| } | ||
|
|
||
| if (trans_commit_stmt(thd) || trans_commit(thd)) { |
There was a problem hiding this comment.
P1: [P1] Binlog CREATE TYPE before committing
The new dictionary object is committed without writing the statement to the binary log. On replicated or point-in-time-recovered systems, the type will exist only on the originating server and later routines referring to it will fail elsewhere. Binlog the DDL before committing and roll back both statement and transaction if logging or commit fails.
Confidence: 98%
| #ifdef WITH_EXPERIMENTAL_UDT | ||
| WARN_NOT_IMPLEMENTED(thd, "Sql_cmd_create_type::execute()"); |
There was a problem hiding this comment.
P1: [P1] Check CREATE TYPE privileges before modifying the dictionary
The command locks and stores a dictionary object without any authorization check. Consequently, a user able to issue SQL can create types in schemas where they lack CREATE/appropriate DDL privileges. Perform the schema-level privilege check before acquiring locks or writing the DD object.
Confidence: 94%
There was a problem hiding this comment.
Codex PR review
Overall: patch is incorrect (99% confidence)
The change has authorization and replication gaps, unsafe component-unload behavior, and several defects that prevent declared and returned UDT values from retaining their intended types.
6 finding(s) were posted inline.
| WARN_NOT_IMPLEMENTED(thd, "Sql_cmd_create_type::execute()"); | ||
|
|
||
| if (m_type_ident->db.length == 0) { |
There was a problem hiding this comment.
P1: [P1] Enforce privileges before creating a type
This execution path never performs an access check before writing directly to the data dictionary. Consequently, a user without an appropriate CREATE privilege can create types in any existing schema. Check the relevant schema-level privilege before acquiring locks or storing the object.
Confidence: 99%
| return true; | ||
| } | ||
|
|
||
| if (trans_commit_stmt(thd) || trans_commit(thd)) { |
There was a problem hiding this comment.
P1: [P1] Write CREATE TYPE to the binary log
The new dictionary object is committed without first logging the CREATE TYPE statement. On a binlogged server, replicas therefore never create the type and subsequently fail to resolve routines or declarations that reference it. Binlog the statement before committing and roll back the dictionary transaction if logging fails.
Confidence: 99%
| // to->m_length = from->length; | ||
| // to->m_dec = from->decimals; |
There was a problem hiding this comment.
P2: [P2] Apply component descriptor lengths to result fields
The conversion drops from->length and from->decimals, leaving the TypeDescriptor defaults. In particular, a registered VARCHAR return type creates a zero-width result field, so storing a nonempty component result truncates it; other size- or precision-sensitive types are likewise misrepresented. Convert these numeric descriptor values into the representation expected by Create_field::init, or make TypeDescriptor retain them numerically.
Confidence: 96%
| evaluate_to_field(field); | ||
|
|
||
| // FIXME | ||
| return TYPE_ERR_BAD_VALUE; |
There was a problem hiding this comment.
P2: [P2] Return the actual save result
Even when evaluate_to_field succeeds, this method always reports TYPE_ERR_BAD_VALUE. Callers that honor conversion status, such as key materialization paths, consequently treat valid UDT function values as fatal conversion failures. Propagate evaluation failure appropriately and return TYPE_OK after a successful save.
Confidence: 95%
|
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA). To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
There was a problem hiding this comment.
Codex PR review
Overall: patch is incorrect (99% confidence)
The new UDT implementation permits unauthorized, unreplicated DDL and contains fundamental type-resolution and component-lifetime defects that can produce incorrect schemas or server crashes.
6 finding(s) were posted inline.
| bool Sql_cmd_create_type::execute(THD *thd) { | ||
| bool rc; | ||
|
|
||
| #ifdef WITH_EXPERIMENTAL_UDT | ||
| WARN_NOT_IMPLEMENTED(thd, "Sql_cmd_create_type::execute()"); |
There was a problem hiding this comment.
P1: [P1] Enforce authorization before creating a type
This execution path writes directly to the data dictionary without checking any global or schema privilege. Consequently, an authenticated user able to issue the statement can create a type in any existing schema. Check the appropriate CREATE privilege before acquiring locks or storing the object.
Confidence: 96%
| if (dd::create_udt_type(thd, *existing_schema, type_name)) { | ||
| return true; | ||
| } | ||
|
|
||
| if (trans_commit_stmt(thd) || trans_commit(thd)) { |
There was a problem hiding this comment.
P1: [P1] Write CREATE TYPE to the binary log
The successful DDL path commits the dictionary transaction without calling thd->binlog_query. On a binlogged server, replicas therefore never create the type and subsequently diverge or reject statements that reference it. Log the statement before committing and roll back if logging fails.
Confidence: 98%
| explicit PT_user_defined_type(const POS &pos, Type_ident *ident) | ||
| : PT_type(pos, MYSQL_TYPE_INVALID), type_ident(ident) {} | ||
|
|
||
| const Type_ident *get_type_ident() const override { return type_ident; } |
There was a problem hiding this comment.
P1: [P1] Resolve UDTs used in ordinary column definitions
This node leaves its native type as MYSQL_TYPE_INVALID, while the existing PT_field_def path reads only that native type and never consumes get_type_ident(). Thus the newly accepted CREATE TABLE ... (c schema.type) syntax passes an invalid type into Alter_info::add_field rather than resolving the UDT. Carry the identifier through field contextualization and invoke resolve_type_descriptor before constructing the Create_field.
Confidence: 97%
| evaluate_to_field(field); | ||
|
|
||
| // FIXME | ||
| return TYPE_ERR_BAD_VALUE; |
There was a problem hiding this comment.
P2: [P2] Return success after saving a successful UDT value
This method ignores whether evaluation succeeded and always returns TYPE_ERR_BAD_VALUE. Callers using the normal save_in_field path therefore treat successfully stored UDT-function results as conversion failures. Return TYPE_OK after successful evaluation and an error status only when evaluate_to_field fails.
Confidence: 98%
| String *data = m_item->val_str(&m_string_data); | ||
| *val = reinterpret_cast<unsigned char *>(data->ptr()); |
There was a problem hiding this comment.
P1: [P1] Handle NULL results before dereferencing val_str
val_str() legitimately returns nullptr for SQL NULL, but get_blob() immediately dereferences it. For example, nesting a NULL-producing UDT function as an argument makes Item_udt_func::is_null() initially report false, then this path crashes when evaluating the value. Check the returned pointer and propagate NULL through the value API; get_null() must also evaluate the current item reliably.
Confidence: 97%
| evaluate_to_field(field); | ||
|
|
||
| // FIXME | ||
| return TYPE_ERR_BAD_VALUE; |
There was a problem hiding this comment.
P1: [P1] Return success after saving a valid UDT value
The function result is written to the destination field, but this method unconditionally returns TYPE_ERR_BAD_VALUE and even ignores evaluation failure. Any execution path using save_in_field() can therefore reject or warn about every valid UDT result. Return TYPE_OK when evaluate_to_field() succeeds and an appropriate error status when it fails.
Confidence: 99%
|
|
||
| field_def: | ||
| type opt_column_attribute_list | ||
| broken_type /* FIXME: opt_collate */ opt_column_attribute_list |
There was a problem hiding this comment.
P2: [P2] Resolve UDTs used in table column definitions
Changing field_def to accept broken_type exposes user-defined types in CREATE TABLE, but PT_user_defined_type supplies MYSQL_TYPE_INVALID and the existing field contextualization neither retains get_type_ident() nor calls resolve_type_descriptor(). Thus accepted syntax such as CREATE TABLE t(c test.usbn13) reaches field creation with an invalid type. Propagate and resolve the type identifier for columns, or do not add UDTs to this grammar path yet.
Confidence: 94%
| if (m_type_ident->db.length == 0) { | ||
| m_type_ident->db = thd->db(); |
There was a problem hiding this comment.
P1: [P1] Enforce privileges before creating a type
This execution path performs no access check before storing an object in the target schema, allowing an authenticated user without CREATE privileges to create types. Check the appropriate schema-level privilege (for example, CREATE_ACL) before acquiring locks or modifying the dictionary.
Confidence: 99%
| // FIXME: lifecycle, may be in use | ||
| udt_function_hash->erase(key); |
There was a problem hiding this comment.
P1: [P1] Keep registered functions alive while statements reference them
Unregistration erases the function even when ref_count is nonzero. Prepared statements and cached stored-program item trees retain pointers to the component-owned descriptor and implementation, so uninstalling the component and later executing them can call unloaded code. Items must release their references during cleanup, and unregistration must defer removal/unloading or fail while references remain.
Confidence: 98%
| String *data = m_item->val_str(&m_string_data); | ||
| *str = data->ptr(); | ||
| *length = data->length(); |
There was a problem hiding this comment.
P1: [P1] Handle dynamically evaluated NULL arguments
The null service uses Item::is_null(), whose base implementation returns false without evaluating most expressions. A dynamically null string expression therefore reaches this code with data == nullptr, and dereferencing it can crash the server. Evaluate the argument and propagate its null_value, checking the returned String * before dereferencing; the blob getter has the same issue.
Confidence: 97%
| // FIXME: forged BINARY(16) | ||
| td->m_type = MYSQL_TYPE_BLOB; | ||
| td->m_type_flags = 0; | ||
| td->m_length = "16"; |
There was a problem hiding this comment.
P2: [P2] Resolve the type definition stored by CREATE TYPE
Every user-defined type is resolved as BLOB(16) regardless of its declared base type. For example, CREATE TYPE ... AS CHAR(13) subsequently creates binary blob variables rather than character variables. Persist the parsed m_type descriptor in the DD object and reconstruct TypeDescriptor from that data here.
Confidence: 100%
| evaluate_to_field(field); | ||
|
|
||
| // FIXME | ||
| return TYPE_ERR_BAD_VALUE; |
There was a problem hiding this comment.
P2: [P2] Return the actual field-evaluation status
This method always reports TYPE_ERR_BAD_VALUE, even when evaluate_to_field() succeeds. Callers that honor the conversion status will treat valid UDT function assignments as failed. Return TYPE_OK on a false evaluation result and TYPE_ERR_BAD_VALUE only on failure.
Confidence: 98%
There was a problem hiding this comment.
Codex PR review
Overall: patch is incorrect (98% confidence)
Core UDT metadata is lost, table-column UDTs are not resolved, result stores signal failure, and runtime registration has unsafe type and lifetime handling.
4 finding(s) were posted inline.
Findings without a current diff anchor
These findings remain in the summary because their locations could not be verified against the current GitHub diff.
P1: [P1] Resolve UDTs before constructing table columns
Adding user_defined_type to field_def leaves PT_field_def_base::type as MYSQL_TYPE_INVALID; the existing table-column contextualization passes that directly to Create_field::init and never propagates or resolves Type_ident. Thus CREATE/ALTER TABLE ... (c schema.type) cannot construct a valid column. Carry the identifier into the field definition, resolve it, and call the descriptor-aware initializer.
Confidence: 96%
sql/sql_yacc.yy:7206-7206
| return true; | ||
| } | ||
|
|
||
| if (dd::create_udt_type(thd, *existing_schema, type_name)) { |
There was a problem hiding this comment.
P1: [P1] Persist the declared underlying type
m_type from CREATE TYPE ... AS ... is discarded here, and the DD object stores only the type name. Consequently CHAR(13) and BINARY(16) declarations become indistinguishable, while resolution later hard-codes every UDT to BLOB(16). Store the parsed base-type descriptor in the DD object and restore it during resolution.
Confidence: 99%
| evaluate_to_field(field); | ||
|
|
||
| // FIXME | ||
| return TYPE_ERR_BAD_VALUE; |
There was a problem hiding this comment.
P1: [P1] Do not report successful stores as bad values
The function result is written to field, but this method unconditionally returns TYPE_ERR_BAD_VALUE, even when evaluation succeeds. Callers such as assignments, inserts, and materialization therefore treat valid UDT results as conversion failures. Return an error only when evaluate_to_field fails, otherwise return TYPE_OK.
Confidence: 99%
| // FIXME: lifecycle, may be in use | ||
| udt_function_hash->erase(key); |
There was a problem hiding this comment.
P1: [P1] Prevent unloading functions that still have live items
Parsing retains a pointer to the registry record and component-owned descriptor/callback, but unregister erases it regardless of ref_count; moreover Item_udt_func never releases its acquired reference. Uninstalling a component while a prepared statement or concurrent execution still references its function can subsequently call unloaded code or dereference unloaded descriptor memory. Release references from item destruction and reject/defer unregister while references remain.
Confidence: 94%
| for (size_t i = 0; i < count; i++) { | ||
| // FIXME: build proper value | ||
| item = that->get_arg(i); | ||
| array[i] = new UDT_value_in(item); |
There was a problem hiding this comment.
P2: [P2] Enforce registered argument descriptors
Argument construction checks only the count and wraps each raw Item; fd->argument_type_array is never consulted. A call with incompatible SQL values therefore reaches component code as if correctly typed—for example the example blob decoder receives short strings and consumes an uninitialized 16-byte buffer. Resolve/coerce each argument against its registered descriptor or reject the call before invoking the callback.
Confidence: 91%
| #ifdef WITH_EXPERIMENTAL_UDT | ||
| WARN_NOT_IMPLEMENTED(thd, "Sql_cmd_create_type::execute()"); |
There was a problem hiding this comment.
P1: [P1] Enforce privileges before creating a type
This execution path writes a persistent DD object without any schema privilege check. Consequently, a user lacking CREATE privileges can issue CREATE TYPE in any existing schema. Call check_access() with the appropriate CREATE privilege before acquiring locks or modifying the dictionary.
Confidence: 98%
| return true; | ||
| } | ||
|
|
||
| if (dd::create_udt_type(thd, *existing_schema, type_name)) { |
There was a problem hiding this comment.
P1: [P1] Persist the declared underlying type
Only the schema and type name are stored; m_type is never used and the DD object has no underlying-type attributes. Resolution subsequently hard-codes every alias as BINARY(16), so declarations such as CREATE TYPE ... AS CHAR(13) have incorrect storage and semantics. Persist the complete declared type descriptor and reconstruct it during resolution.
Confidence: 99%
| // FIXME: lifecycle, may be in use | ||
| udt_function_hash->erase(key); |
There was a problem hiding this comment.
P1: [P1] Prevent unloading functions that are still referenced
Unregistration erases the function unconditionally despite acquire_udt_function() incrementing ref_count, and Item_udt_func never releases that reference. A prepared statement can retain the record, unload the component, and later dereference its unloaded descriptor/callback, causing a crash. Release references from item teardown and reject or defer unregistration while references remain.
Confidence: 95%
| m_target_def.add_foreign_key(FK_SCHEMA_ID, "FK_SCHEMA_ID", | ||
| "FOREIGN KEY (schema_id) " | ||
| "REFERENCES schemata(id)"); |
There was a problem hiding this comment.
P2: [P2] Remove UDT rows when dropping their schema
The new DD table has a restrictive foreign key to schemata, but the DROP DATABASE path has no corresponding cleanup for UDT objects. A schema containing a created type therefore retains a child row and prevents deletion of the schema record. Explicitly drop all schema-owned UDT objects during DROP DATABASE or define appropriate cascading behavior.
Confidence: 90%
| evaluate_to_field(field); | ||
|
|
||
| // FIXME | ||
| return TYPE_ERR_BAD_VALUE; |
There was a problem hiding this comment.
P2: [P2] Return the actual field-save status
save_in_field_inner() reports TYPE_ERR_BAD_VALUE unconditionally and ignores whether evaluation succeeded. Contexts that save a UDT function directly into a field, such as INSERT/UPDATE assignments, therefore report a conversion failure even after producing a valid value. Propagate evaluation failure and return TYPE_OK on success.
Confidence: 98%
Proof Of Concepts - MySQL User Defined Types
Contributes to #674
Caution
Proof of concepts code, do not merge, do not use in production.
This code is dirty and to throw away.
The point of this PR is to do prototyping, to identify what needs to be changed in the server, to start a technical discussion.
Quick start
Build
Compile with
-DWITH_EXPERIMENTAL_UDT=ON -DWITH_DEBUG=ONinCMake.Run
Current progress
Declare a UDT and provide an implementation:
Declare a UDT variable, perform operations provided by the component:
Invoke the UDT implementation at runtime:
Cleanup:
Component implementation
Check files under
components/udt_example.