MDEV-39587 Package-wide TYPE for variable declarations#5179
MDEV-39587 Package-wide TYPE for variable declarations#5179sanja-byelkin wants to merge 1 commit into
Conversation
SET sql_mode=ORACLE;
DELIMITER $$
CREATE OR REPLACE PACKAGE pkg AS
-- Declare a package public data type
TYPE varchar_array IS TABLE OF VARCHAR(2000) INDEX BY INTEGER;
END;
$$
DELIMITER ;
DELIMITER $$
CREATE OR REPLACE PROCEDURE p1 AS
v pkg.varchar_array; -- Use the package public data type
BEGIN
v(0):='test';
SELECT v(0);
END;
$$
DELIMITER ;
Note, the change is done only for sql_mode=ORACLE, because the TYPE
declaration is not available for the default mode.
Where package-wide types are available
--------------------------------------
- Variabe list type:
DECLARE var pkg1.type1;
- RETURN type for a package routine:
CREATE FUNCTION .. RETURN pkg1.type1 ...
- Parameter type for a package routine:
PROCEDURE p1(param1 pkg1.type1);
- Assoc array element type:
TYPE assoc1_t IS TABLE OF pkg1.type1 ...
- REF CURSOR RETURN type:
TYPE cur1_t IS REF CURSOR RETURN pkg1.type1;
Change details
--------------
- Adding a member Lex_length_and_dec_st::m_foreign_module_type
It's set to true when the data type was initialized from a TYPE
in foreign routine (e.g. in PACKAGE spec).
It's needed to prevent use of qualified identifiers in public contexts,
i.e. in schema public routine parameter types and schema publuc function
RETURN types.
Adding a helper method sp_head::check_applicability() which prevents
use of qualified types in public context.
- Adding a helper method sp_head::raise_unknown_data_type().
- Adding methods LEX::set_field_type_typedef_package_spec() for
2-step and 3-step qualified indentifiers.
It's used in field_type_all_with_typedefs which covers cases:
- Variabe list type : DECLARE var pkg1.type1;
- RETURN type : CREATE FUNCTION .. RETURN pkg1.type1 ...
- Parameter type : PROCEDURE p1(param1 pkg1.type1);
- Assoc array element type : TYPE assoc1_t IS TABLE OF pkg1.type1 ...
- Adding a method LEX::declare_type_ref_cursor_return_typedef().
It handles cases when a new TYPE REF CURSOR RETURN is declared,
for both for qualified RETURN types and non-qualified RETURN types:
- TYPE cur0_t IS REF CURSOR RETURN rec1_t;
- TYPE cur0_t IS REF CURSOR RETURN pkg1.rec1_t;
- TYPE cur0_t IS REF CURSOR RETURN db1.pkg1.rec1_t;
The code was moved from LEX::declare_type_ref_cursor() into
LEX::declare_type_ref_cursor_return_typedef() and extended
to cover qualified RETURN types.
- Adding a method Sql_path::find_package_spec_type().
It iterates through all schemas specified in @@path and searches
for the given type in the given package.
- Adding a helper method sp_pcontext::type_defs_add_ref_cursor()
to reuse the code.
- Adding a new method sp_package::get_typedef() to search
for TYPE definitions in PACKAGE specifications.
- Adding a new method sp_head::get_typedef_package_spec()
to search for TYPE definitions used by a PROCEDURE or FUNCTION.
- Adding a helper method
Sp_handler::sp_cache_routine_reentrant_suppress_errors
Adding a method Sp_handler::find_package_spec().
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements package-wide TYPE declarations for variable declarations (MDEV-39587) in MariaDB's Oracle compatibility mode, supporting 2-step and 3-step qualified names and resolution via the @@PATH variable. The review feedback highlights several critical improvement opportunities, including potential buffer truncation risks when formatting long identifier names, multiple potential null pointer dereferences when accessing parse and child contexts, and redundant casting in type declaration logic.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const Lex_ident_sys_st &package, | ||
| const Lex_ident_sys_st &type) | ||
| { | ||
| char buf[128]; |
There was a problem hiding this comment.
The buffer size of 128 bytes is too small and can lead to truncation of the error message. Since database, package, and type names can each be up to 64 characters (NAME_LEN), formatting them with backticks and dots can exceed 200 characters. Consider increasing the buffer size to 512 bytes to safely accommodate long identifier names.
char buf[512];| void sp_head::raise_unknown_data_type(const Lex_ident_sys_st &package, | ||
| const Lex_ident_sys_st &type) | ||
| { | ||
| char buf[128]; |
There was a problem hiding this comment.
The buffer size of 128 bytes is too small and can lead to truncation of the error message. Since package and type names can each be up to 64 characters (NAME_LEN), formatting them with backticks and dots can exceed 130 characters. Consider increasing the buffer size to 512 bytes to safely accommodate long identifier names.
char buf[512];| if (!(tdef[0]= get_parse_context()->child_context(0)-> | ||
| find_type_def(type, false))) |
There was a problem hiding this comment.
Potential null pointer dereference. get_parse_context() or child_context(0) can return nullptr in unexpected or corrupted states. Adding defensive null checks prevents potential crashes.
sp_pcontext *pctx= get_parse_context();
sp_pcontext *child_ctx= pctx ? pctx->child_context(0) : nullptr;
if (!child_ctx || !(tdef[0]= child_ctx->find_type_def(type, false)))| if ((spec= Sp_handler::find_package_spec(thd, ndb, package)) && | ||
| (tdef[0]= spec->get_parse_context()->child_context(0)-> | ||
| find_type_def(type, false))) | ||
| { | ||
| return spec->get_typedef(thd, tdef, ndb, package, type); | ||
| } |
There was a problem hiding this comment.
Potential null pointer dereference. spec->get_parse_context() or child_context(0) can return nullptr. Adding defensive null checks prevents potential crashes.
if ((spec= Sp_handler::find_package_spec(thd, ndb, package)))
{
sp_pcontext *pctx= spec->get_parse_context();
sp_pcontext *child_ctx= pctx ? pctx->child_context(0) : nullptr;
if (child_ctx && (tdef[0]= child_ctx->find_type_def(type, false)))
return spec->get_typedef(thd, tdef, ndb, package, type);
}| sp_type_def *tdef= pkg->get_parse_context()->child_context(0)-> | ||
| find_type_def(type, false); |
There was a problem hiding this comment.
Potential null pointer dereference. pkg->get_parse_context() or child_context(0) can return nullptr. Adding defensive null checks prevents potential crashes.
| sp_type_def *tdef= pkg->get_parse_context()->child_context(0)-> | |
| find_type_def(type, false); | |
| sp_pcontext *pctx= pkg->get_parse_context(); | |
| sp_pcontext *child_ctx= pctx ? pctx->child_context(0) : nullptr; | |
| sp_type_def *tdef= child_ctx ? child_ctx->find_type_def(type, false) : nullptr; |
| if (!dynamic_cast<const Type_handler_row*>(rt->type_handler()) || | ||
| !dynamic_cast<const sp_type_def_record*>(rt)) | ||
| { | ||
| my_error(ER_ILLEGAL_PARAMETER_DATA_TYPE_FOR_OPERATION, MYF(0), | ||
| type.str, "REF CURSOR RETURN"); | ||
| return true; | ||
| } | ||
| Row_definition_list *row= static_cast<const sp_type_def_record*>(rt)-> | ||
| field->deep_copy(thd); |
There was a problem hiding this comment.
Avoid redundant and inefficient casting. We can store the result of the first dynamic_cast to avoid performing it twice and to eliminate the subsequent static_cast.
const sp_type_def_record *rec_type= dynamic_cast<const sp_type_def_record*>(rt);
if (!dynamic_cast<const Type_handler_row*>(rt->type_handler()) || !rec_type)
{
my_error(ER_ILLEGAL_PARAMETER_DATA_TYPE_FOR_OPERATION, MYF(0),
type.str, "REF CURSOR RETURN");
return true;
}
Row_definition_list *row= rec_type->field->deep_copy(thd);There was a problem hiding this comment.
Pull request overview
This PR extends Oracle-mode stored program parsing and type resolution to allow using package-spec TYPE definitions (e.g., pkg1.type1 and db1.pkg1.type1) in more contexts such as variable declarations, refcursor RETURN typedefs, and other type-declaration sites, with @@PATH-based resolution when needed.
Changes:
- Add support for 2-step and 3-step qualified package-spec type references in the grammar and LEX type resolution paths (including @@path lookup).
- Refactor/extend REF CURSOR type declaration logic to support qualified typedefs in the
RETURNclause. - Add extensive Oracle-compatibility test coverage for package-spec types, refcursors, records, assoc arrays, and PATH resolution behavior.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sql/structs.h | Adds a flag (m_foreign_module_type) to mark types originating from package specs for later applicability checks. |
| sql/sql_yacc.yy | Extends grammar to parse package-spec type references and qualified REF CURSOR RETURN typedefs; allows TYPE declarations in package specs (Oracle mode). |
| sql/sql_path.h | Declares helper to resolve package-spec types via @@path. |
| sql/sql_path.cc | Implements @@path iteration to locate package specs containing requested TYPE definitions. |
| sql/sql_lex.h | Adds new LEX helpers for package-spec typedef resolution and qualified REF CURSOR RETURN typedef declarations. |
| sql/sql_lex.cc | Implements package-spec typedef field-type setup and qualified REF CURSOR RETURN typedef declaration logic; applies applicability checks for params/returns. |
| sql/sp.h | Introduces Sp_handler helpers for finding/caching package specs safely and suppressing errors. |
| sql/sp.cc | Implements suppressed-error reentrant cache helper and package-spec lookup routine. |
| sql/sp_pcontext.h | Adds helper for adding REF CURSOR typedefs to reduce duplication. |
| sql/sp_pcontext.cc | Implements type_defs_add_ref_cursor() helper. |
| sql/sp_head.h | Adds APIs for applicability checks and package-spec typedef lookup; adds sp_package typedef lookup. |
| sql/sp_head.cc | Implements applicability checks, improved unknown-type errors, and package-spec TYPE lookup logic (2-step/3-step + @@path). |
| plugin/type_cursor/mysql-test/type_cursor/sp-ref_cursor-return-type-of-var.test | Updates expected error class for unsupported qualified %TYPE usage. |
| plugin/type_cursor/mysql-test/type_cursor/sp-ref_cursor-return-type-of-var.result | Updates expected output to match new unknown-data-type error behavior. |
| plugin/type_cursor/mysql-test/type_cursor/sp-ref_cursor-return-rowtype-of-table.test | Updates expected error class for unsupported qualified %ROWTYPE usage. |
| plugin/type_cursor/mysql-test/type_cursor/sp-ref_cursor-return-rowtype-of-table.result | Updates expected output to match new unknown-data-type error behavior. |
| mysql-test/suite/compat/oracle/t/type_date.test | Updates expectations to reflect new parsing path (schema treated as package name) and unknown type error. |
| mysql-test/suite/compat/oracle/t/sp-package-spec-type.test | Adds new test coverage for package-spec types in variable declarations (2-step/3-step) and orphan references. |
| mysql-test/suite/compat/oracle/t/sp-package-spec-type-refcursor.test | Adds comprehensive tests for package-spec REFCURSOR typedef usage and restrictions. |
| mysql-test/suite/compat/oracle/t/sp-package-spec-type-record.test | Adds comprehensive tests for package-spec RECORD typedef usage and restrictions. |
| mysql-test/suite/compat/oracle/t/sp-package-spec-type-path.test | Adds tests validating @@path resolution ordering and CURRENT_SCHEMA behavior for package types. |
| mysql-test/suite/compat/oracle/t/sp-package-spec-type-inet6.test | Adds tests for package-spec types based on built-in types (INET6) and usage in procs/packages. |
| mysql-test/suite/compat/oracle/t/sp-package-spec-type-assoc.test | Adds tests for assoc array element types referencing package-spec types (incl. error cases). |
| mysql-test/suite/compat/oracle/r/type_date.result | Updates golden output for new unknown-data-type errors. |
| mysql-test/suite/compat/oracle/r/sp-package-spec-type.result | Adds golden output for new package-spec type tests. |
| mysql-test/suite/compat/oracle/r/sp-package-spec-type-refcursor.result | Adds golden output for new refcursor package-spec type tests. |
| mysql-test/suite/compat/oracle/r/sp-package-spec-type-record.result | Adds golden output for new record package-spec type tests. |
| mysql-test/suite/compat/oracle/r/sp-package-spec-type-path.result | Adds golden output for @@path resolution tests. |
| mysql-test/suite/compat/oracle/r/sp-package-spec-type-inet6.result | Adds golden output for inet6 package-spec type tests. |
| mysql-test/suite/compat/oracle/r/sp-package-spec-type-assoc.result | Adds golden output for assoc-array package-spec type tests. |
| mysql-test/main/parser.result | Updates parser golden output for a changed syntax error message location. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!(pkg= Sp_handler::find_package_spec(thd, ncdb, package))) | ||
| continue; | ||
| sp_type_def *tdef= pkg->get_parse_context()->child_context(0)-> | ||
| find_type_def(type, false); |
| if (!(tdef[0]= get_parse_context()->child_context(0)-> | ||
| find_type_def(type, false))) |
| if ((spec= Sp_handler::find_package_spec(thd, ndb, package)) && | ||
| (tdef[0]= spec->get_parse_context()->child_context(0)-> | ||
| find_type_def(type, false))) |
sanja-byelkin
left a comment
sanja-byelkin
left a comment
There was a problem hiding this comment.
It looks OK, but as usual there are small problems
| to sp_head after Lex_ident_routine::check_name_with_error(). | ||
| */ | ||
| if (m_handler == &sp_handler_package_spec && | ||
| dbn.streq(db) && |
There was a problem hiding this comment.
dbn made from db, so probably it is almost always true : dbn.streq(db) ?
| const Lex_ident_sys_st &package, | ||
| const Lex_ident_sys_st &type) | ||
| { | ||
| char buf[128]; |
There was a problem hiding this comment.
Is 128 enough? NAME_LEN is 64*SYSTEM_CHARSET_MBMAXLEN
| void sp_head::raise_unknown_data_type(const Lex_ident_sys_st &package, | ||
| const Lex_ident_sys_st &type) | ||
| { | ||
| char buf[128]; |
| const Lex_ident_sys_st &package, | ||
| const Lex_ident_sys_st &type) | ||
| { | ||
| if (!(tdef[0]= get_parse_context()->child_context(0)-> |
There was a problem hiding this comment.
If get_parse_context()->child_context(0) it is better to make DBUG_ASSERT, otherwise it should be checked on NULL
| if (!(ndb= thd->to_ident_db_normalized_with_error(m_db)).str) | ||
| return true; | ||
| sp_package *spec; | ||
| if ((spec= Sp_handler::find_package_spec(thd, ndb, package)) && |
There was a problem hiding this comment.
Here I put claude quate:
A routine in mydb references pkg1.type1. find_package_spec finds mydb.pkg1, but find_type_def returns null (type1 not defined there). The condition at 4402-4404 fails, and execution falls through to @@path search. PATH may find type1 in a completely different database's pkg1, silently resolving to an unrelated type instead of raising ER_UNKNOWN_DATA_TYPE for the local package. This implicit 'try-local-then-fallback-to-path' is undocumented and asymmetric with the 3-step path which never silently retries.
SET sql_mode=ORACLE;
DELIMITER $$
CREATE OR REPLACE PACKAGE pkg AS
-- Declare a package public data type
TYPE varchar_array IS TABLE OF VARCHAR(2000) INDEX BY INTEGER;
END;
$$
DELIMITER ;
DELIMITER $$
CREATE OR REPLACE PROCEDURE p1 AS
v pkg.varchar_array; -- Use the package public data type
BEGIN
v(0):='test';
SELECT v(0);
END;
$$
DELIMITER ;
Note, the change is done only for sql_mode=ORACLE, because the TYPE declaration is not available for the default mode.
Where package-wide types are available
Variabe list type: DECLARE var pkg1.type1;
RETURN type for a package routine: CREATE FUNCTION .. RETURN pkg1.type1 ...
Parameter type for a package routine: PROCEDURE p1(param1 pkg1.type1);
Assoc array element type: TYPE assoc1_t IS TABLE OF pkg1.type1 ...
REF CURSOR RETURN type: TYPE cur1_t IS REF CURSOR RETURN pkg1.type1;
Change details
Adding a member Lex_length_and_dec_st::m_foreign_module_type It's set to true when the data type was initialized from a TYPE in foreign routine (e.g. in PACKAGE spec). It's needed to prevent use of qualified identifiers in public contexts, i.e. in schema public routine parameter types and schema publuc function RETURN types. Adding a helper method sp_head::check_applicability() which prevents use of qualified types in public context.
Adding a helper method sp_head::raise_unknown_data_type().
Adding methods LEX::set_field_type_typedef_package_spec() for 2-step and 3-step qualified indentifiers. It's used in field_type_all_with_typedefs which covers cases:
Adding a method LEX::declare_type_ref_cursor_return_typedef(). It handles cases when a new TYPE REF CURSOR RETURN is declared, for both for qualified RETURN types and non-qualified RETURN types:
The code was moved from LEX::declare_type_ref_cursor() into
LEX::declare_type_ref_cursor_return_typedef() and extended
to cover qualified RETURN types.
Adding a method Sql_path::find_package_spec_type(). It iterates through all schemas specified in @@path and searches for the given type in the given package.
Adding a helper method sp_pcontext::type_defs_add_ref_cursor() to reuse the code.
Adding a new method sp_package::get_typedef() to search for TYPE definitions in PACKAGE specifications.
Adding a new method sp_head::get_typedef_package_spec() to search for TYPE definitions used by a PROCEDURE or FUNCTION.
Adding a helper method Sp_handler::sp_cache_routine_reentrant_suppress_errors Adding a method Sp_handler::find_package_spec().