Hello Sergei, Thanks for review. Please find a new patch version here: https://github.com/MariaDB/server/commit/715dbfa6757d319b1a18004467ed0b5b26f... See comments below: On 8/24/24 00:43, Sergei Golubchik wrote:
Hi, Alexander,
Looks good, thanks. Great commit comment too.
Just a few questions and test suggestions - see below.
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
On Aug 23, Alexander Barkov wrote:
revision-id: c46e7c5cb64 (mariadb-11.4.1-5-gc46e7c5cb64) parent(s): ae6684d79f4 author: Alexander Barkov committer: Alexander Barkov timestamp: 2024-02-20 13:49:54 +0400 message:
MDEV-12252 ROW data type for stored function return values
diff --git a/mysql-test/main/sp-anchor-row-type-table.test b/mysql-test/main/sp-anchor-row-type-table.test index 7123f9160db..2fbcfbffc1b 100644 --- a/mysql-test/main/sp-anchor-row-type-table.test +++ b/mysql-test/main/sp-anchor-row-type-table.test @@ -939,3 +939,70 @@ DROP TABLE t1; --echo # --echo # End of 10.4 tests --echo # + +--echo # +--echo # Start of 11.3 tests +--echo # + +--echo # +--echo # MDEV-12252 ROW data type for stored function return values +--echo # + +--error ER_PARSE_ERROR +CREATE FUNCTION f1() RETURNS ROW TYPE OF step1.step2.step3 RETURN ROW(1,2); + +CREATE FUNCTION f1() RETURNS ROW TYPE OF t1 RETURN ROW(1,2);
you don't resolve t1 at CREATE FUNCTION time. why? is it how TYPE OF works in DECLARE too?
Yes. In DECLARE it also gets resolved only at the execution time.
+SHOW CREATE FUNCTION f1; +--error ER_NO_SUCH_TABLE +SELECT f1(); +DROP FUNCTION f1; + +CREATE FUNCTION f1() RETURNS ROW TYPE OF test.t1 RETURN ROW(1,2); +SHOW CREATE FUNCTION f1; +--error ER_NO_SUCH_TABLE +SELECT f1(); +DROP FUNCTION f1; + + +CREATE TABLE t1 (a INT, b VARCHAR(32)); +CREATE FUNCTION f1() RETURNS ROW TYPE OF test.t1 RETURN (SELECT *
FROM t1);
+SHOW CREATE FUNCTION f1; +DELIMITER $$; +CREATE PROCEDURE p1() +BEGIN + DECLARE r ROW TYPE OF t1 DEFAULT f1(); + SELECT r.a, r.b; +END; +$$ +DELIMITER ;$$ + +# Testing with no rows +CALL p1(); +--error ER_OPERAND_COLUMNS +SELECT f1(); +SELECT f1()=ROW(1,'b1') AS c; +SELECT f1()=ROW(NULL,NULL) AS c; + +# Testing with one row +INSERT INTO t1 VALUES (1,'b1'); +CALL p1(); +--error ER_OPERAND_COLUMNS +SELECT f1(); +SELECT f1()=ROW(1,'b1') AS c; +SELECT f1()=ROW(1,'') AS c; +SELECT f1()=ROW(2,'b1') AS c; +SELECT f1()=ROW(1,NULL) AS c; +SELECT f1()=ROW(NULL,'b1') AS c;
do one more SHOW CREATE FUNCTION here
Done.
+ +# Testing with two rows +INSERT INTO t1 VALUES (2,'b2'); +--error ER_SUBQUERY_NO_1_ROW +CALL p1(); +--error ER_OPERAND_COLUMNS +SELECT f1(); +--error ER_SUBQUERY_NO_1_ROW +SELECT f1()=ROW(1,'b1') AS c; + +DROP PROCEDURE p1; +DROP FUNCTION f1; +DROP TABLE t1; diff --git a/mysql-test/main/sp-anchor-type.test b/mysql-test/main/sp-anchor-type.test index 0e24ef900d8..fed53fd011a 100644 --- a/mysql-test/main/sp-anchor-type.test +++ b/mysql-test/main/sp-anchor-type.test @@ -767,3 +767,78 @@ BEGIN NOT ATOMIC END; $$ DELIMITER ;$$ <cut> +SHOW CREATE FUNCTION f1; +SELECT f1(); +CREATE TABLE t2 AS SELECT f1(); +SHOW CREATE TABLE t2; +SELECT * FROM t2; +DROP TABLE t2; +INSERT INTO t1 VALUES ('10:20:30'); +SELECT f1(); +CREATE TABLE t2 AS SELECT f1(); +SHOW CREATE TABLE t2; +SELECT * FROM t2; +DROP TABLE t2;
please, also try here
DROP TABLE t1; CREATE TABLE t1 (a INT); INSERT INTO t1 VALUES (10); CREATE TABLE t2 AS SELECT f1();
the point is to change TYPE OF t1.a after the function has been successfully called, and thus is already in the sp cache.
Done.
+DROP FUNCTION f1; +DROP TABLE t1; diff --git a/mysql-test/suite/compat/oracle/t/sp-package.test
b/mysql-test/suite/compat/oracle/t/sp-package.test
index aefede41c8b..7c231d736c5 100644 --- a/mysql-test/suite/compat/oracle/t/sp-package.test +++ b/mysql-test/suite/compat/oracle/t/sp-package.test @@ -3109,3 +3109,79 @@ DROP TABLE t1; DROP FUNCTION f1_deterministic; DROP FUNCTION f2_not_deterministic; DROP PACKAGE pkg1; + +--echo # +--echo # MDEV-12252 ROW data type for stored function return values +--echo # + +--echo # +--echo # Testing fixed ROW type with package routines +--echo # + +DELIMITER $$; +CREATE PACKAGE pkg +AS + FUNCTION f1() RETURN ROW(a INT, b VARCHAR(32)); + PROCEDURE p1(r ROW(a INT, b VARCHAR(32))); + PROCEDURE p2(); +END; +$$ <cut> +DELIMITER ;$$ +CALL pkg.p1(pkg.f1()); +CALL pkg.p2; +DROP PACKAGE pkg; + + +--echo # +--echo # Testing table%ROWTYPE with package routines +--echo #
now you can also add package tests outside of oracle mode
Done: I've just added tests into main/sp-package.test.
+ +CREATE TABLE t1 (a INT, b VARCHAR(32)); +INSERT INTO t1 VALUES (1,'b1'); +DELIMITER /; +CREATE PACKAGE pkg +AS + FUNCTION f1 RETURN t1%ROWTYPE; + PROCEDURE p1(r t1%ROWTYPE); + PROCEDURE p2; +END; +/ +CREATE PACKAGE BODY pkg +AS + FUNCTION f1 RETURN t1%ROWTYPE AS + r t1%ROWTYPE; + BEGIN + SELECT * INTO r FROM t1; + RETURN r; + END; + PROCEDURE p1(r t1%ROWTYPE) AS + BEGIN + SELECT r.a || ' ' || r.b; + END; + PROCEDURE p2 AS + BEGIN + p1(f1()); + END; +END; +/ +DELIMITER ;/ +CALL pkg.p1(pkg.f1()); +CALL pkg.p2; +DROP PACKAGE pkg; +DROP TABLE t1; diff --git a/sql/field.h b/sql/field.h index 7f1c243a180..7af069a9735 100644 --- a/sql/field.h +++ b/sql/field.h @@ -1373,6 +1374,20 @@ class Field: public Value_source in str and restore it with set() if needed */ virtual void sql_type(String &str) const =0; + void sql_type_for_sp_returns(String &str) const
This is oddly specific. Strange that we don't need to print the field type with charset/collation anywhere else, but for sp returns we do.
This is true. Only the RETURN clause prints the data type and charset+collation. Other things do not do that: - Table columns: their character set and collation needed to be printed separately, e.g. in separate I_S columns. So the method sql_type() does not print character sets and collations. - SP parameters and variables: they get remembered as fragments of the original CREATE PROCEDURE or CREATE FUNCTION query. So variables and parameters don't print their data type individually at all: the corresponding query fragment prints them all instead (one fragment for the parameters, one fragment for the routine body). I don't change much here. Just instead of a global function sp_returns_type() I introduced a method Field::sql_type_for_sp_returns(), which I think a more proper place for this code. The actual code printing CHARSET and COLLATE is moving from sp_returns_type() to here.
+ { + sql_type(str); + if (has_charset()) + { + str.append(STRING_WITH_LEN(" CHARSET ")); + str.append(charset()->cs_name); + if (Charset(charset()).can_have_collate_clause()) + { + str.append(STRING_WITH_LEN(" COLLATE ")); + str.append(charset()->coll_name); + } + } + } virtual void sql_rpl_type(String *str) const { sql_type(*str); } virtual uint size_of() const =0; // For new field inline bool is_null(my_ptrdiff_t row_offset= 0) const diff --git a/sql/field.cc b/sql/field.cc index 35be045620d..23b050de221 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -2768,6 +2768,32 @@ bool Field_row::sp_prepare_and_store_item(THD
*thd, Item **value)
}
+uint Field_row::cols() const +{ + // The table with ROW members must be already instantiated + DBUG_ASSERT(m_table); + return m_table->s->fields; +} + + +void Field_row::sql_type(String &res) const
would be more logical to call this Field_row::sql_type_for_sp_returns()
Thanks. Correct. I renamed it.
+{ + res.set_ascii(STRING_WITH_LEN("row(")); + for (uint i= 0; i < m_table->s->fields; i++) + { + if (i > 0) + res.append(','); + Field *field= m_table->field[i]; + res.append(field->field_name); + res.append(' '); + StringBuffer<64> col; + field->sql_type_for_sp_returns(col); + res.append(col.to_lex_cstring()); + } + res.append(')'); +} + +
/****************************************************************************
Functions for the Field_decimal class This is an number stored as a pre-space (or pre-zero) string diff --git a/sql/item.cc b/sql/item.cc index 54ad511cb6b..0a125752979 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -3001,10 +3003,31 @@ Item_sp::init_result_field(THD *thd, uint
max_length, uint maybe_null,
dummy_table->s->table_name= empty_clex_str; dummy_table->maybe_null= maybe_null;
+ if (m_sp->m_return_field_def.is_column_type_ref() && + m_sp->m_return_field_def.column_type_ref()-> + resolve_type_ref(thd, &m_sp->m_return_field_def)) + DBUG_RETURN(TRUE);
Here you overwrite old m_return_field_def value which was a
column_type_ref > with its actual type, don't you? > > What will happen if you change the column's type > after the sp was already executed once?
Well, it was overriding only the "Column_definition" part of m_return_field_def, while is_column_type_ref() still returned "true" on the next execution. I agree it looked confusing. I changed this way: - added a "const Column_defintion &def" parameter to create_result_field() - call resolve_type_def() with a new local variable. - call create_result_field() with the same local variable. m_return_field_def stays intouched.
+ if (!(sp_result_field= m_sp->create_result_field(max_length, name, dummy_table))) DBUG_RETURN(TRUE);
+ /* + In case of a ROW return type we need to create Item_field_row + on top of Field_row, and remember it in sp_result_item_field_row. + ROW members are later accessed using
sp_result_item_field_row->addr(i),
+ e.g. when copying the function return value to a local variable. + For scalar return types no Item_field is needed around sp_result_field, + as the value is fetched directly from sp_result_field, + inside Item_func_sp::val_xxx() methods.
Sorry, I don't understand that. Why cannot you use Field_row here?
I realized that creating Item_field_row was redundant here. So in class Item_sp I'm not adding the member "Item_field_row *sp_result_item_field_row" any more. Instead I'm adding "Item_args sp_result_field_items" - as the storage for an array of Item_field corresponding to the ROW members. They are needed to have the descendant class Item_func_sp implement methods element_index() and addr() properly.
+ */ + if (Field_row *field_row= dynamic_cast<Field_row*>(sp_result_field)) + { + if (!(sp_result_item_field_row= + m_sp->m_return_field_def.make_item_field_row(thd, field_row))) + DBUG_RETURN(true); + } + if (sp_result_field->pack_length() > sizeof(result_buf)) { void *tmp; diff --git a/sql/sp.cc b/sql/sp.cc index ddaeeb8cb7a..7eab8304c31 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -1091,18 +1136,19 @@ sp_returns_type(THD *thd, String &result, const sp_head *sp) bzero((char*) &share, sizeof(share)); table.in_use= thd; table.s = &share; - field= sp->create_result_field(0, 0, &table); - field->sql_type(result); + field= create_result_field(0, 0, &table);
- if (field->has_charset()) + if (m_return_field_def.is_row()) { - result.append(STRING_WITH_LEN(" CHARSET ")); - result.append(field->charset()->cs_name); - if (Charset(field->charset()).can_have_collate_clause()) - { - result.append(STRING_WITH_LEN(" COLLATE ")); - result.append(field->charset()->coll_name); - } + Field_row *field_row= dynamic_cast<Field_row*>(field); + if (!field_row->row_create_fields( + thd, m_return_field_def.row_field_definitions())) + field->sql_type(result);
when is this used? you're creating a TABLE and Field's - only to print types and charsets. This might be tolerable, but only if it's not needed often
This is called during CREATE FUNCTION. This is needed to remember the normalized representation of the return data type in mysql.proc.returns, to print its normalized form later in "SELECT returns FROM mysql.proc" and the corresponding I_S queries. The code in Sp_handler::sp_create_routine() instantiates the Field befind the RETURNS clause and then calls the method of the created Field to print the returned data type. It has always worked this way. I'm not chaning it. Just in case of "RETURNS ROW" it needs to instantiate Fields befind all ROW members, so the data types of the ROW parts also get remembered in normalized forms.
+ } + else + { + DBUG_ASSERT(m_return_field_def.type_handler()->is_scalar_type()); + field->sql_type_for_sp_returns(result); }
delete field; diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 5977e309ae4..f11e4f89466 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -18347,7 +18338,50 @@ sp_parameters: ;
sf_returned_type_clause: - RETURNS_SYM sf_return_type + RETURNS_SYM + { + LEX *lex= Lex;
you still do this lex=Lex? :) it became obsolete, like, 15 years ago. If not more.
Removed.
+ lex->init_last_field(&lex->sphead->m_return_field_def, + &empty_clex_str); + } + sf_return_type + ; +
<cut>