maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nikita,

This is good, but I think fill_extra_persistent_columns() shouldn't be
used at all.

It doesn't handle default values. I've changed rpl_alter_extra_persistent test
as

-alter table t1 add column z1 int as(a+1) virtual, add column z2 int as (a+2) persistent;
+alter table t1 add column z1 int as(a+1) virtual, add column z2 int default (a+2);

and, of course, new column z2 wasn't updated properly.
An easy fix would be to use your code in all cases and remove
fill_extra_persistent_columns().

Could you do that too, please? And add the test case too. May be just,
append

   , add column z3 int default (a+2)

On Jul 07, Nikita Malyavin wrote:
> revision-id: ce5cc8fb905 (mariadb-10.6.1-506-gce5cc8fb905)
> parent(s): 49ad875902e
> author: Nikita Malyavin
> committer: Nikita Malyavin
> timestamp: 2022-07-04 15:55:03 +0300
> message:
> 
> MDEV-29021 ALTER TABLE fails when a stored virtual column is dropped+added
> 
> We shouldn't rely on `fill_extra_persistent_columns`, as it only updates
> fields which have an index > cols->n_bits (replication bitmap width).
> 
> Normal update_virtual_fields+update_default_fields shoudl be done.
> 
> diff --git a/sql/rpl_record.cc b/sql/rpl_record.cc
> index 17731e64600..42f476fc112 100644
> --- a/sql/rpl_record.cc
> +++ b/sql/rpl_record.cc
> @@ -412,6 +412,26 @@ int unpack_row(rpl_group_info *rgi, TABLE *table, uint const colcnt,
>      {
>        copy->do_copy(copy);
>      }
> +    if (table->default_field)
> +    {
> +      error= table->update_default_fields(table->in_use->lex->ignore);
> +      if (unlikely(error))
> +        DBUG_RETURN(error);
> +    }
> +    if (table->vfield)
> +    {
> +      error= table->update_virtual_fields(table->file, VCOL_UPDATE_FOR_WRITE);
> +      if (unlikely(error))
> +        DBUG_RETURN(error);
> +    }
> +  }
> +  else
> +  {
> +    /*
> +      Add Extra slave persistent columns
> +    */
> +    if (unlikely(error= fill_extra_persistent_columns(table, cols->n_bits)))
> +      DBUG_RETURN(error);
>    }
>  
>    /*
> @@ -439,12 +459,6 @@ int unpack_row(rpl_group_info *rgi, TABLE *table, uint const colcnt,
>      }
>    }
>  
> -  /*
> -    Add Extra slave persistent columns
> -  */
> -  if (unlikely(error= fill_extra_persistent_columns(table, cols->n_bits)))
> -    DBUG_RETURN(error);
> -
>    /*
>      We should now have read all the null bytes, otherwise something is
>      really wrong.

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