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 secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
