Re: [Maria-developers] dc2ace70f1b: Syntax with MIGRATE keyword
Sergei, On Wed, Sep 1, 2021 at 10:35 PM Sergei Golubchik wrote: > > Hi, Aleksey! > > On Sep 01, Aleksey Midenkov wrote: > > On Wed, Sep 1, 2021 at 10:13 PM Sergei Golubchik wrote: > > > On Sep 01, Aleksey Midenkov wrote: > > > > > > > > > > > > +move_out_partition: > > > > > > +MIGRATE_SYM PARTITION_SYM > > > > > > +| MIGRATE_SYM OUT_SYM PARTITION_SYM > > > > > > > > > > Where does this OUT_SYM came out from? > > > > > It doesn't add any values, it doesn't make the syntax more natural. > > > > > Why did you add it? > > > > > > > > I think that helps to make syntax more natural if we do MIGRATE > > > > PARTITION in both directions. > > > > If it is written MIGRATE OUT or MIGRATE IN that is easier to > > > > understand what's going on. FROM/TO in the end is not helping much > > > > because it is harder to notice. Partition specification presence or > > > > absence: not so explicit to understand quickly. > > > > > > It incorrect English. And it's a noise word that adds no value and > > > doesn't make anything easier to understand. > > > > > > MIGRATE PARTITION x TO TABLE y > > > > > > is correct and unambigous although not a very traditional use of the > > > word "migrate". MIGRATE OUT PARTITION x TO TABLE y is just wrong. > > > > If there is "move in" and "move out", "migrate in" and "migrate out" > > cannot be wrong. Look for examples for "migrate out to" and "migrate > > in from". > > Move would be a correct word here, as migrate is normally an > intransitive word. Anyway, "out" is used similarly for them, it'd be > >MIGRATE PARTITION x OUT TO TABLE y > The above is not helping much. Ok, let's remove OUT. > is that what you want? This looks kind of redundant, but I cannot say it > is wrong. But MIGRATE OUT PARTITION x TO TABLE y - this is clearly > broken. > > Regards, > Sergei > VP of MariaDB Server Engineering > and secur...@mariadb.org -- All the best, Aleksey Midenkov @midenok ___ 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] 6768e3a2830: MDEV-22166 MIGRATE PARTITION: move out partition into a table
Sergei, On Wed, Sep 1, 2021 at 10:20 PM Sergei Golubchik wrote: > > Hi, Aleksey! > > On Sep 01, Aleksey Midenkov wrote: > > > > > > > > Looks like IF_DBUG is superfluous macro and should be replaced by > > > > > > > > #ifndef DBUG_OFF > > > > #endif > > > > > > No, it's used in expression. Precisely, to avoid ifdefs. > > > > So, what about DBUG(A) variant? > > We have IF_XXX for many different XXX. IF_DBUG was created to follow > this convention. Anytime you see a macro IF_XXX you know what it does. > Let's keep it that way. We also use DBUG_ prefix for debug things. IF_DBUG() scheme is superfluous as only a1 used in most cases. So if renamed to DBUG_DO() or just to DBUG() the scheme is not broken and the code looks nicer. Please have a glance at the patch attached. > > > > > > > About your ER_KEY_COLUMN_DOES_NOT_EXITS replacement: > > > > > > $ grep -c DOES_NOT_EXIST sql/share/errmsg-utf8.txt > > > 6 > > > $ grep -c NOT_EXIST sql/share/errmsg-utf8.txt > > > 7 > > > > > > So only one existing error message uses NOT_EXIST without DOES. > > > Let's keep the conventional naming. So, it should be > > > > > >ER_KEY_DOES_NOT_EXIST > > >ER_KEY_COLUMN_DOES_NOT_EXIST > > >ER_PARTITION_DOES_NOT_EXIST > > >ER_REORG_PARTITION_DOES_NOT_EXIST > > > > That is longer by the whole useless word... > > It correct grammar. Messages looking bad otherwise. But we are talking only about code names and they are not visible to a user, are they? Even SQL itself does use the short form "NOT EXISTS". :) I don't see any problems here. > > Or, better, don't rename error messages at all and > preserve the compatibility with older applications. > > Regards, > Sergei > VP of MariaDB Server Engineering > and secur...@mariadb.org -- All the best, Aleksey Midenkov @midenok u.diff.gz Description: application/gzip ___ 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] dc2ace70f1b: Syntax with MIGRATE keyword
Hi, Aleksey! On Sep 01, Aleksey Midenkov wrote: > On Wed, Sep 1, 2021 at 10:13 PM Sergei Golubchik wrote: > > On Sep 01, Aleksey Midenkov wrote: > > > > > > > > > > +move_out_partition: > > > > > +MIGRATE_SYM PARTITION_SYM > > > > > +| MIGRATE_SYM OUT_SYM PARTITION_SYM > > > > > > > > Where does this OUT_SYM came out from? > > > > It doesn't add any values, it doesn't make the syntax more natural. > > > > Why did you add it? > > > > > > I think that helps to make syntax more natural if we do MIGRATE > > > PARTITION in both directions. > > > If it is written MIGRATE OUT or MIGRATE IN that is easier to > > > understand what's going on. FROM/TO in the end is not helping much > > > because it is harder to notice. Partition specification presence or > > > absence: not so explicit to understand quickly. > > > > It incorrect English. And it's a noise word that adds no value and > > doesn't make anything easier to understand. > > > > MIGRATE PARTITION x TO TABLE y > > > > is correct and unambigous although not a very traditional use of the > > word "migrate". MIGRATE OUT PARTITION x TO TABLE y is just wrong. > > If there is "move in" and "move out", "migrate in" and "migrate out" > cannot be wrong. Look for examples for "migrate out to" and "migrate > in from". Move would be a correct word here, as migrate is normally an intransitive word. Anyway, "out" is used similarly for them, it'd be MIGRATE PARTITION x OUT TO TABLE y is that what you want? This looks kind of redundant, but I cannot say it is wrong. But MIGRATE OUT PARTITION x TO TABLE y - this is clearly broken. 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] dc2ace70f1b: Syntax with MIGRATE keyword
Sergei, On Wed, Sep 1, 2021 at 10:13 PM Sergei Golubchik wrote: > > Hi, Aleksey! > > On Sep 01, Aleksey Midenkov wrote: > > > > > > > > +move_out_partition: > > > > +MIGRATE_SYM PARTITION_SYM > > > > +| MIGRATE_SYM OUT_SYM PARTITION_SYM > > > > > > Where does this OUT_SYM came out from? > > > It doesn't add any values, it doesn't make the syntax more natural. > > > Why did you add it? > > > > I think that helps to make syntax more natural if we do MIGRATE > > PARTITION in both directions. > > If it is written MIGRATE OUT or MIGRATE IN that is easier to > > understand what's going on. FROM/TO in the end is not helping much > > because it is harder to notice. Partition specification presence or > > absence: not so explicit to understand quickly. > > It incorrect English. And it's a noise word that adds no value and > doesn't make anything easier to understand. > > MIGRATE PARTITION x TO TABLE y > > is correct and unambigous although not a very traditional use of the > word "migrate". MIGRATE OUT PARTITION x TO TABLE y is just wrong. > If there is "move in" and "move out", "migrate in" and "migrate out" cannot be wrong. Look for examples for "migrate out to" and "migrate in from". > > > > +; > > > > + > > > > +to_table: > > > > +TO_SYM TABLE_SYM > > > > +| TO_SYM > > > > > > Let's keep the TABLE_SYM. Saying > > > > > >MIGRATE PARTITION p1 TO p2 > > > > > > is very vague, the syntax should explicitly say that it migrates a > > > PARTITION to a TABLE. The statement mentions many different types of > > > objects, if should be clear about what object type every identifier > > > refers to. > > > > That is a shortcut for easier typing and that is precious for > > command-line work! If some names can make misunderstanding one can > > always write > > > > MIGRATE PARTITION p1 TO TABLE p2 > > > > Please do not restrict user freedom to choose between verbosity and > > brevity. I guess you have a preference for perl over python (and the > > democracy over dictatorship). > > I do. But it's SQL, not perl. SQL is very verbose even when it isn't > really justified. And here is is justified, so let's keep the syntax > unambigous. And why this is good and should be followed? There are shortcuts in SQL too. Okay, let's keep this unambiguous for a while. > > Regards, > Sergei > VP of MariaDB Server Engineering > and secur...@mariadb.org -- All the best, Aleksey Midenkov @midenok ___ 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] 6768e3a2830: MDEV-22166 MIGRATE PARTITION: move out partition into a table
Hi, Aleksey! On Sep 01, Aleksey Midenkov wrote: > > > > > > Looks like IF_DBUG is superfluous macro and should be replaced by > > > > > > #ifndef DBUG_OFF > > > #endif > > > > No, it's used in expression. Precisely, to avoid ifdefs. > > So, what about DBUG(A) variant? We have IF_XXX for many different XXX. IF_DBUG was created to follow this convention. Anytime you see a macro IF_XXX you know what it does. Let's keep it that way. > > > > About your ER_KEY_COLUMN_DOES_NOT_EXITS replacement: > > > > $ grep -c DOES_NOT_EXIST sql/share/errmsg-utf8.txt > > 6 > > $ grep -c NOT_EXIST sql/share/errmsg-utf8.txt > > 7 > > > > So only one existing error message uses NOT_EXIST without DOES. > > Let's keep the conventional naming. So, it should be > > > >ER_KEY_DOES_NOT_EXIST > >ER_KEY_COLUMN_DOES_NOT_EXIST > >ER_PARTITION_DOES_NOT_EXIST > >ER_REORG_PARTITION_DOES_NOT_EXIST > > That is longer by the whole useless word... It correct grammar. Messages looking bad otherwise. Or, better, don't rename error messages at all and preserve the compatibility with older applications. 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] dc2ace70f1b: Syntax with MIGRATE keyword
Hi, Aleksey! On Sep 01, Aleksey Midenkov wrote: > > > > > > +move_out_partition: > > > +MIGRATE_SYM PARTITION_SYM > > > +| MIGRATE_SYM OUT_SYM PARTITION_SYM > > > > Where does this OUT_SYM came out from? > > It doesn't add any values, it doesn't make the syntax more natural. > > Why did you add it? > > I think that helps to make syntax more natural if we do MIGRATE > PARTITION in both directions. > If it is written MIGRATE OUT or MIGRATE IN that is easier to > understand what's going on. FROM/TO in the end is not helping much > because it is harder to notice. Partition specification presence or > absence: not so explicit to understand quickly. It incorrect English. And it's a noise word that adds no value and doesn't make anything easier to understand. MIGRATE PARTITION x TO TABLE y is correct and unambigous although not a very traditional use of the word "migrate". MIGRATE OUT PARTITION x TO TABLE y is just wrong. > > > +; > > > + > > > +to_table: > > > +TO_SYM TABLE_SYM > > > +| TO_SYM > > > > Let's keep the TABLE_SYM. Saying > > > >MIGRATE PARTITION p1 TO p2 > > > > is very vague, the syntax should explicitly say that it migrates a > > PARTITION to a TABLE. The statement mentions many different types of > > objects, if should be clear about what object type every identifier > > refers to. > > That is a shortcut for easier typing and that is precious for > command-line work! If some names can make misunderstanding one can > always write > > MIGRATE PARTITION p1 TO TABLE p2 > > Please do not restrict user freedom to choose between verbosity and > brevity. I guess you have a preference for perl over python (and the > democracy over dictatorship). I do. But it's SQL, not perl. SQL is very verbose even when it isn't really justified. And here is is justified, so let's keep the syntax unambigous. 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] 6768e3a2830: MDEV-22166 MIGRATE PARTITION: move out partition into a table
Sergei, On Wed, Sep 1, 2021 at 9:07 PM Sergei Golubchik wrote: > > Hi, Aleksey! > > On Sep 01, Aleksey Midenkov wrote: > > > > > > We have a series of IF_xxx(A,B) macros, IF_PARTITIONING, IF_WIN, etc. > > > IF_DBUG(A,B) expands to A if dbug is compiled in, otherwise into B. > > > > > > having IF_DBUG(A,B) and DBUG_IF(keyword) might be confusing. > > > > Looks like IF_DBUG is superfluous macro and should be replaced by > > > > #ifndef DBUG_OFF > > #endif > > No, it's used in expression. Precisely, to avoid ifdefs. So, what about DBUG(A) variant? > > > > > > > > > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > > > > > > > > index 85d880cbbb4..4172a61812e 100644 > > > > > > > > --- a/sql/sql_partition.cc > > > > > > > > +++ b/sql/sql_partition.cc > > > > > > > > Renamed this error code and ugly ER_KEY_COLUMN_DOES_NOT_EXITS > > > > Changed the message. > > > > > > Unfortunatey ER_xxx constants are generally part of the API, client > > > applications do > > > > > >if (mysql_errno() == ER_KEY_COLUMN_DOES_NOT_EXITS) ... > > > > > > so renaming cannot be done lightly. Only if really unavoidable and then > > > with a compatibility fallback, like we have now in mysql.h: > > > > > > #define ER_WARN_DATA_TRUNCATED WARN_DATA_TRUNCATED > > > #define WARN_PLUGIN_DELETE_BUILTIN ER_PLUGIN_DELETE_BUILTIN > > > #define ER_FK_DUP_NAME ER_DUP_CONSTRAINT_NAME > > > ... > > > > I'd make that anyway with that proxy stuff in mysql.h. > > C++14 can mark variables deprecated. That can help throw away > > deprecated constants after some period of time. > > Those are defines, I don't know if you can mark them deprecated. > > About your ER_KEY_COLUMN_DOES_NOT_EXITS replacement: > > $ grep -c DOES_NOT_EXIST sql/share/errmsg-utf8.txt > 6 > $ grep -c NOT_EXIST sql/share/errmsg-utf8.txt > 7 > > So only one existing error message uses NOT_EXIST without DOES. > Let's keep the conventional naming. So, it should be > >ER_KEY_DOES_NOT_EXIST >ER_KEY_COLUMN_DOES_NOT_EXIST >ER_PARTITION_DOES_NOT_EXIST >ER_REORG_PARTITION_DOES_NOT_EXIST That is longer by the whole useless word... > > Regards, > Sergei > VP of MariaDB Server Engineering > and secur...@mariadb.org -- All the best, Aleksey Midenkov @midenok ___ 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] 6768e3a2830: MDEV-22166 MIGRATE PARTITION: move out partition into a table
Hi, Aleksey! On Sep 01, Aleksey Midenkov wrote: > > > > We have a series of IF_xxx(A,B) macros, IF_PARTITIONING, IF_WIN, etc. > > IF_DBUG(A,B) expands to A if dbug is compiled in, otherwise into B. > > > > having IF_DBUG(A,B) and DBUG_IF(keyword) might be confusing. > > Looks like IF_DBUG is superfluous macro and should be replaced by > > #ifndef DBUG_OFF > #endif No, it's used in expression. Precisely, to avoid ifdefs. > > > > > > > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > > > > > > > index 85d880cbbb4..4172a61812e 100644 > > > > > > > --- a/sql/sql_partition.cc > > > > > > > +++ b/sql/sql_partition.cc > > > > > > Renamed this error code and ugly ER_KEY_COLUMN_DOES_NOT_EXITS > > > Changed the message. > > > > Unfortunatey ER_xxx constants are generally part of the API, client > > applications do > > > >if (mysql_errno() == ER_KEY_COLUMN_DOES_NOT_EXITS) ... > > > > so renaming cannot be done lightly. Only if really unavoidable and then > > with a compatibility fallback, like we have now in mysql.h: > > > > #define ER_WARN_DATA_TRUNCATED WARN_DATA_TRUNCATED > > #define WARN_PLUGIN_DELETE_BUILTIN ER_PLUGIN_DELETE_BUILTIN > > #define ER_FK_DUP_NAME ER_DUP_CONSTRAINT_NAME > > ... > > I'd make that anyway with that proxy stuff in mysql.h. > C++14 can mark variables deprecated. That can help throw away > deprecated constants after some period of time. Those are defines, I don't know if you can mark them deprecated. About your ER_KEY_COLUMN_DOES_NOT_EXITS replacement: $ grep -c DOES_NOT_EXIST sql/share/errmsg-utf8.txt 6 $ grep -c NOT_EXIST sql/share/errmsg-utf8.txt 7 So only one existing error message uses NOT_EXIST without DOES. Let's keep the conventional naming. So, it should be ER_KEY_DOES_NOT_EXIST ER_KEY_COLUMN_DOES_NOT_EXIST ER_PARTITION_DOES_NOT_EXIST ER_REORG_PARTITION_DOES_NOT_EXIST 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] dc2ace70f1b: Syntax with MIGRATE keyword
Sergei,
On Wed, Sep 1, 2021 at 8:21 PM Sergei Golubchik wrote:
>
> Hi, Aleksey!
>
> On Sep 01, Aleksey Midenkov wrote:
> > revision-id: dc2ace70f1b (mariadb-10.6.1-68-gdc2ace70f1b)
> > parent(s): 19fbfab084f
> > author: Aleksey Midenkov
> > committer: Aleksey Midenkov
> > timestamp: 2021-08-30 22:37:08 +0300
> > message:
> >
> > Syntax with MIGRATE keyword
> >
> > ALTER TABLE tbl_name
> > [alter_option [, alter_option] ...] |
> > [partition_options]
> >
> > partition_option: {
> > ...
> > | MIGRATE [OUT] PARTITION partition_name TO [TABLE] tbl_name
> > }
> >
> > Examples:
> >
> > ALTER TABLE t1 MIGRATE PARTITION p1 TO tp1;
> > ALTER TABLE t1 MIGRATE PARTITION p2 TO TABLE tp2;
> > ALTER TABLE t1 MIGRATE OUT PARTITION p3 TO TABLE tp3;
> >
> > diff --git a/mysql-test/suite/parts/r/alter_table.result
> > b/mysql-test/suite/parts/r/alter_table.result
> > index bc1bf661d4c..7a2d45c51c1 100644
> > --- a/mysql-test/suite/parts/r/alter_table.result
> > +++ b/mysql-test/suite/parts/r/alter_table.result
> > @@ -258,9 +258,46 @@ show grants for current_user;
> > Grants for alan@%
> > GRANT USAGE ON *.* TO `alan`@`%`
> > GRANT INSERT, CREATE, DROP, ALTER ON `test`.* TO `alan`@`%`
> > -alter table t1 extract partition p1 as table tp1;
> > +alter table t1 migrate partition p1 to table tp1;
> > disconnect alan;
> > connection default;
> > drop database EXISTENT;
> > drop user alan;
> > drop tables t1, tp1, tp2, tp4;
> > +# Cunning syntax
>
> what's so cunning here?
>
> > +create or replace table t1 (x int)
> > +partition by range(x) (
> > diff --git a/sql/handler.h b/sql/handler.h
> > index c455625aafe..7ac9e929ecc 100644
> > --- a/sql/handler.h
> > +++ b/sql/handler.h
> > @@ -824,7 +824,7 @@ typedef bool Log_func(THD*, TABLE*, bool, const uchar*,
> > const uchar*);
> > #define ALTER_PARTITION_TRUNCATE(1ULL << 11)
> > // Set for REORGANIZE PARTITION
> > #define ALTER_PARTITION_TABLE_REORG (1ULL << 12)
> > -#define ALTER_PARTITION_EXTRACT(1ULL << 13)
> > +#define ALTER_PARTITION_MIGRATE_OUT(1ULL << 13)
> >
> > /*
> >This is master database for most of system tables. However there
> > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> > index 49dd244f2ff..e1be6ea9557 100644
> > --- a/sql/sql_yacc.yy
> > +++ b/sql/sql_yacc.yy
> > @@ -7492,6 +7492,16 @@ ident_or_empty:
> > | ident
> > ;
> >
> > +move_out_partition:
> > +MIGRATE_SYM PARTITION_SYM
> > +| MIGRATE_SYM OUT_SYM PARTITION_SYM
>
> Where does this OUT_SYM came out from?
> It doesn't add any values, it doesn't make the syntax more natural.
> Why did you add it?
I think that helps to make syntax more natural if we do MIGRATE
PARTITION in both directions.
If it is written MIGRATE OUT or MIGRATE IN that is easier to
understand what's going on. FROM/TO in the end is not helping much
because it is harder to notice. Partition specification presence or
absence: not so explicit to understand quickly.
>
> > +;
> > +
> > +to_table:
> > +TO_SYM TABLE_SYM
> > +| TO_SYM
>
> Let's keep the TABLE_SYM. Saying
>
>MIGRATE PARTITION p1 TO p2
>
> is very vague, the syntax should explicitly say that it migrates a
> PARTITION to a TABLE. The statement mentions many different types of
> objects, if should be clear about what object type every identifier
> refers to.
That is a shortcut for easier typing and that is precious for
command-line work! If some names can make misunderstanding one can
always write
MIGRATE PARTITION p1 TO TABLE p2
Please do not restrict user freedom to choose between verbosity and
brevity. I guess you have a preference for perl over python (and the
democracy over dictatorship).
>
> > +;
> > +
> > alter_commands:
> >/* empty */
> > | DISCARD TABLESPACE
> > @@ -7619,15 +7629,15 @@ alter_commands:
> >MYSQL_YYABORT;
> > Lex->alter_info.partition_flags|= ALTER_PARTITION_EXCHANGE;
> >}
> > -| EXTRACT_SYM PARTITION_SYM alt_part_name_item
> > - AS TABLE_SYM table_ident have_partitioning
> > +| move_out_partition alt_part_name_item
> > + to_table table_ident have_partitioning
>
> please, let it be just
>
> MIGRATE_SYM PARTITION_SYM alt_part_name_item
> TO_SYM TABLE_SYM table_ident have_partitioning
>
> >{
> > -if (Lex->stmt_alter_table($6))
> > +if (Lex->stmt_alter_table($4))
> >MYSQL_YYABORT;
> > Lex->m_sql_cmd= new (thd->mem_root) Sql_cmd_alter_table();
> > if (unlikely(Lex->m_sql_cmd == NULL))
> >MYSQL_YYABORT;
> > -Lex->alter_info.partition_flags|= ALTER_PARTITION_EXTRACT;
> > +Lex->alter_info.partition_flags|= ALTER_PARTITION_MIGRATE_OUT;
> >}
> > ;
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org
--
All the best,
Aleksey Midenkov
Re: [Maria-developers] 6768e3a2830: MDEV-22166 MIGRATE PARTITION: move out partition into a table
Hi, Aleksey! On Aug 27, Aleksey Midenkov wrote: > > > > I still don't quite understand why did you introduce a new method > > with frm backup and a (low probability) window where it won't clean > > up. > > > > Isn't it similar to normal ALTER TABLE? It also needs to replace > > frms and it is solved with ddl log - if a crash happens after the > > point of no return (after the frm was replaced with a new one), ddl > > log will binlog the query on recovery. > > > > Could you do the same, like > > > > case DDL_LOCK_MIGRATE_PARTITION_ACTION: > > if (shadow frm no longer exists && xid not in binlog) > > write_bin_log(...); > > > > would that work? Or would partitioning-specific old ddl logging code > > interfere? > > I believe ALTER TABLE atomicity is not the perfect one in respect of > rollback on error so why should that be an example for me? let's start from a statement. You're stating that ALTER TABLE atomicity is not the perfect one in respect of rollback on error. What do you mean by that? Can you show how ALTER TABLE wouldn't be atomic after a rollback on an error? > Another issue with the scheme you proposed is worse complexity and > reliability. It's the scheme that ALTER TABLE and other DDL statements are using now. If you have something simpler and more reliable, could you please describe it so that we could change all DDL statements to use your approach? > Why do I have to do some vague logic about binlogging if it can be > straightforward and simple? So either I have to expand frm handling or > add another DDL log action: why are you asking me not to do the former > and do the latter? Because - as far as I understand - the latter is what all other DDL statements use, except old partitioning statements that weren't converted yet. we really do not want *three* different approaches to atomicity in the same ALTER TABLE statement. It's just not maintainable. But perhaps I misunderstood and DDL atomicity was implemented differently from what I've described? Then I'm sorry, I didn't intend to suggest that you should implement yet another approach to DDL atomicity, I was saying, please, do what other statements do. 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] 6768e3a2830: MDEV-22166 MIGRATE PARTITION: move out partition into a table
Hi, Sergei! On Wed, Sep 1, 2021 at 2:58 PM Sergei Golubchik wrote: > > Hi, Aleksey! > > On Aug 30, Aleksey Midenkov wrote: > > Hi Sergei! > > > > Updated bb-10.7-midenok-MDEV-22166 is d4668e7254c6 > > I'll look right after replying to this email. > > > > Okay, indeed, it seems that DBUG_EVALUATE_IF is almost always used with > > > one of the arguments being 0 or 1. If you replace all > > > DBUG_EVALUATE_IF's, let's just remove it completely. > > > Note, that DBUG_TRUE_IF can be defined simply as > > > > > > #define DBUG_TRUE_IF(keyword) _db_keyword_(0, (keyword), 1) > > > > > > so may be you'd like to call id DBUG_KEYWORD_IF or something? > > > Although DBUG_TRUE_IF is shorter :) > > > > It turned out DBUG_IF() is even more short! ;) > > We have a series of IF_xxx(A,B) macros, IF_PARTITIONING, IF_WIN, etc. > IF_DBUG(A,B) expands to A if dbug is compiled in, otherwise into B. > > having IF_DBUG(A,B) and DBUG_IF(keyword) might be confusing. > Looks like IF_DBUG is superfluous macro and should be replaced by #ifndef DBUG_OFF #endif But for some code like here IF_DBUG(uchar const *const old_pack_ptr= pack_ptr;,) I'd make DBUG(uchar const *const old_pack_ptr= pack_ptr); So DBUG(A) macro would be a one-liner for #ifndef DBUG_OFF. > > > > > > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > > > > > > index 85d880cbbb4..4172a61812e 100644 > > > > > > --- a/sql/sql_partition.cc > > > > > > +++ b/sql/sql_partition.cc > > > > Renamed this error code and ugly ER_KEY_COLUMN_DOES_NOT_EXITS > > Changed the message. > > Unfortunatey ER_xxx constants are generally part of the API, client > applications do > >if (mysql_errno() == ER_KEY_COLUMN_DOES_NOT_EXITS) ... > > so renaming cannot be done lightly. Only if really unavoidable and then > with a compatibility fallback, like we have now in mysql.h: > > #define ER_WARN_DATA_TRUNCATED WARN_DATA_TRUNCATED > #define WARN_PLUGIN_DELETE_BUILTIN ER_PLUGIN_DELETE_BUILTIN > #define ER_FK_DUP_NAME ER_DUP_CONSTRAINT_NAME > ... > I'd make that anyway with that proxy stuff in mysql.h. C++14 can mark variables deprecated. That can help throw away deprecated constants after some period of time. > > > > > > goto err; > > > > > >} > > Regards, > Sergei > VP of MariaDB Server Engineering > and secur...@mariadb.org -- All the best, Aleksey Midenkov @midenok ___ 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] 19fbfab084f: Compilation fix
Hi, Aleksey!
On Sep 01, Aleksey Midenkov wrote:
> On Wed, Sep 1, 2021 at 3:46 PM Sergei Golubchik wrote:
>
> Here
>
> #define DBUG_SUICIDE() do { } while(0)
>
> DBUG_SUICIDE is not an expression. It's a statement and cannot be used
> in expression.
But that's a bug. Every DBUG_xxx macro is defined at least twice, for
dbug and non-dbug builds. All definitions must be either expressions or
statements, it cannot arbitrary switch from a statement to an
expression or vice versa.
In debug builds it's defined as
#define DBUG_SUICIDE() (_db_flush_(), _db_suicide_())
so it must be defined as an expression in non-dbug builds too.
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] dc2ace70f1b: Syntax with MIGRATE keyword
Hi, Aleksey!
On Sep 01, Aleksey Midenkov wrote:
> revision-id: dc2ace70f1b (mariadb-10.6.1-68-gdc2ace70f1b)
> parent(s): 19fbfab084f
> author: Aleksey Midenkov
> committer: Aleksey Midenkov
> timestamp: 2021-08-30 22:37:08 +0300
> message:
>
> Syntax with MIGRATE keyword
>
> ALTER TABLE tbl_name
> [alter_option [, alter_option] ...] |
> [partition_options]
>
> partition_option: {
> ...
> | MIGRATE [OUT] PARTITION partition_name TO [TABLE] tbl_name
> }
>
> Examples:
>
> ALTER TABLE t1 MIGRATE PARTITION p1 TO tp1;
> ALTER TABLE t1 MIGRATE PARTITION p2 TO TABLE tp2;
> ALTER TABLE t1 MIGRATE OUT PARTITION p3 TO TABLE tp3;
>
> diff --git a/mysql-test/suite/parts/r/alter_table.result
> b/mysql-test/suite/parts/r/alter_table.result
> index bc1bf661d4c..7a2d45c51c1 100644
> --- a/mysql-test/suite/parts/r/alter_table.result
> +++ b/mysql-test/suite/parts/r/alter_table.result
> @@ -258,9 +258,46 @@ show grants for current_user;
> Grants for alan@%
> GRANT USAGE ON *.* TO `alan`@`%`
> GRANT INSERT, CREATE, DROP, ALTER ON `test`.* TO `alan`@`%`
> -alter table t1 extract partition p1 as table tp1;
> +alter table t1 migrate partition p1 to table tp1;
> disconnect alan;
> connection default;
> drop database EXISTENT;
> drop user alan;
> drop tables t1, tp1, tp2, tp4;
> +# Cunning syntax
what's so cunning here?
> +create or replace table t1 (x int)
> +partition by range(x) (
> diff --git a/sql/handler.h b/sql/handler.h
> index c455625aafe..7ac9e929ecc 100644
> --- a/sql/handler.h
> +++ b/sql/handler.h
> @@ -824,7 +824,7 @@ typedef bool Log_func(THD*, TABLE*, bool, const uchar*,
> const uchar*);
> #define ALTER_PARTITION_TRUNCATE(1ULL << 11)
> // Set for REORGANIZE PARTITION
> #define ALTER_PARTITION_TABLE_REORG (1ULL << 12)
> -#define ALTER_PARTITION_EXTRACT(1ULL << 13)
> +#define ALTER_PARTITION_MIGRATE_OUT(1ULL << 13)
>
> /*
>This is master database for most of system tables. However there
> diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> index 49dd244f2ff..e1be6ea9557 100644
> --- a/sql/sql_yacc.yy
> +++ b/sql/sql_yacc.yy
> @@ -7492,6 +7492,16 @@ ident_or_empty:
> | ident
> ;
>
> +move_out_partition:
> +MIGRATE_SYM PARTITION_SYM
> +| MIGRATE_SYM OUT_SYM PARTITION_SYM
Where does this OUT_SYM came out from?
It doesn't add any values, it doesn't make the syntax more natural.
Why did you add it?
> +;
> +
> +to_table:
> +TO_SYM TABLE_SYM
> +| TO_SYM
Let's keep the TABLE_SYM. Saying
MIGRATE PARTITION p1 TO p2
is very vague, the syntax should explicitly say that it migrates a
PARTITION to a TABLE. The statement mentions many different types of
objects, if should be clear about what object type every identifier
refers to.
> +;
> +
> alter_commands:
>/* empty */
> | DISCARD TABLESPACE
> @@ -7619,15 +7629,15 @@ alter_commands:
>MYSQL_YYABORT;
> Lex->alter_info.partition_flags|= ALTER_PARTITION_EXCHANGE;
>}
> -| EXTRACT_SYM PARTITION_SYM alt_part_name_item
> - AS TABLE_SYM table_ident have_partitioning
> +| move_out_partition alt_part_name_item
> + to_table table_ident have_partitioning
please, let it be just
MIGRATE_SYM PARTITION_SYM alt_part_name_item
TO_SYM TABLE_SYM table_ident have_partitioning
>{
> -if (Lex->stmt_alter_table($6))
> +if (Lex->stmt_alter_table($4))
>MYSQL_YYABORT;
> Lex->m_sql_cmd= new (thd->mem_root) Sql_cmd_alter_table();
> if (unlikely(Lex->m_sql_cmd == NULL))
>MYSQL_YYABORT;
> -Lex->alter_info.partition_flags|= ALTER_PARTITION_EXTRACT;
> +Lex->alter_info.partition_flags|= ALTER_PARTITION_MIGRATE_OUT;
>}
> ;
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] 19fbfab084f: Compilation fix
Hi Sergei!
On Wed, Sep 1, 2021 at 3:46 PM Sergei Golubchik wrote:
>
> Hi, Aleksey!
>
> On Sep 01, Aleksey Midenkov wrote:
> > revision-id: 19fbfab084f (mariadb-10.6.1-67-g19fbfab084f)
> > parent(s): 8009680fbc3
> > author: Aleksey Midenkov
> > committer: Aleksey Midenkov
> > timestamp: 2021-08-30 22:02:40 +0300
> > message:
> >
> > Compilation fix
> >
> > diff --git a/sql/sql_table.h b/sql/sql_table.h
> > index 83e88c82f0e..8cb9ebe0445 100644
> > --- a/sql/sql_table.h
> > +++ b/sql/sql_table.h
> > @@ -20,8 +20,14 @@
> > #include // pthread_mutex_t
> > #include "m_string.h" // LEX_CUSTRING
> >
> > +inline bool suicide()
> > +{
> > + DBUG_SUICIDE();
> > + return false;
> > +}
> > +
> > #define ERROR_INJECT_CRASH(code) \
> > - (DBUG_IF(code) && (DBUG_SUICIDE(), 0))
> > + (DBUG_IF(code) && suicide())
>
> What did that change? A compiler didn't like (void,0) ?
Here
#define DBUG_SUICIDE() do { } while(0)
DBUG_SUICIDE is not an expression. It's a statement and cannot be used
in expression.
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org
--
All the best,
Aleksey Midenkov
@midenok
___
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] 87953156c76: Vanilla cleanup: DBUG_EVALUATE_IF() macro replaced by shorter form DBUG_IF()
Hi, Aleksey!
On Sep 01, Aleksey Midenkov wrote:
> revision-id: 87953156c76 (mariadb-10.6.1-61-g87953156c76)
> parent(s): b67fe0c009d
> author: Aleksey Midenkov
> committer: Aleksey Midenkov
> timestamp: 2021-08-30 21:24:35 +0300
> message:
>
> Vanilla cleanup: DBUG_EVALUATE_IF() macro replaced by shorter form DBUG_IF()
Thanks, I agree, this looks much better
> diff --git a/dbug/user.r b/dbug/user.r
> index 8d8a9ce6910..3740a60851c 100644
> --- a/dbug/user.r
> +++ b/dbug/user.r
> @@ -785,18 +785,20 @@ EX:\fC
> +.B 0
> +Like DBUG_EXECUTE_IF this could be used to conditionally execute
> "dangerous" actions.
> .SP 1
> EX:\fC
> .br
> if (prepare_transaction () ||
> .br
> -DBUG_EVALUATE ("crashme", (DBUG_ABORT(), 0), 0) ||
> +(DBUG_IF("crashme") && (DBUG_ABORT(), 0)) ||
oh, a bug in the manual! good catch.
> .br
> commit_transaction () )\fR
> .SP 1
> diff --git a/include/my_dbug.h b/include/my_dbug.h
> index e25bfcf28a7..2afcb010e7c 100644
> --- a/include/my_dbug.h
> +++ b/include/my_dbug.h
> @@ -102,8 +102,7 @@ extern int (*dbug_sanity)(void);
> do {if (_db_keyword_(0, (keyword), 1)) { a1 }} while(0)
> #define DBUG_EVALUATE(keyword,a1,a2) \
> (_db_keyword_(0,(keyword), 0) ? (a1) : (a2))
you've kept DBUG_EVALUATE, although it was only added for consistency
together with DBUG_EVALUATE_IF. And DBUG_EVALUATE is not used anywhere,
I've just grepped. Remove it too?
> -#define DBUG_EVALUATE_IF(keyword,a1,a2) \
> -(_db_keyword_(0,(keyword), 1) ? (a1) : (a2))
> +#define DBUG_IF(keyword) _db_keyword_(0, (keyword), 1)
> #define DBUG_PRINT(keyword,arglist) \
> do if (_db_pargs_(__LINE__,keyword)) _db_doprnt_ arglist; while(0)
>
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] 19fbfab084f: Compilation fix
Hi, Aleksey!
On Sep 01, Aleksey Midenkov wrote:
> revision-id: 19fbfab084f (mariadb-10.6.1-67-g19fbfab084f)
> parent(s): 8009680fbc3
> author: Aleksey Midenkov
> committer: Aleksey Midenkov
> timestamp: 2021-08-30 22:02:40 +0300
> message:
>
> Compilation fix
>
> diff --git a/sql/sql_table.h b/sql/sql_table.h
> index 83e88c82f0e..8cb9ebe0445 100644
> --- a/sql/sql_table.h
> +++ b/sql/sql_table.h
> @@ -20,8 +20,14 @@
> #include // pthread_mutex_t
> #include "m_string.h" // LEX_CUSTRING
>
> +inline bool suicide()
> +{
> + DBUG_SUICIDE();
> + return false;
> +}
> +
> #define ERROR_INJECT_CRASH(code) \
> - (DBUG_IF(code) && (DBUG_SUICIDE(), 0))
> + (DBUG_IF(code) && suicide())
What did that change? A compiler didn't like (void,0) ?
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] 6768e3a2830: MDEV-22166 MIGRATE PARTITION: move out partition into a table
Hi, Aleksey! On Aug 30, Aleksey Midenkov wrote: > Hi Sergei! > > Updated bb-10.7-midenok-MDEV-22166 is d4668e7254c6 I'll look right after replying to this email. > > Okay, indeed, it seems that DBUG_EVALUATE_IF is almost always used with > > one of the arguments being 0 or 1. If you replace all > > DBUG_EVALUATE_IF's, let's just remove it completely. > > Note, that DBUG_TRUE_IF can be defined simply as > > > > #define DBUG_TRUE_IF(keyword) _db_keyword_(0, (keyword), 1) > > > > so may be you'd like to call id DBUG_KEYWORD_IF or something? > > Although DBUG_TRUE_IF is shorter :) > > It turned out DBUG_IF() is even more short! ;) We have a series of IF_xxx(A,B) macros, IF_PARTITIONING, IF_WIN, etc. IF_DBUG(A,B) expands to A if dbug is compiled in, otherwise into B. having IF_DBUG(A,B) and DBUG_IF(keyword) might be confusing. > > > > > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > > > > > index 85d880cbbb4..4172a61812e 100644 > > > > > --- a/sql/sql_partition.cc > > > > > +++ b/sql/sql_partition.cc > > Renamed this error code and ugly ER_KEY_COLUMN_DOES_NOT_EXITS > Changed the message. Unfortunatey ER_xxx constants are generally part of the API, client applications do if (mysql_errno() == ER_KEY_COLUMN_DOES_NOT_EXITS) ... so renaming cannot be done lightly. Only if really unavoidable and then with a compatibility fallback, like we have now in mysql.h: #define ER_WARN_DATA_TRUNCATED WARN_DATA_TRUNCATED #define WARN_PLUGIN_DELETE_BUILTIN ER_PLUGIN_DELETE_BUILTIN #define ER_FK_DUP_NAME ER_DUP_CONSTRAINT_NAME ... > > > > > goto err; > > > > >} 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
