maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr,

On Jan 19, Oleksandr Byelkin wrote:
> revision-id: e4ea1544097 (mariadb-10.2.40-198-ge4ea1544097)
> parent(s): fd7a553cb80
> author: Oleksandr Byelkin
> committer: Oleksandr Byelkin
> timestamp: 2022-01-19 11:12:57 +0100
> message:
> 
> MDEV-17124: mariadb 10.1.34, views and prepared statements:  ERROR 1615 (HY000): Prepared statement needs to be re-prepared
> 
> The problem is that if table definition cache (TDC) is full of real tables
> which are in tables cache, view definition can not stay there so will be
> removed by its own underlying tables.
> In situation above old mechanism of detection matching definition in PS
> and current version always require reprepare and so prevent executing
> the PS.
> 
> One work around is to increase TDC, other - improve version check for
> views/triggers (which is done here). Now in suspicious cases we check:
>  - MD5 of the view to be sure that version really have changed;
>  - time of creation of a trigger related to time of statement preparation.
> 
> diff --git a/mysql-test/r/ps_ddl.result b/mysql-test/r/ps_ddl.result
> index e69c6e06193..005c78a0319 100644
> --- a/mysql-test/r/ps_ddl.result
> +++ b/mysql-test/r/ps_ddl.result
> @@ -214,7 +214,7 @@ new trigger: 10
>  drop trigger t1_bd;
>  set @val=11;
>  execute stmt using @val;
> -call p_verify_reprepare_count(1);
> +call p_verify_reprepare_count(0);
>  SUCCESS
>  
>  select @message;
> @@ -831,8 +831,8 @@ a          b          c
>  20      40         100
>  30      60         150
>  call p_verify_reprepare_count(1);
> -SUCCESS
> -
> +ERROR
> +Expected: 1, actual: 0
>  # Check that we properly handle ALTER VIEW statements.
>  execute stmt;
>  a       b          c
> @@ -882,9 +882,9 @@ a          b          c
>  10      50         60
>  20      100        120
>  30      150        180
> -call p_verify_reprepare_count(1);
> -SUCCESS
> -
> +call p_verify_reprepare_count(0);
> +ERROR
> +Expected: 0, actual: 1
>  execute stmt;
>  a       b          c

you forgot to update the expected number or reprepares in two places.

