* Sergei Golubchik <serg@stripped> [07/07/26 22:21]:
> A couple of questions and style comments below.
> otherwise looks ok.
>
> > This patch introduces no changes in behavior -- the discrepancy in
> > behavior will be fixed when we start calling
> > ::store_lock()/::external_lock() for all tables, regardless whether
> > they are transactional or not, temporary or not.
>
> So, what will happen with this bug when you push ? cannot be closed,
> right ? you didn't fix the sympthoms in the original bugreport.
I guess it'll be downgraded back to where it was.
> > diff -Nrup a/sql/lock.cc b/sql/lock.cc
> > --- a/sql/lock.cc 2007-06-01 12:54:30 +04:00
> > +++ b/sql/lock.cc 2007-07-24 22:52:15 +04:00
> > @@ -151,7 +151,8 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd,
> > }
> >
> > thd->proc_info="System lock";
> > - if (lock_external(thd, tables, count))
> > + if (sql_lock->table_count && lock_external(thd,
> sql_lock->table,
> > + sql_lock->table_count))
>
> Why ? Am I correct that this is just an optimization, not critical for
> correctness of the code ?
No, please see get_lock_data(), for non-transactional temporary
tables we do not call ::store_lock, so should not call
::external_lock().
The old version would go over the list of all tables, including
the non-transactional temporary tables.
The corresponding code in mysql_unlock_tables goes over the
members of sql_lock.
This triggered the assert in handler::~handler() that I added.
> > +/**
> > + Try to find the table in the list of locked tables.
> > + In case of success, unlock the table and remove it from this list.
> > +
> > + @note This function has a legacy side effect: the table is
> > + unlocked even if it is not found in the locked list.
>
> Could that happen ?
Yes, e.g. see mysql_rm_table_part2, where we call mysql_lock_remove.
It would lead to redundant calls to ::store_lock(TL_UNLOCK), but
otherwise no side effects, because, as I write below:
> > + It's not clear if this side effect is intentional or still
> > + desirable. It might lead to unmatched calls to
> > + unlock_external(). Moreover, a discrepancy can be left
> > + unnoticed by the storage engine, because in
> > + unlock_external() we call handler::external_lock(F_UNLCK) only
> > + if table->current_lock is not F_UNLCK.
unlock_external() compensates for extra calls to
::external_lock(F_UNLCK):
if ((*table)->current_lock != F_UNLCK)
{
(*table)->current_lock = F_UNLCK;
if ((error=(*table)->file->external_lock(thd, F_UNLCK)))
> > + mysql_lock_remove(thd, thd->locked_tables, table,
> > + /* Unlock the table even if it is not
> > + found in the list of locked tables. */
> > + TRUE);
>
> If you absolutely must have this last argument documented, instead of
> adding a comment to the every call, give it a better name, for example:
>
> #define UNLOCK_EVEN_IF_NOT_IN_THE_LOCKED_LIST TRUE
No, I don't have to, but I found this style convenient.
Sometimes you can see this style in Postgres:
subplan = grouping_planner(&subroot, 0.0 /* retrieve all tuples */ );
I can easily remove the comment, after all it's only a matter of
one tag jump, but using defines for commenting purposes I find
somewhat artificial.
> > + TABLE *open_tables, *handler_tables, *derived_tables;
> > + /**
> > + List of temporary tables used by this thread. Contains user-level
> > + temporary tables, created with CREATE TEMPORARY TABLE, and
> > + internal temporary tables, created, e.g., to resolve a SELECT,
> > + or for an intermediate table used in ALTER.
> > + XXX Why are internal temporary tables added to this list?
> > + */
> > + TABLE *temporary_tables;
>
> Generally it's best to avoid moving members in the class when adding
> comments. Otherwise constructors that initialize them will start to
> trigger compiler warnings.
OK, I'll fix that.
--
-- Konstantin Osipov Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com The best DATABASE COMPANY in the GALAXY
