On Tue, 2007-06-26 at 17:31 +0200, Ingo Strüwing wrote:
> Hi Antony,
>
> thank you for agreeing with most of my suggestions. Please see below for
> some comments.
>
> antony@stripped wrote:
> ...
> > ChangeSet@stripped, 2007-06-22 15:44:18-07:00, antony@stripped +5 -0
> > Bug#25513
> > "Federared Transactions Failure"
> > Bug occurs when the user performs an operation which inserts more than
> > one row into the federated table and the federated table references a
> > remote table stored within a transactional storage engine. When the
> > insert operation for any one row in the statement fails due to
> > constraint violation, the federated engine is unable to perform
> > statement rollback and so the remote table contains a partial commit.
> > The user would expect a statement to perform the same so a statement
> > rollback is expected.
> > This bug was fixed by implementing bulk-insert handling into the
> > federated storage engine. This will relieve the bug for most common
> > situations by enabling the generation of a multi-row insert into the
> > remote table and thus permitting the remote table to perform
> > statement rollback when neccessary.
> > The multi-row insert is limited to the maximum packet size between
> > servers and should the size overflow, more than one insert statement
> > will be sent and this bug will reappear. Multi-row insert is disabled
> > when an "INSERT...ON DUPLICATE KEY UPDATE" is being performed.
>
> We do not yet have agreement on ON DUPLICATE. This sentence might need a
> change later.
Even if we want DUP_UPDATE mode to fail, this same policy would still be
needed.
>
> > The bulk-insert handling will offer a significant performance boost
> > when inserting a large number of small rows.
> ...
> > --- New file ---
> > +++ mysql-test/t/federated_innodb-slave.opt 07/06/22 15:44:11
> > --innodb
>
> I think this is not necessary because you have "source
> include/have_innodb.inc" in the test.
in my 5.0, it appeared to start the slave with --skip-innodb...
> >
> > --- New file ---
> > +++ mysql-test/t/federated_innodb.test 07/06/22 15:44:11
> > source include/federated.inc;
> > source include/have_innodb.inc;
>
> ...
> > +/**
> > + @brief Construct the INSERT statement.
> > +
> > + @details This method will construct the INSERT statement and appends it to
> > + the supplied query string buffer.
> > +
>
> What editor do you use? I see spaces in the empty lines. These are
> non-issues, but ugly anyway. No need to change, but if you can configure
> your editor for future patches...
vi and nedit
> > + @return
> > + @retval FALSE No error
> > + @retval TRUE Failure
> > +*/
>
> ...
> > + if (replace_duplicates)
> > + insert_string.append(STRING_WITH_LEN("REPLACE INTO "));
> > + else if (ignore_duplicates && !maybe_dup_update)
>
> This "maybe_dup_update" may go away, depending on our agreement about
> Bug#25511.
>
> But. Is it correct at all? Can INSERT IGNORE be combined with ON
> DUPLICATE UPDATE? Won't this be detected in an upper layer already? And
> if not, does the storage engine need to resolve the conflict? As the
> name "maybe_dup_update" correctly says: We do not have a safe way to
> tell if ON DUPLICATE UPDATE is in effect. But if ignore_duplicates is
> set, we can be pretty sure that IGNORE is in effect.
When ON DUPLICATE ..UPDATE is in effect, it can look like ignore is
enabled too as the same ::extra() signal is used.
> ...
> > + insert_string.append(FEDERATED_VALUES);
> > +
> > + DBUG_RETURN(query->append(insert_string));
>
> What is the advantage of building insert_string and then appending it to
> 'query' over building directly in 'query'?
Main reason: I wanted to keep/use existing lines of code so code
geneology is clearly visible. The code was basically copy/pasted with
only minor edits. The objective was to make life easier for reviewers.
> ...
> > values_string.length(0);
> > - insert_string.length(0);
> > insert_field_value_string.length(0);
> > DBUG_ENTER("ha_federated::write_row");
>
> This is a bit too late here. It belongs after the last declaration. Ok,
> it is unchanged from old code...
This is fixed in 5.1 - 5.0 just has it like this.
> >
> > @@ -1611,18 +1690,15 @@
>
> Can you please, please, please install diff-p-helper?
Ok... it was installed but I think I did a bk upgrade which removed it.
> >
> > /*
> > start both our field and field values strings
> > + We must disable multi-row insert for "INSERT...ON DUPLICATE KEY UPDATE"
> > + When replace_duplicates == TRUE, we can safely enable multi-row insert.
> > */
> ...
> > + if (!(use_bulk_insert= bulk_insert.str &&
> > + (!maybe_dup_update || replace_duplicates)))
> > + append_stmt_insert(&values_string);
>
> For a moment I was deeply confused. I forgot what I proposed in a former
> review: Do not create the INSERT statement header for every row of a
> bulk insert. I thought that you would create a statement that begins
> with openparen. :)
>
> To avoid this to happen with others (who did not make that proposal and
> can't remember of it ;-), I solicit you to add a comment like: "In case
> of bulk-insert we do collect the column values for the row only. The
> header of the INSERT statement is created when the first value string is
> copied to the the bulk_insert string."
ok
> My former comment on 'maybe_dup_update' in append_stmt_insert() applies
> here also. At least we should let 'ignore_duplicates' override
> "!maybe_dup_update" in the same way as 'replace_duplicates' does.
by not allowing dup_update to participate in multi-row insert, we allow
it to fail sanely at the first error.
> The indentation of the second condition line is not perfect. It does not
> emphasize that the innermost parenthesis belongs into the outermost
> negation.
oops.
> ...
> > + if (bulk_insert.length == 0)
> > + {
> > + char insert_buffer[FEDERATED_QUERY_BUFFER_SIZE];
> > + String insert_string(insert_buffer, sizeof(insert_buffer),
> &my_charset_bin);
>
> Please split lines > 80 characters. (this one and 2 lines below)
ok
> > + insert_string.length(0);
> > + append_stmt_insert(&insert_string);
> > + dynstr_append_mem(&bulk_insert, insert_string.ptr(),
> insert_string.length());
>
> Just for the records: What a pity that we have different string
> implementations. I agree that we need to potter with two strings here.
Yes... also the dynstr implementation doesn't have as rich functions as
String, nor does it understand character sets. It is more of a dynamic
char array than a dynamic string.
Perhaps for future cleanup, it would be preferable for String class use
be removed to remove a dependency on sql/*, or the other option is that
String class should come out and be a part of libmysys or libstrings.
But that is a topic for another time.
Regards,
Antony.
--
Antony T Curtis, Senior Software Developer
MySQL Inc, www.mysql.com
SIP: 4468@stripped
