Re: bzr commit into mysql-5.1-bugteam branch (davi:2739) Bug#41348

[Davi Arnaut]

On 1/7/09 7:59 AM, Sergei Golubchik wrote:
> Hi, Davi!
>
> On Jan 06, Davi Arnaut wrote:
>> On 1/6/09 11:35 AM, Sergei Golubchik wrote:
>>> On Dec 17, Davi Arnaut wrote:
>>>> # At a local mysql-5.1-bugteam repository of davi
>>>>
>>>>    2739 Davi Arnaut	2008-12-17
>>>>         Bug#41348: INSERT INTO tbl SELECT * FROM temp_tbl overwrites
>>>>                     locking type of temp table
>>>>
>>>>         The problem is that INSERT INTO .. SELECT FROM .. and CREATE
>>>>         TABLE .. SELECT FROM a temporary table could inadvertently
>>>>         overwrite the locking type of the temporary table. The lock
>>>>         type of temporary tables should be a write lock by default.
>>> why here ?
>>> You've added the code that overwrites
> tables->table->reginfo.lock_type,
>>> perhaps you should fix what you've added ? Something like
>>>
>>> -    if (tables->lock_type != TL_UNLOCK&&   !
> thd->locked_tables)
>>> +    if (tables->lock_type != TL_UNLOCK&&   !
> thd->locked_tables&&
>>> +        tables->table->s->tmp_table == NO_TMP_TABLE)
>>>
>>> The intention of "NO_TMP_TABLE" check was to "skip lock_type update
>>> for temporary tables", but apparently it was placed too late.
>> I initially went with this approach, as can be seen in the first associated
>> to the bug, but it turns out that there a couple of other places that will
>> overwrite the lock_type without checking if its a temporary table.
> ...
>> So, IMHO, this is a dormant problem (present in 5.0) that we need to
>> address in a more general way. Discussing the problem with Konstantin, we
>> seem to agree (kostja: chime in if I misunderstood) that temporary tables
>> shouldn't differ from base one in this regard and that a acceptable generic
>> solution is to reset the lock type of temporary tables when "closing" then.
>> Do you agree? Suggestions?
>
> Yes, I agree, it's much less error-prone approach.
> But it looks inconsistent and confusing that in some places we check for
> NO_TMP_TABLE but still restore the lock type because in other places we
> don't check for it.
>
> If you start restoring the lock type, then I suppose you should remove
> NO_TMP_TABLE protection checks, right ?
>

Yes, but this is a change that I wasn't comfortable in making because I 
haven't looked if it might cause problems. I'll investigate this a bit 
further and commit with your suggestion if all goes well. Thanks.

Regards,

-- Davi Arnaut