> diff --git a/sql/parse_file.cc b/sql/parse_file.cc
> index 999f53bd681..3c750f4df29 100644
> --- a/sql/parse_file.cc
> +++ b/sql/parse_file.cc
> @@ -145,6 +145,7 @@ write_parameter(IO_CACHE *file, uchar* base, File_option *parameter)
>  
>    switch (parameter->type) {
>    case FILE_OPTIONS_STRING:
> +  case FILE_OPTIONS_FIXSTRING:
>    {
>      LEX_STRING *val_s= (LEX_STRING *)(base + parameter->offset);
>      if (my_b_write(file, (const uchar *)val_s->str, val_s->length))
> @@ -830,6 +831,22 @@ File_parser::parse(uchar* base, MEM_ROOT *mem_root,
>            }
>            ptr= eol+1;
>            break;
> +        case FILE_OPTIONS_FIXSTRING:
> +        {
> +          /* string have to be allocated already  and length set */
> +          LEX_STRING *val= (LEX_STRING *)(base + parameter->offset);
> +          DBUG_ASSERT(val->length != 0);
> +          if (ptr[val->length] != '\n')
> +          {
> +            my_error(ER_FPARSER_ERROR_IN_PARAMETER, MYF(0),
> +                     parameter->name.str, line);
> +            DBUG_RETURN(TRUE);
> +          }
> +          memcpy(val->str, ptr, val->length);
> +          val->str[val->length]= '\0';
> +          ptr+= (val->length + 1);
> +          break;

I'd suggest to use FILE_OPTIONS_STRING here, it's much simpler and it's
known to work.

This doesn't check that end - ptr < val->length, and it doesn't check
that even if ptr[val->length] == '\n', then it's the first '\n' after
ptr.

It's all possible to fix, but using FILE_OPTIONS_STRING is just easier.

> +        }
>          case FILE_OPTIONS_TIMESTAMP:
>          {
>            /* string have to be allocated already */
> diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc
> index bb1a99d9eef..b9198647f98 100644
> --- a/sql/sql_prepare.cc
> +++ b/sql/sql_prepare.cc
> @@ -4177,6 +4177,10 @@ bool Prepared_statement::prepare(const char *packet, uint packet_len)
>    Query_arena *old_stmt_arena;
>    DBUG_ENTER("Prepared_statement::prepare");
>    DBUG_ASSERT(m_sql_mode == thd->variables.sql_mode);
> +
> +  // The same format as for triggers to compare
> +  create_time= make_trigger_time(thd->query_start(),
> +                                 thd->query_start_sec_part());

Better use hrtime here, you might compare it with more than triggers in
the future.

For triggers, you can multiply it by 10000 for comparison. Or you can
start storing trigger time with a microsecond precision - that's even
better.

>    /*
>      If this is an SQLCOM_PREPARE, we also increase Com_prepare_sql.
>      However, it seems handy if com_stmt_prepare is increased always,
> diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc
> index 52817ef65ae..38febce4fef 100644
> --- a/sql/sql_trigger.cc
> +++ b/sql/sql_trigger.cc
> @@ -929,7 +933,21 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables,
>  
>    if (!sql_create_definition_file(NULL, &file, &triggers_file_type,
>                                    (uchar*)this, triggers_file_parameters))
> +  {
> +    // delay if needed to be sure that time of creation is unique
> +    for (;;)
> +    {
> +      my_hrtime_t hrtime= my_hrtime();
> +      ulonglong current_time= make_trigger_time(hrtime_to_my_time(hrtime),
> +                                                hrtime_sec_part(hrtime));
> +      if (trigger->create_time == current_time)
> +        pthread_yield();
> +      else
> +        break;
> +    }

This shouldn't be needed.
Better

1. store statement->create_time with microsecond precision
2. store trigger->create_time with microsecond precision
3. at the end of prepare wait (like above) to make sure
   statement->create_time is larger than all triggers' create_time.
   this will be a no-op unless someone created a trigger and prepared in
   the same microsecond. Safety check  will ensure you won't wait longer
   than one microsecond.

>      DBUG_RETURN(false);
> +  }
>  
>  err_with_cleanup:
>    /* Delete .TRN file */
> diff --git a/sql/sql_view.cc b/sql/sql_view.cc
> index 020830483c5..830a42b3cf3 100644
> --- a/sql/sql_view.cc
> +++ b/sql/sql_view.cc
> @@ -1131,7 +1141,31 @@ static int mysql_register_view(THD *thd, TABLE_LIST *view,
>    DBUG_RETURN(error);
>  }
>  
> +/**
> +  Check is TABLE_LEST and SHARE match
> +  @param[in]  view                TABLE_LIST of the view
> +  @param[in]  share               Share object of view
> +
> +  @return false on error or misspatch
> +*/
>  
> +bool mariadb_view_version_get(TABLE_SHARE *share)
> +{
> +  DBUG_ASSERT(share->is_view);
> +
> +  if (!(share->tabledef_version.str=
> +        (uchar*) alloc_root(&share->mem_root, VIEW_MD5_LEN + 1)))
> +    return TRUE;
> +  share->tabledef_version.length= VIEW_MD5_LEN;
> +
> +  DBUG_ASSERT(share->view_def != NULL);
> +  if (share->view_def->parse((uchar *) &share->tabledef_version, NULL,
> +                             view_md5_parameters, 1,
> +                             &file_parser_dummy_hook))
> +    return TRUE;
> +  DBUG_ASSERT(share->tabledef_version.length == VIEW_MD5_LEN);
> +  return FALSE;
> +}

I don't like using md5 as a uuid. md5 collisions are very much possible.
Better options would be
1. use a timestamp, but store it with microsecond precisions, same logic
   as with triggers.
2. use a true tabledef uuid, new field in the view frm, same logic as
   with tables.

timestamp is better, because it already exists in frms, and any old view
will naturally have a timestamp before now(), so it doesn't matter that
it won't have microseconds.

uuid is better, because it's more universal, can be used for more than
just reprepare checks.

But for now I think timestamp benefits outweigh that.

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