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(a)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?
> +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
> +
> +# 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 ;$$
> +
> +
> +--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 TYPE OF a RETURN 1;
> +
> +CREATE FUNCTION f1() RETURNS TYPE OF t1.a RETURN (SELECT min(a) FROM t1);
> +SHOW CREATE FUNCTION f1;
> +--error ER_NO_SUCH_TABLE
> +SELECT f1();
> +CREATE TABLE t1 (b INT);
> +--error ER_BAD_FIELD_ERROR
> +SELECT f1();
> +DROP TABLE t1;
> +DROP FUNCTION f1;
> +
> +CREATE DATABASE db1;
> +DELIMITER $$;
> +CREATE FUNCTION db1.f1() RETURNS TYPE OF db1.t1.a
> +BEGIN
> + RETURN (SELECT min(a) FROM t1);
> +END;
> +$$
> +DELIMITER ;$$
> +SHOW CREATE FUNCTION db1.f1;
> +--error ER_NO_SUCH_TABLE
> +SELECT db1.f1();
> +CREATE TABLE db1.t1 (b TIME);
> +--error ER_BAD_FIELD_ERROR
> +SELECT db1.f1();
> +DROP TABLE db1.t1;
> +CREATE TABLE db1.t1 (a TIME);
> +SELECT db1.f1();
> +INSERT INTO db1.t1 VALUES ('10:20:30');
> +SELECT db1.f1();
> +DROP TABLE db1.t1;
> +DROP FUNCTION db1.f1;
> +DROP DATABASE db1;
> +
> +CREATE TABLE t1 (a TIME);
> +CREATE FUNCTION f1() RETURNS TYPE OF test.t1.a RETURN (SELECT min(a) FROM t1);
> +SHOW CREATE FUNCTION f1;
> +SELECT f1();
> +DROP FUNCTION f1;
> +DROP TABLE t1;
> +
> +CREATE TABLE t1 (a TIME);
> +DELIMITER $$;
> +CREATE FUNCTION f1() RETURNS TYPE OF t1.a
> +BEGIN
> + RETURN (SELECT min(a) FROM t1);
> +END;
> +$$
> +DELIMITER ;$$
> +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.
> +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;
> +$$
> +CREATE PACKAGE BODY pkg
> +AS
> + FUNCTION f1() RETURN ROW(a INT, b VARCHAR(32)) AS
> + BEGIN
> + RETURN ROW(1,'b1');
> + END;
> + PROCEDURE p1(r ROW(a INT, b VARCHAR(32))) AS
> + BEGIN
> + SELECT r.a, r.b;
> + END;
> + PROCEDURE p2() AS
> + BEGIN
> + CALL p1(f1());
> + END;
> +END;
> +$$
> +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
> +
> +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.
> + {
> + 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()
> +{
> + 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?
> +
> 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?
> + */
> + 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
> + }
> + 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.
> + lex->init_last_field(&lex->sphead->m_return_field_def,
> + &empty_clex_str);
> + }
> + sf_return_type
> + ;
> +
> +sf_return_type:
> + field_type
> + {
> + if (unlikely(Lex->sf_return_fill_definition($1)))
> + MYSQL_YYABORT;
> + }
> + | ROW_SYM row_type_body
> + {
> + if (Lex->sf_return_fill_definition_row($2))
> + MYSQL_YYABORT;
> + }
> + | ROW_SYM TYPE_SYM OF_SYM ident
> + {
> + if (Lex->sf_return_fill_definition_rowtype_of(
> + Qualified_column_ident(&$4)))
> + MYSQL_YYABORT;
> + }
> + | ROW_SYM TYPE_SYM OF_SYM ident '.' ident
> + {
> + if (Lex->sf_return_fill_definition_rowtype_of(
> + Qualified_column_ident(&$4, &$6)))
> + MYSQL_YYABORT;
> + }
> + | TYPE_SYM OF_SYM ident '.' ident
> + {
> + if (Lex->sf_return_fill_definition_type_of(
> + Qualified_column_ident(&$3, &$5)))
> + MYSQL_YYABORT;
> + }
> + | TYPE_SYM OF_SYM ident '.' ident '.' ident
> + {
> + if (Lex->sf_return_fill_definition_type_of(
> + Qualified_column_ident(thd, &$3, &$5, &$7)))
> + MYSQL_YYABORT;
> + }
> ;
>
> package_implementation_item_declaration:
