maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Rucha!

Looks quite good. A couple of small comments, see below.
After you fix them, no need to ask for a new review, just push into
bb-10.7-row_number

On Oct 15, Rucha Deodhar wrote:
> revision-id: 97458a7767b (mariadb-10.6.1-131-g97458a7767b)
> parent(s): a6cf8b34a83
> author: Rucha Deodhar
> committer: Rucha Deodhar
> timestamp: 2021-10-15 01:54:51 +0530
> message:
> 
> MDEV-26832: ROW_NUMBER in SIGNAL/RESIGNAL causes a syntax error
> 
> Analysis: Parser was missing ROW_NUMBER as syntax for SIGNAL and RESIGNAL.
> Fix: Fix parser and fix how m_row_number is copied like other attributes
> to avoid ROW_NUMBER from assuming default value.
> 
> diff --git a/mysql-test/main/get_diagnostics.test b/mysql-test/main/get_diagnostics.test
> index 83ea1dee343..186d1bfc438 100644
> --- a/mysql-test/main/get_diagnostics.test
> +++ b/mysql-test/main/get_diagnostics.test
> @@ -1552,3 +1552,53 @@ GET DIAGNOSTICS CONDITION 2 @rnum = ROW_NUMBER, @msg = MESSAGE_TEXT, @err = MYSQ
...
> +
> +CREATE PROCEDURE resignal_syntax()
> +BEGIN
> +   DECLARE CONTINUE HANDLER
> +   FOR 1146
> +   BEGIN
> +      RESIGNAL SET
> +      MESSAGE_TEXT = '`temptab` does not exist', ROW_NUMBER= 105;
> +   END;
> +   SELECT `c` FROM `temptab`;
> +END|
> +
> +DELIMITER ;|
> +
> +--error ER_NO_SUCH_TABLE
> +CALL resignal_syntax();

You test that the above isn't a syntax error.
But you also need to verify that row_number was, indeed, set. Like, do
GET DIAGNOSTICS and print the ROW_NUMBER.

> +
> +DROP PROCEDURE resignal_syntax;
> diff --git a/sql/sql_error.cc b/sql/sql_error.cc
> index a2178831697..84065ddce15 100644
> --- a/sql/sql_error.cc
> +++ b/sql/sql_error.cc
> @@ -190,6 +190,11 @@ static void copy_string(MEM_ROOT *mem_root, String* dst, const String* src)
>      dst->length(0);
>  }
>  
> +void copy_value(ulong *dst, const ulong *src)
> +{
> +  memcpy(dst, src, sizeof(*src));
> +}
> +
>  void
>  Sql_condition::copy_opt_attributes(const Sql_condition *cond)
>  {
> @@ -204,6 +209,7 @@ Sql_condition::copy_opt_attributes(const Sql_condition *cond)
>    copy_string(m_mem_root, & m_table_name, & cond->m_table_name);
>    copy_string(m_mem_root, & m_column_name, & cond->m_column_name);
>    copy_string(m_mem_root, & m_cursor_name, & cond->m_cursor_name);
> +  copy_value(& m_row_number, & cond->m_row_number);

That makes no sense. Other values were strings and had to be strdup-ed,
but that's a number, you can simply do

   m_row_number= cond->m_row_number;

>  }

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx