* Dmitry Lenev <dlenev@stripped> [08/08/19 16:51]:
> #At file:///home/dlenev/src/bzr/mysql-6.1-mil7-ctl/
>
> 2682 Dmitry Lenev 2008-08-19
> Patch for the 7th milestone of WL#148 "Foreign keys"
> ("DDL checks and changes CREATE, CREATE TABLE SELECT,
> CREATE TABLE LIKE") implementing necessary changes in
> CREATE TABLE LIKE (and thus fixing bug #35526 "Foreign
> keys: CREATE TABLE LIKE copies what it shouldn't").
>
Hello Dmitri,
Please find my review for Mil. 7 of WL#148 below. The patch is
approved provided the review comments below are addressed.
Error codes and messages:
-------------------------
- Child column missing - please instead say: No such column <column name> in
child table.
- Parent column missing - please instead say: No such column <column name> in
parent table.
- Parent column duplicated, Child column duplicated:
please instead say:
Child column <column name> is duplicated.
Parent column <column name> in duplicated.
- Parent key has nullable column: Parent key has nullable column <column_name>
- Parent column 'b' type not same as child column 'fk' type
- Type of parent column '%s' is not same as type of child column '%s'
- Parent columns doesn't correspond to PRIMARY KEY or UNIQUE constraint
doesn't -> don't, add an article "a"
- Non-transactional engine and CASCADE/SET NULL/SET DEFAULT
Engine of table %s is non-transactional and CASCADE/SET NULL/SET DEFAULT
is specified.
- NO ACTION is not allowed for a non-transactional engine -- print
the table name in the error message
General
-------
- generally, try not to use auto-generated names in tests.
They are a bit volatile,
still
- in the test for locking, please add --echo # connection <name> when
switching between connections
- suggest to universally use --echo # for comments in tests.
- + switch (sql_type)
+ {
+ /* These date-time types don't have any attributes.*/
-> change to switch (sql_type) {
- get_additional_names_part_length -> rename to
get_unique_constraint_name_length()
- /**
+ Get length of names part of foreign key description in .FRM
+ which is specific for parent table.
+*/
-> /**
Get the length of the unique constraint name, to book for
in the child .frm. We store unique constraint name of the parent
in the child .frm in order to properly reflect it in
I_S.TABLE_CONSTRAINTS.UNIQUE_CONSTRAINT_NAME
*/
- Foreign_key_parent::save_additional_names_part() -> rename to
save_unique_constraint_name(). Rename other methods of the family
in a similar way.
- /**
+ Save names part which is specific for parent table to the
+ description of foreign key in .FRM.
+*/
->
/**
Save the name of the unique constraint of the parent in the child .frm.
Necessary to fill information_schema.
*/
- + names_num= 3 + col_number * 2 + get_additional_names_num() + 1;
-> please add a comment for 3, 2 and 1
something like:
3 /* name of the schema, table, and constraint */
+ col_number * 2 /* names of fk columns in the child and names of fk columns
in the parent */ + ... + 1 /* to store the trailing NULL pointer */.
- +
+void
+Foreign_key_child_share::restore_from_additional_names_part(const char
**names_cols_arr, uint *names_cols_lengths)
- > please re-align
- +void
+Foreign_key_child_share::restore_from_additional_names_part(const char
**names_cols_arr,
+ uint
*names_cols_lengths)
- should accept a pointer, not a pointer to pointer.
- + Populate statement's list of names of foreign keys on which exclusive
+ metadata locks should be obtained from the list of foreign keys.
-> For each foreign key mentioned in CREATE/ALTER table specification, add
an entry to the list of foreign key names to acquire exclusive metadata
locks on.
-
+ Foreign_key_name_entry *fk_name=
+ (Foreign_key_name_entry *) thd->alloc(sizeof(Foreign_key_name_entry));
Check for out-of memory. I.e. don't access it if it's 0, just return.
-
+ Delete .CNS files for the list of constraints in the database and thus
+ mark their names as unoccupied.
+
The comment could say instead:
Delete .CNS files for the given list of constraints.
A .CNS file has the same name as the constraint name, and exists
on the filesystem to indicate that the constaint with such a name
exists. This mechanism is used to ensure unqiueness of constraint
names in a schema.
By deleting the .CNS file we mark the constriant name unused.
It's assumed that all constraints in the list are from the same
database. In fact, this function is only called to delete all
constraints of a signle table.
- the function fk_drop_constraint_names should be resilient to errors:
if an error occurs, it should continue dropping the names.
- fk_check_constraint_added(): please use helper functions for checks that
require a loop. You can have one helper function:
fk_check_columns() and for each sub-loop you could have
fk_check_column_duplicated, fk_check_column_missing(), etc.
- The loop to find a suitable key should be put into a separate function.
It slightly worries me that we not only check for errors there, but also
assign parent_constraint_name in this loop:
+ key_it.rewind();
+ while ((key= key_it++))
Maybe the code that is responsible for finding supporting indexes
in parent and child can be moved to a separate function, so that
fk_check_constraint_added() performs only checks, and modifications
of the tree are performed separately?
- + if ((fkey->update_opt != FK_OPTION_NO_ACTION &&
+ fkey->update_opt != FK_OPTION_RESTRICT) ||
+ (fkey->delete_opt != FK_OPTION_NO_ACTION &&
+ fkey->delete_opt != FK_OPTION_RESTRICT))
This is a good candidate for an inline member function.
- er_fk_child_columns_overlap could be moved to fk_check_child_columns_overlap
- throughout the patch, please don't use abbreviation 'par' for "parent"
in variable names. It's ambiguous, and can expand either to parent,
parallel or partition.
-
+ /*
+ QQ: Should we mark this index auto-generated in order to be
+ able to get rid of it in future ?
+ */
Yes.
- in comments, you often add "Class that describes" at start.
IMHO this doesn't give any information, and is an attempt to
follow more official style, which we don't follow generally in our comments.
- TABLE_LIST::BASE_ONLY:
Please rename table_type to open_type, open_type to open_action.
Please use a named enum_ type for the enum.
- + QQ: How can we ensure that this thing is called for temporary
+ table or when we are under LOCK TABLES?
+ */
When opening tables for LOCK TABLES, we will need to acquire
shared-for-upgrade metadata locks on foreign keys when table data lock type
is TL_WRITE.
As to temporary tables, we agreed to defer the decision till some further
milestone.
-
+/**
+ Class that describes parent table in the description of
+ foreign key.
+*/
->
A parent table referenced by a foreign key. When creating a table
with foreign keys, multiple constraints may refer to the same parent
table. This structure represents such a parent table. A unique list
of such structures is generated during parsing, so that we have
only one instance of create/alter info per parent during execution.
- sql_insert.cc: i'm skipping this file completely, assuming for now
it's just going to be an error if one attemts a CTS with foreign keys.
- + /* Clear the list to make statement re-execution safe. */
+ lex->foreign_keys_to_lock.empty();
Please explain why we need to free and re-add foreign key names on
each execution in a comment.
-
ereate_table_set_open_type_and_adjust_tables()
create_table_set_open_type_and_adjust_tables()
@@ -2182,6 +2182,7 @@ void Query_tables_list::reset_query_tabl
sroutines_list.empty();
sroutines_list_own_last= sroutines_list.next;
sroutines_list_own_elements= 0;
+ foreign_keys_to_lock.empty();
binlog_stmt_flags= 0;
It needs to be explained why we clean up foreign keys to lock.
+ Prepare Parent_info (and HA_CREATE_INFO and Alter_info) objects
+ describing parent tables.
- Please add a comment why we can't just allocate these structures in the
lexer, but need to go over them after open_and_lock_tables.
We agreed to have a pointer to Parent_info in TABLE_LIST.
--
