Re: [Maria-developers] MDEV-318 IF (NOT) EXIST clauses for ALTER TABLE
Hi, Holyfoot! On Jul 08, holyfoot@askmonty.org wrote:
At file:///home/hf/wmar/mdev-318/
------------------------------------------------------------ revno: 3552 revision-id: holyfoot@askmonty.org-20120708152354-d7iu7yoqo3cohidz parent: sanja@montyprogram.com-20120626184334-ptpg39ptq3t79dg5 committer: Alexey Botchkov <holyfoot@askmonty.org> branch nick: mdev-318 timestamp: Sun 2012-07-08 20:23:54 +0500 message: MDEV-318 IF (NOT) EXIST clauses for ALTER TABLE (MWL #252). ALTER TABLE syntax modified and related implementations added to allow: ALTER TABLE ADD/DROP COLUMN ALTER TABLE ADD/DROP INDEX ALTER TABLE ADD/DROP FOREIGN KEY ALTER TABLE ADD/DROP PARTITION ALTER TABLE CHANGE COLUMN ALTER TABLE MODIFY COLUMN
Please, find my comments below:
=== modified file 'mysql-test/r/alter_table.result' --- mysql-test/r/alter_table.result 2011-11-21 17:13:14 +0000 +++ mysql-test/r/alter_table.result 2012-07-11 14:24:32 +0000 @@ -1282,3 +1282,30 @@ # Clean-up. drop table t1; End of 5.1 tests +CREATE TABLE t1 ( +id INT(11) NOT NULL, +x_param INT(11) DEFAULT NULL, +PRIMARY KEY (id), +KEY x_param (x_param) +); +ALTER TABLE t1 ADD COLUMN IF NOT EXISTS id INT, +ADD COLUMN IF NOT EXISTS lol INT AFTER id; +Warnings: +Note 1060 Duplicate column name 'id' +ALTER TABLE t1 ADD COLUMN IF NOT EXISTS lol INT AFTER id; +Warnings: +Note 1060 Duplicate column name 'lol' +ALTER TABLE t1 DROP COLUMN IF EXISTS lol; +ALTER TABLE t1 DROP COLUMN IF EXISTS lol; +Warnings: +Note 1091 Can't DROP 'lol'; check that column/key exists +ALTER TABLE t1 ADD KEY IF NOT EXISTS x_param(x_param); +Warnings: +Note 1061 Duplicate key name 'x_param' +ALTER TABLE t1 ADD KEY IF NOT EXISTS x_param(x_param);
typo? you test ADD KEY IF NOT EXISTS twice.
+Warnings: +Note 1061 Duplicate key name 'x_param' +ALTER TABLE t1 MODIFY IF EXISTS lol INT; +Warnings: +Note 1054 Unknown column 'lol' in 't1' +DROP TABLE t1; === modified file 'sql/field.cc' --- sql/field.cc 2012-03-13 14:38:43 +0000 +++ sql/field.cc 2012-07-11 14:28:40 +0000 @@ -9182,6 +9182,7 @@ void Create_field::init_for_tmp_table(en
First: please, see the last section in http://kb.askmonty.org/en/how-to-get-more-out-of-bzr-when-working-on-mariadb... and do as it suggests I had to apply your patch locally and re-run bzr diff to get the function names in the diff :(
(maybe_null ? FIELDFLAG_MAYBE_NULL : 0) | (is_unsigned ? 0 : FIELDFLAG_DECIMAL)); vcol_info= 0; + create_if_not_exists= FALSE; stored_in_db= TRUE; }
=== modified file 'sql/sql_lex.h' --- sql/sql_lex.h 2011-12-11 16:39:33 +0000 +++ sql/sql_lex.h 2012-07-11 14:28:40 +0000 @@ -1816,6 +1816,7 @@ typedef struct st_lex : public Query_tab uint16 create_view_algorithm; uint8 create_view_check; bool drop_if_exists, drop_temporary, local_file, one_shot_set; + bool check_exists; /* Enabled if the IF [NOT] EXISTS specified for ALTER */
I think, you can remove drop_if_exists and use check_exists instead
bool autocommit; bool verbose, no_write_to_binlog;
=== modified file 'sql/sql_yacc.yy' --- sql/sql_yacc.yy 2012-06-10 09:53:06 +0000 +++ sql/sql_yacc.yy 2012-07-11 14:28:40 +0000 @@ -4656,8 +4659,16 @@ table_option: ;
opt_if_not_exists: - /* empty */ { $$= 0; } - | IF not EXISTS { $$=HA_LEX_CREATE_IF_NOT_EXISTS; } + /* empty */ + { + Lex->check_exists= FALSE;
Let's initialize check_exists either before the grammar is parsed (like in the add_create_index_prepare and in the create rule) of in the grammar (here, in the opt_if_not_exists). But don't do it twice.
+ $$= 0; + } + | IF not EXISTS + { + Lex->check_exists= TRUE; + $$=HA_LEX_CREATE_IF_NOT_EXISTS; + } ;
opt_create_table_options: @@ -5879,6 +5891,18 @@ opt_ident: | field_ident { $$=$1.str; } ;
+opt_if_not_exists_ident: + opt_if_not_exists opt_ident + { + LEX *lex= Lex; + if (lex->check_exists && lex->sql_command != SQLCOM_ALTER_TABLE) + { + my_parse_error(ER(ER_SYNTAX_ERROR)); + MYSQL_YYABORT; + } + $$= $2; + }; +
why did you create opt_if_not_exists_ident rule, instead of adding opt_if_not_exists before ident in the alter table grammar?
opt_component: /* empty */ { $$= null_lex_str; } | '.' ident { $$= $2; } @@ -6455,6 +6482,11 @@ opt_column: | COLUMN_SYM {} ;
+opt_column_if_exists: + opt_column if_exists + {} + ; +
same as above - why a special rule?
opt_ignore: /* empty */ { Lex->ignore= 0;} | IGNORE_SYM { Lex->ignore= 1;} @@ -10096,8 +10128,16 @@ table_alias_ref: ;
if_exists:
please, rename this old rule to opt_if_exists
- /* empty */ { $$= 0; } - | IF EXISTS { $$= 1; } + /* empty */ + { + Lex->check_exists= FALSE; + $$= 0; + } + | IF EXISTS + { + Lex->check_exists= TRUE; + $$= 1; + } ;
opt_temporary:
you didn't change CREATE INDEX and DROP INDEX, did you? and ALTER TABLE DROP PRIMARY KEY isn't covered either
=== modified file 'sql/sql_table.cc' --- sql/sql_table.cc 2012-04-05 21:07:18 +0000 +++ sql/sql_table.cc 2012-07-11 14:28:40 +0000 @@ -5788,6 +5788,226 @@ is_index_maintenance_unique (TABLE *tabl
/* + Preparation for table creation + + SYNOPSIS + handle_if_exists_option()
Why did you do that in a separate function? You have basically duplicated all checks. normal alter table code checks for duplicate columns/key names and for non-existant columns/keys for DROP. this code is still in place after your patch. so, basically, you'll check for duplicates and non-existant names twice. what's the point of that? I'd expect you to take check_exists flag into account in the existing checking code and convert errors into warnings as appropriate.
+ thd Thread object. + table The altered table. + alter_info List of columns and indexes to create + + DESCRIPTION + Looks for the IF [NOT] EXISTS options, checks the states and remove items + from the list if existing found. + + RETURN VALUES + NONE +*/ + +static void +handle_if_exists_options(THD *thd, TABLE *table, Alter_info *alter_info) +{ + Field **f_ptr;
Regards, Sergei
11.07.2012 20:14, Sergei Golubchik wrote:
+Note 1061 Duplicate key name 'x_param' +ALTER TABLE t1 ADD KEY IF NOT EXISTS x_param(x_param); typo? you test ADD KEY IF NOT EXISTS twice.
Yes, but not this line. The 'KEY x_param' wasn't supposed to be created with the CREATE TABLE.
uint8 create_view_check; bool drop_if_exists, drop_temporary, local_file, one_shot_set; + bool check_exists; /* Enabled if the IF [NOT] EXISTS specified for ALTER */ I think, you can remove drop_if_exists and use check_exists instead
Seems to work so far...
+opt_if_not_exists_ident: + opt_if_not_exists opt_ident + { + LEX *lex= Lex; + if (lex->check_exists && lex->sql_command != SQLCOM_ALTER_TABLE) + { + my_parse_error(ER(ER_SYNTAX_ERROR)); + MYSQL_YYABORT; + } + $$= $2; + }; + why did you create opt_if_not_exists_ident rule, instead of adding opt_if_not_exists before ident in the alter table grammar?
I'm afraid if i didn't create this rule, i had to add this code to 4 cases of the 'key_def:' And i'd also had to modify more lines, as numbers of parameters after the opt_if_not_exists would increase.
+opt_column_if_exists: + opt_column if_exists + {} + ; + same as above - why a special rule?
Here the sole reason is to avoid modifying the code with the increased parameter's numbers. Ok, i can it's not enough to add a separate rule, going to get rid of this one.
=== modified file 'sql/sql_table.cc' --- sql/sql_table.cc 2012-04-05 21:07:18 +0000 +++ sql/sql_table.cc 2012-07-11 14:28:40 +0000 @@ -5788,6 +5788,226 @@ is_index_maintenance_unique (TABLE *tabl
/* + Preparation for table creation + + SYNOPSIS + handle_if_exists_option() Why did you do that in a separate function? You have basically duplicated all checks. normal alter table code checks for duplicate columns/key names and for non-existant columns/keys for DROP.
this code is still in place after your patch. so, basically, you'll check for duplicates and non-existant names twice. what's the point of that?
I'd expect you to take check_exists flag into account in the existing checking code and convert errors into warnings as appropriate.
Naturally i started doing it that way initially. Then i realized the code looks rather ugly. These checks are scattered around the code and parts of the code with these checks are used not only for ALTER TABLE, but for CREATE and other internal needs. There we need to be sure these check_if_exist flags aren't set. And the loops should be done in a different way in these places so we can remove items from the list in the process. Thus i decided to do that in the separate function to be called from the mysql_alter_table exclusively. In my opinion, the fuction won't affect the performance of the existing functions at all. And it's easy to understand what and why it does, so the code duplication will produce the more error-prone code here. Well, did i convince you the separate function is nicer? Best regards. HF
Hi, Alexey! First: You didn't include all my comments in your reply. Should I assume that all omitted comments you simply agree with and you will change the patch accordingly? On Jul 16, Alexey Botchkov wrote:
11.07.2012 20:14, Sergei Golubchik wrote:
uint8 create_view_check; bool drop_if_exists, drop_temporary, local_file, one_shot_set; + bool check_exists; /* Enabled if the IF [NOT] EXISTS specified for ALTER */ I think, you can remove drop_if_exists and use check_exists instead
Seems to work so far...
Yes. I mean, LEX::drop_if_exists is an old flag that remembers whether IF EXISTS was used in DROP TABLE. While check_exists is a new flag that remembers whether IF EXISTS or IF NOT EXIST was used in ALTER TABLE (and possibly in CREATE INDEX, etc). So, the old flag is only used when dropping a table, and the new one - when dropping or adding a column, partition, or an index. I suggest to remove drop_if_exists flag completely and use check_exists also for DROP TABLE.
+opt_if_not_exists_ident: + opt_if_not_exists opt_ident + { + LEX *lex= Lex; + if (lex->check_exists && lex->sql_command != SQLCOM_ALTER_TABLE) + { + my_parse_error(ER(ER_SYNTAX_ERROR)); + MYSQL_YYABORT; + } + $$= $2; + }; + why did you create opt_if_not_exists_ident rule, instead of adding opt_if_not_exists before ident in the alter table grammar?
I'm afraid if i didn't create this rule, i had to add this code to 4 cases of the 'key_def:'
Ah, indeed. key_def is used both in CREATE and ALTER, and you need your special check to allow the new syntax only in ALTER.
+opt_column_if_exists: + opt_column if_exists + {} + ; + same as above - why a special rule?
Here the sole reason is to avoid modifying the code with the increased parameter's numbers. Ok, i can it's not enough to add a separate rule, going to get rid of this one.
Yes, please.
=== modified file 'sql/sql_table.cc' --- sql/sql_table.cc 2012-04-05 21:07:18 +0000 +++ sql/sql_table.cc 2012-07-11 14:28:40 +0000 @@ -5788,6 +5788,226 @@ is_index_maintenance_unique (TABLE *tabl
/* + Preparation for table creation + + SYNOPSIS + handle_if_exists_option() Why did you do that in a separate function? You have basically duplicated all checks. normal alter table code checks for duplicate
columns/key names and for non-existant columns/keys for DROP.
this code is still in place after your patch. so, basically, you'll check for duplicates and non-existant names twice. what's the point of that?
I'd expect you to take check_exists flag into account in the existing checking code and convert errors into warnings as appropriate.
Naturally i started doing it that way initially. Then i realized the code looks rather ugly. These checks are scattered around the code and parts of the code with these checks are used not only for ALTER TABLE, but for CREATE and other internal needs.
I didn't spend on this much time, but grepping for ER_DUP_FIELDNAME and ER_DUP_KEYNAME (which, I suppose, should show all checks), didn't show all that many places. Only in sql_table.cc, and only in mysql_prepare_create_table().
There we need to be sure these check_if_exist flags aren't set.
Yes, but because it's not set by default, it should be always unset, unless you set it explicitly in sql_yacc.yy, right?
And the loops should be done in a different way in these places so we can remove items from the list in the process.
It is already supported by the current code - both for fields and keys.
Thus i decided to do that in the separate function to be called from the mysql_alter_table exclusively.
In my opinion, the fuction won't affect the performance of the existing functions at all. And it's easy to understand what and why it does, so the code duplication will produce the more error-prone code here.
Well, did i convince you the separate function is nicer?
I'm sorry. Not quite. If the current code is structured badly, it does not mean that we should add a special function with duplicate checks for every new task. If you think the current code is ugly, please, do a little refactoring and move these checks to separate functions, that you can easily extend for your purposes. But, really, I don't see even this being necessary, I think it would be easy to add checks to the existing code as it is. But I may be wrong, of course. Regards, Sergei
30.07.2012 18:27, Sergei Golubchik wrote:
Hi, Alexey!
First: You didn't include all my comments in your reply. Should I assume that all omitted comments you simply agree with and you will change the patch accordingly? That's right.
Why did you do that in a separate function? You have basically duplicated all checks. normal alter table code checks for duplicate columns/key names and for non-existant columns/keys for DROP.
this code is still in place after your patch. so, basically, you'll check for duplicates and non-existant names twice. what's the point of that?
I'd expect you to take check_exists flag into account in the existing checking code and convert errors into warnings as appropriate. Naturally i started doing it that way initially. Then i realized the code looks rather ugly. These checks are scattered around the code and parts of the code with these checks are used not only for ALTER TABLE, but for CREATE and other internal needs. === modified file 'sql/sql_table.cc' --- sql/sql_table.cc 2012-04-05 21:07:18 +0000 +++ sql/sql_table.cc 2012-07-11 14:28:40 +0000 @@ -5788,6 +5788,226 @@ is_index_maintenance_unique (TABLE *tabl
/* + Preparation for table creation + + SYNOPSIS + handle_if_exists_option() I didn't spend on this much time, but grepping for ER_DUP_FIELDNAME and ER_DUP_KEYNAME (which, I suppose, should show all checks), didn't show all that many places. Only in sql_table.cc, and only in mysql_prepare_create_table().
That's not enough. You also should grep for ER_SAME_NAME_PARTITION, ER_DROP_PARTITION_NON_EXISTENT, ER_CANT_DROP_FIELD_OR_KEY, ER_BAD_FIELD_ERROR
There we need to be sure these check_if_exist flags aren't set. Yes, but because it's not set by default, it should be always unset, unless you set it explicitly in sql_yacc.yy, right?
Yes. Still i can imagine one setting that value and we're having mysterious bugs as a result.
And the loops should be done in a different way in these places so we can remove items from the list in the process. It is already supported by the current code - both for fields and keys.
Not really. For fields it works like that: mysql_alter_table() calls mysql_prepare_alter_table() there the new_alter_info is formed from all the old fields that weren't dropped and all the added fields. No duplicated fields checks yet. then it calls compare_tables() which calls mysql_prepare_create_table() which finally discovers the duplicated fields. The code to remove the field with the 'if_exists' mark from the list look ugly there. Which is worse, there it's no use to remove something from the list there at all as it gets only a temporary copy of the Alter_info. Later in the code we call the same mysql_prepare_create_table() with the real data. For partitions code is not nice for removing something from the list at all. And we have to provide the 'check_exists' flag to the remote functions like partition_info::check_partition_info(). And one more thing. It's easy to imagine an user that likes to do the ALTER IF EXISTS queries often. But even if the 'new_field' exists already, ADD FIELD IF EXISTS new_field still does a lot. The list of calls is: mysql_create_table_no_lock() - creates the .frm file with the temporary name mysql_rename_table() mysql_rename_table() once more quick_rm_table() - removes the old .frm query_cache_invalidate() reopen_table() Using the separate function like i did it's easy to notice that the current ALTER is NOOP and return at once.
Thus i decided to do that in the separate function to be called from the mysql_alter_table exclusively.
In my opinion, the fuction won't affect the performance of the existing functions at all. And it's easy to understand what and why it does, so the code duplication will produce the more error-prone code here.
Well, did i convince you the separate function is nicer? I'm sorry. Not quite. If the current code is structured badly, it does not mean that we should add a special function with duplicate checks for every new task.
If you think the current code is ugly, please, do a little refactoring and move these checks to separate functions, that you can easily extend for your purposes.
So i see two options - seriously revise the way mysql_alter_table works or keep the EXISTS handling in the separate function. BTW i don't feel the separate function is that bad. I don't duplicate that much code there besides the loops. But we do such loops in many places anyway. Best regards. HF
Hi, Alexey! On Aug 08, Alexey Botchkov wrote:
=== modified file 'sql/sql_table.cc' --- sql/sql_table.cc 2012-04-05 21:07:18 +0000 +++ sql/sql_table.cc 2012-07-11 14:28:40 +0000 @@ -5788,6 +5788,226 @@ is_index_maintenance_unique (TABLE *tabl
/* + Preparation for table creation + + SYNOPSIS + handle_if_exists_option() I didn't spend on this much time, but grepping for ER_DUP_FIELDNAME and ER_DUP_KEYNAME (which, I suppose, should show all checks), didn't show all that many places. Only in sql_table.cc, and only in mysql_prepare_create_table().
That's not enough. You also should grep for ER_SAME_NAME_PARTITION, ER_DROP_PARTITION_NON_EXISTENT, ER_CANT_DROP_FIELD_OR_KEY, ER_BAD_FIELD_ERROR
Okay. Only one occurence for ER_SAME_NAME_PARTITION and ER_CANT_DROP_FIELD_OR_KEY. Many for ER_BAD_FIELD_ERROR, but only one is relevant to this task. Only ER_DROP_PARTITION_NON_EXISTENT can be issued in more than one place - but partitining code is generally pretty awful, so it's not surprising. But aside of ER_DROP_PARTITION_NON_EXISTENT, every other error happens in only one place. As far as I can see.
There we need to be sure these check_if_exist flags aren't set. Yes, but because it's not set by default, it should be always unset, unless you set it explicitly in sql_yacc.yy, right?
Yes. Still i can imagine one setting that value and we're having mysterious bugs as a result.
because only the parser can set this value - if it's set elsewhere this is a bug on itself. and it'd better be fixed, not masked by ignoring the symptom.
And the loops should be done in a different way in these places so we can remove items from the list in the process. It is already supported by the current code - both for fields and keys.
Not really. For fields it works like that: mysql_alter_table() calls mysql_prepare_alter_table() there the new_alter_info is formed from all the old fields that weren't dropped and all the added fields. No duplicated fields checks yet. then it calls compare_tables() which calls mysql_prepare_create_table() which finally discovers the duplicated fields. The code to remove the field with the 'if_exists' mark from the list look ugly there. Which is worse, there it's no use to remove something from the list there at all as it gets only a temporary copy of the Alter_info. Later in the code we call the same mysql_prepare_create_table() with the real data.
I see. That's right. I've checked the history in bzr to see why compare_tables() works with a temporary copy of the Alter_info. The reason, as far as I can see, was that it shouldn't do permanent changes to the LEX structure, as it breaks stored routines, the statement can no longer be reexecuted correctly. If we'll go with your approach, it must work in this use case. You are changing the original LEX structure, don't you? Could you please extend the test cases to cover that? See the changeset I'm referring to above (compare_tables(), etc) for these use cases.
For partitions code is not nice for removing something from the list at all. And we have to provide the 'check_exists' flag to the remote functions like partition_info::check_partition_info().
yuck. that's bad and buggy piece of code. feel free to remove the check for duplicate names from check_partition_info and move it elsewhere. Test case: CREATE TABLE t1 ( f_int1 INTEGER, f_int2 INTEGER, f_char1 CHAR(20), f_char2 CHAR(20), f_charbig VARCHAR(1000)) PARTITION BY RANGE(f_int1) SUBPARTITION BY KEY(f_int1,f_int1) ( PARTITION part1 VALUES LESS THAN (1000) (SUBPARTITION subpart11, SUBPARTITION subpart1)); it should be an error, because of "KEY(f_int1,f_int1)"
And one more thing. It's easy to imagine a user that likes to do the ALTER IF EXISTS queries often.
But even if the 'new_field' exists already, ADD FIELD IF EXISTS new_field still does a lot. The list of calls is: mysql_create_table_no_lock() - creates the .frm file with the temporary name mysql_rename_table() mysql_rename_table() once more quick_rm_table() - removes the old .frm query_cache_invalidate() reopen_table()
Hmm, I don't see it. If the field exists, mysql_alter_table will notice it in compare_tables() - that is, before mysql_create_table_no_lock().
If you think the current code is ugly, please, do a little refactoring and move these checks to separate functions, that you can easily extend for your purposes.
So i see two options - seriously revise the way mysql_alter_table works or keep the EXISTS handling in the separate function. BTW i don't feel the separate function is that bad. I don't duplicate that much code there besides the loops. But we do such loops in many places anyway.
Loops aren't the problem, the problem is the unmaintainable spaghetti code. We have a lot of it, and we should improve the situation, not make it worse. So, if possible, I'd encourage you to look at how mysql_alter_table checks its input and and improve that. Regards, Sergei
18.12.2012 15:30, Sergei Golubchik wrote:
And one more thing. It's easy to imagine a user that likes to do the ALTER IF EXISTS queries often.
But even if the 'new_field' exists already, ADD FIELD IF EXISTS new_field still does a lot. The list of calls is: mysql_create_table_no_lock() - creates the .frm file with the temporary name mysql_rename_table() mysql_rename_table() once more quick_rm_table() - removes the old .frm query_cache_invalidate() reopen_table() Hmm, I don't see it. If the field exists, mysql_alter_table will notice it in compare_tables() - that is, before mysql_create_table_no_lock().
It's easy to see, just run the ALTER TABLE ADD FIELD IF NOT EXISTS (int existing_field). If the added field already exists, the ALTER TABLE will recreate the table anyway. The can mysql_compare_tables notice it. But in the IF EXISTS case we don't break the command. So that the alter_table process will be continued. We can check in the mysql_compare_tables() if the tables are equal and return the specific code for that. But it's not trivial to implement. It wasn't a problem until this MDEV as the ALTER list could never be empty. But now i think it can be useful to perform the NOOP ALTER possible faster. Otherwise i'm working now to reform the mysql_alter_table() so we can do the IF EXISTS checks in the same places where we return the errors about (not yet) existing objects. It seems working, but the modification is going to be significant. Are we ok with big changes in this part of code? Regards. HF
Hi, Alexey! On Jan 11, Alexey Botchkov wrote:
But now i think it can be useful to perform the NOOP ALTER possible faster.
Agree. But I suppose it's easy. We're trying already to optimize certain cases of ALTER TABLE to avoid copying the data. Like, changing certain table attributes (comments, may be?). Taking into account a completely no-op ALTER fit in well there.
Otherwise i'm working now to reform the mysql_alter_table() so we can do the IF EXISTS checks in the same places where we return the errors about (not yet) existing objects. It seems working, but the modification is going to be significant. Are we ok with big changes in this part of code?
If you say "it seems working", it means you have a (probably incomplete) patch already. Could you send it to me, please, so that I could see how big it is and what it is changing? Regards, Sergei
Hi, Sergei. +++ sql/field.cc 2012-07-11 14:28:40 +0000 @@ -9182,6 +9182,7 @@ void Create_field::init_for_tmp_table(en
First: please, see the last section in http://kb.askmonty.org/en/how-to-get-more-out-of-bzr-when-working-on-mariadb... and do as it suggests
I had to apply your patch locally and re-run bzr diff to get the function names in the diff :(
Wasn't able to start that last section. I created the .bazaar/diff_p/, the __init__.py, and .bazaar/rules files, then get: $ bzr diff >dif2 bzr: ERROR: exceptions.UnboundLocalError: local variable 'diff_file' referenced before assignment Although i used other tips from that page so my diff includes the function's names already. Maybe that's enough. Regards HF
Hi, Alexey! On Jul 16, Alexey Botchkov wrote:
Hi, Sergei.
+++ sql/field.cc 2012-07-11 14:28:40 +0000 @@ -9182,6 +9182,7 @@ void Create_field::init_for_tmp_table(en
First: please, see the last section in http://kb.askmonty.org/en/how-to-get-more-out-of-bzr-when-working-on-mariadb... and do as it suggests
I had to apply your patch locally and re-run bzr diff to get the function names in the diff :(
Wasn't able to start that last section. I created the .bazaar/diff_p/, the __init__.py, and .bazaar/rules files, then get:
it should be .bazaar/plugins/diff_p/
$ bzr diff >dif2 bzr: ERROR: exceptions.UnboundLocalError: local variable 'diff_file' referenced before assignment
Python is sentisive to indentation levels. May be you've messed up the whitespace when copy-pasting from the kb? I'll attach the correct __init__.py to this email.
Although i used other tips from that page so my diff includes the function's names already. Maybe that's enough.
diff_p plugin is better because it adds function names to (almost) *all* diffs. Not only to 'bzr diff', but also to 'bzr commit' generated emails, 'bzr log', etc. Regards, Sergei
$ bzr diff >dif2 bzr: ERROR: exceptions.UnboundLocalError: local variable 'diff_file' referenced before assignment Python is sentisive to indentation levels. May be you've messed up the whitespace when copy-pasting from the kb? I'll attach the correct __init__.py to this email.
Yep, that was the reason. Thanks. Best regards. HF
participants (2)
-
Alexey Botchkov -
Sergei Golubchik