Re: [Maria-developers] ce5cc8fb905: MDEV-29021 ALTER TABLE fails when a stored virtual column is dropped+added
Hi, Nikita, Sure, as you like On Jul 13, Nikita Malyavin wrote: > I did, in rpl_alter_extra_persistent.test. > > Btw, maybe also rename this file, into something not referring to > PERSISTENT? > > On Wed, 13 Jul 2022 at 23:41, Sergei Golubchik wrote: > > > Hi, Nikita, > > > > Please, add a test case for DEFAULT in replication, something like I > > described below. > > > > On Jul 13, Nikita Malyavin wrote: > > > On Thu, 7 Jul 2022 at 16:05, Sergei Golubchik wrote: > > > > > > > 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) > > > > > > > I'm afraid that we still should mark whose fields for write, or we'll > > > end up broken. > > > So i guess i'll just remove the function and will add a raw cycle > > > through thow fields with marking. > > > There's no error check, so it's not worth a function anymore > > > > Regards, > > Sergei > > VP of MariaDB Server Engineering > > and secur...@mariadb.org > > > > > -- > Yours truly, > Nikita Malyavin 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
Re: [Maria-developers] ce5cc8fb905: MDEV-29021 ALTER TABLE fails when a stored virtual column is dropped+added
I did, in rpl_alter_extra_persistent.test. Btw, maybe also rename this file, into something not referring to PERSISTENT? On Wed, 13 Jul 2022 at 23:41, Sergei Golubchik wrote: > Hi, Nikita, > > Please, add a test case for DEFAULT in replication, something like I > described below. > > On Jul 13, Nikita Malyavin wrote: > > On Thu, 7 Jul 2022 at 16:05, Sergei Golubchik wrote: > > > > > 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) > > > > > I'm afraid that we still should mark whose fields for write, or we'll > > end up broken. > > So i guess i'll just remove the function and will add a raw cycle > > through thow fields with marking. > > There's no error check, so it's not worth a function anymore > > Regards, > Sergei > VP of MariaDB Server Engineering > and secur...@mariadb.org > -- Yours truly, Nikita Malyavin ___ 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
Re: [Maria-developers] ce5cc8fb905: MDEV-29021 ALTER TABLE fails when a stored virtual column is dropped+added
Hi, Nikita, Please, add a test case for DEFAULT in replication, something like I described below. On Jul 13, Nikita Malyavin wrote: > On Thu, 7 Jul 2022 at 16:05, Sergei Golubchik wrote: > > > 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) > > > I'm afraid that we still should mark whose fields for write, or we'll > end up broken. > So i guess i'll just remove the function and will add a raw cycle > through thow fields with marking. > There's no error check, so it's not worth a function anymore 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
Re: [Maria-developers] ce5cc8fb905: MDEV-29021 ALTER TABLE fails when a stored virtual column is dropped+added
On Thu, 7 Jul 2022 at 16:05, Sergei Golubchik wrote: > 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) > > I'm afraid that we still should mark whose fields for write, or we'll end up broken. So i guess i'll just remove the function and will add a raw cycle through thow fields with marking. There's no error check, so it's not worth a function anymore ___ 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
Re: [Maria-developers] ce5cc8fb905: MDEV-29021 ALTER TABLE fails when a stored virtual column is dropped+added
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
