Re: bk commit into 5.0 tree (antony:1.2493) BUG#25511

On Tue, 2007-06-26 at 11:01 +0200, Ingo Strüwing wrote:
> Hi Antony,
>
> Antony T Curtis wrote:
> > On Mon, 2007-06-25 at 19:09 +0200, Ingo Strüwing wrote:
> ...
> > In case where a duplicate key occurrs, if the key cannot be extracted,
> > then the server will do a PK based update. If the ON UPDATE condition
> > fails on the 2nd try, then it matters not.
>
> If I understand sql_insert.cc:write_record() correctly, a duplicate key
> error on handler::write_row(new_row) is handled by trying to find the
> offended unique index, reading old_row based on the unique key value,
> and then updating with handler::update_row(new_row, old_row). If the
> "key cannot be extracted", then the statement fails.
>
> An offended PK may be the most common problem, but one should easily
> find cases where we still fail to perform the update even when the table
> definitions match perfectly.
>
> ...
> >> A reasonable alternative would be to _always_ return a certain error
> >> message and document this for federated. IMHO this should work if we
> >> return -1 as errkey in ::info(), and translate error == HA_WRITE_SKIP
> >> into the desired message in ha_federated::get_error_message().
> >
> > Then perhaps a new table flag to indicate that ON DUPLICATE KEY UPDATE
> > is not supported? So that other things don't break, we have 3 options.
> >
> > 1. Mark this bug as "Won't fix"
> > 2. New table flag for disabling "ON DUPLICATE KEY UPDATE".
> > 3. This patch.
>
> IMHO, 3. is not an acceptable fix. Unless we put major effort in
> documenting the circumstances in which it works. If I understand your
> patch correctly, you check if the column names of the PK match. IMHO
> this is insufficient for a safe match. And you do not handle other
> unique indexes, which could also be guilty of the duplicate key problems.
>
> Also, I beg you to add my proposal to the list: Always let "ON DUPLICATE
> KEY UPDATE" fail for FEDERATED with an error messages that tells "A
> duplicate key error happened and FEDERATED cannot turn it into an
> update". Of course this does also require some documentation.
>
> With this list please ask the architecture board what to do.
>
> > To cut out parts of the patch (its pretty much minimal) will result in
> > Bug#29019 regression.
>
> Please explain. I can't follow.
>
> > The PK check is a quick test which is not in the
> > critical path when there are no errors and provides a good safety net.
>
> Agree except of the last part "provides a good safety net". IMHO the
> check is too limited to call it a "safety net".

Ok, if we remove the PK check and the new code from ::info(), we effectively have disabled ON DUPLICATE KEY UPDATE for Federated.

Would that be acceptable?

> I still believe that the only way for a real fix goes via a safe
> identification of the offended unique index and a safe match of the
> table definitions.
>
> 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
>

-- 
Antony T Curtis, Senior Software Developer
MySQL Inc, www.mysql.com
SIP: 4468@sip.mysql.com


-- 
MySQL Code Commits Mailing List
For list archives: 
http://lists.mysql.com/commits
To unsubscribe:    
http://lists.mysql.com/commits?unsub=lists@pantek.com