From: Date: August 5 2008 2:05pm
Subject: Re: bzr commit into mysql-6.1-fk branch (dlenev:2680) WL#148
List-Archive: http://lists.mysql.com/commits/50920
Message-Id: <20080805120531.GA7458@bodhi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii

Hello,

The new patch looks very nice. I don't have serious comments,
except the one related CREATE TABLE error recovery, which is a subject
of the next milestone anyway.

OK to push.

Minor comments re naming and style below.

* Dmitry Lenev <dlenev@stripped> [08/08/03 23:08]:

> @@ -1,3 +1,93 @@
> +drop tables if exists t1, t2, t3;
> +create table t1 (pk int primary key) engine=myisam;
> +insert into t1 values (1), (2), (3);

Would be nice to have --echo # switching to connection "blocker"
and so on when switching between connections, to improve
legibility of the result files.

> +TABLE_LIST* Foreign_key::get_table_for_frm()
> +{
> +  return ref_table;
> +}

Is it possible to use parent_table here instead? Shall we try to
use parent/child consistently, instead of referenced/referencing?
(the deduction whether ref_table is referenced or referencing is
not simple btw ;)).

> +   @param predicted_length Length of the descriptions which was predicted
>                             by fk_get_frm_section_length() function.

I think this value is more accurate than a prediction, you could
just use "length" or "section_length" or "total_length" here.

FK_FRM_FKS_SECTION_LENGTH_SIZE -- this constant is not for the
total size of the section, but only for the fixed part of it, is
it?

FK_FRM_FIXED_PART_SIZE?
> +bool fk_save_to_frm_section(THD *thd, File file,
> +                            List<Foreign_key> &fkey_list,
> +                            List<Foreign_key_parent> &parent_fkey_list,

If only for the sake of symmetry, I suggest to rename Foreign_key
to Foreign_key_child.

> +List<LEX_STRING>& Foreign_key_child_share::get_table_columns()

Shouldn't it be const and return const &?

> +enum fk_option { FK_OPTION_UNDEF, FK_OPTION_RESTRICT, FK_OPTION_CASCADE,
> +                 FK_OPTION_SET_NULL, FK_OPTION_NO_ACTION, FK_OPTION_DEFAULT};

Enums names should be prefixed with enum_ according to the CS.
enum_fk_option, in other words.

> +/**
> +   Class that represents foreign key in the structures describing its
> +   parent table in CREATE TABLE/ALTER TABLE statements. Contains all
> +   information about foreign key which should be saved in .FRM of its
> +   parent table.
> +*/
>  
> +class Foreign_key_parent: public Sql_alloc {


> +/**
> +   For the object respesenting foreign key in a table share get list
> +   of columns in this table which participate in this foreign key.
> +   (in case of Foreign_key_parent_share it is list of referenced
> +   columns).
> +*/
> +
> +List<LEX_STRING>& Foreign_key_parent_share::get_table_columns()
> +{
> +  return ref_columns;
> +}

parent_columns?


> +
> +/**
> +   Class that represents foreign key in CREATE TABLE/ALTER TABLE
> +   statements. Contains all information about foreign key which
> +   is needed for its creation (and for saving its description
> +   in .FRM file of its child table).
> +*/
> +
> +class Foreign_key: public Foreign_key_parent {

As commented earlier, suggest to rename to Foreign_key_child.
Here you could clarify why this name was chosen -- according to
the SQL standard grammar, when defining a new or altering an
existing foreign key, we define it as part of CREATE/ALTER
specification of the child table, in all cases, hence the name.
> +public:
--