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.
> 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.
>
> --- 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...
> + @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.
...
> + 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'?
...
> 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...
>
> @@ -1611,18 +1690,15 @@
Can you please, please, please install diff-p-helper?
>
> /*
> 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."
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.
The indentation of the second condition line is not perfect. It does not
emphasize that the innermost parenthesis belongs into the outermost
negation.
...
> + 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)
> + 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.
...
Regards
Ingo
--
Ingo Strüwing, Senior Software Developer
MySQL GmbH, Radlkoferstr. 2, D-81373 München
Geschäftsführer: Kaj Arnö - HRB München 162140
