On Wed, 17 Jun 2009 21:58:21 +0200, Konstantin Osipov <kostja@stripped>
wrote:
> * Tor Didriksen <Tor.Didriksen@stripped> [09/06/17 12:19]:
>> 3357 Tor Didriksen 2009-06-16
>> http://bugs.mysql.com/bug.php?id=45523
>
> Please add a bug title and describe the idea of the fix.
>
> E.g. :
>
> Bug#45523 "Objects of class base_ilist should not be copyable".
done
>
> Suppress the compiler-generated public copy constructor
> and assignment operator of class base_ilist; instead, implement
> move_elements() method that would transfer ownership of elements
> from one list to another.
>
Thanks, I was deliberately brief in my commenting :-)
This is exactly the kind of comment I wanted:
what are the MySQL conventions for commenting on changesets.
>
> Notice the style in which I wrote bug id above: Bug#XXX triggers
> various automatic actions upon commit, so it's better to stick to
> it.
Thanks.
This should have been mentioned in https://inside.mysql.com/wiki/Bazaar
which talks about setting up commit mails, but not how to reference
worklog items or bug reports.
>
>
>>
>> modified:
>> sql/sp_head.cc
>> sql/sql_cursor.cc
>> sql/sql_list.h
>> sql/sql_prepare.cc
>
> Normally we write per-file comments as well. Your change is small,
> so perhaps they are not necessary, but usually we're pretty
> verbose.
done
>
>
>> === modified file 'sql/sp_head.cc'
>> --- a/sql/sp_head.cc 2009-06-12 02:01:08 +0000
>> +++ b/sql/sp_head.cc 2009-06-16 13:17:15 +0000
>> @@ -1174,8 +1174,7 @@ sp_head::execute(THD *thd)
>> We should also save Item tree change list to avoid rollback
>> something
>> too early in the calling query.
>> */
>> - old_change_list= thd->change_list;
>> - thd->change_list.empty();
>> + thd->change_list.move_elements(&old_change_list);
>
> OK.
>
>> /*
>> Cursors will use thd->packet, so they may corrupt data which was
>> prepared
>> for sending by upper level. OTOH cursors in the same routine can
>> share this
>> @@ -1321,9 +1320,7 @@ sp_head::execute(THD *thd)
>> /* Restore all saved */
>> old_packet.swap(thd->packet);
>> DBUG_ASSERT(thd->change_list.is_empty());
>> - thd->change_list= old_change_list;
>> - /* To avoid wiping out thd->change_list on old_change_list
>> destruction */
>> - old_change_list.empty();
>> + old_change_list.move_elements(&thd->change_list);
>
> OK.
>
>> thd->lex= old_lex;
>> thd->query_id= old_query_id;
>> DBUG_ASSERT(!thd->derived_tables);
>>
>> === modified file 'sql/sql_cursor.cc'
>> --- a/sql/sql_cursor.cc 2009-01-26 16:03:39 +0000
>> +++ b/sql/sql_cursor.cc 2009-06-16 13:17:15 +0000
>> @@ -320,7 +320,7 @@ Sensitive_cursor::post_open(THD *thd)
>> lock= thd->lock;
>> query_id= thd->query_id;
>> free_list= thd->free_list;
>> - change_list= thd->change_list;
>> + thd->change_list.move_elements(&change_list);
>> reset_thd(thd);
>> /* Now we have an active cursor and can cause a deadlock */
>> thd->lock_info.n_cursors++;
>> @@ -436,7 +436,7 @@ Sensitive_cursor::fetch(ulong num_rows)
>> thd->open_tables= open_tables;
>> thd->lock= lock;
>> thd->query_id= query_id;
>> - thd->change_list= change_list;
>> + change_list.move_elements(&thd->change_list);
>> /* save references to memory allocated during fetch */
>> thd->set_n_backup_active_arena(this, &backup_arena);
>>
>> @@ -458,7 +458,7 @@ Sensitive_cursor::fetch(ulong num_rows)
>> /* Grab free_list here to correctly free it in close */
>> thd->restore_active_arena(this, &backup_arena);
>>
>> - change_list= thd->change_list;
>> + thd->change_list.move_elements(&change_list);
>> reset_thd(thd);
>>
>> for (info= ht_info; info->read_view; info++)
>> @@ -505,7 +505,7 @@ Sensitive_cursor::close()
>> info->ht= 0;
>> }
>>
>> - thd->change_list= change_list;
>> + change_list.move_elements(&thd->change_list);
>> {
>> /*
>> XXX: Another hack: we need to set THD state as if in a fetch to
>> be
>> @@ -531,7 +531,6 @@ Sensitive_cursor::close()
>> join= 0;
>> stmt_arena= 0;
>> free_items();
>> - change_list.empty();
>> DBUG_VOID_RETURN;
>> }
>
> OK.
>
>> === modified file 'sql/sql_list.h'
>> --- a/sql/sql_list.h 2009-03-28 00:27:25 +0000
>> +++ b/sql/sql_list.h 2009-06-16 13:17:15 +0000
>> @@ -511,8 +511,9 @@ template <class T> class I_List_iterator
>>
>> class base_ilist
>> {
>> + struct ilink *first;
>> + struct ilink last;
>> public:
>> - struct ilink *first,last;
>> inline void empty() { first= &last; last.prev= &first; }
>> base_ilist() { empty(); }
>> inline bool is_empty() { return first == &last; }
>> @@ -540,7 +541,20 @@ public:
>> {
>> return (first != &last) ? first : 0;
>> }
>> - friend class base_list_iterator;
>> +
>> + /* Moves list elements to new owner. */
>
> Please use doxygen changeset comments.
>
> Why do you assert that new_owner is empty? It should be documented
> in the method comment.
>
> If your use cases are always such that new_owner is empty, perhaps
> you should implement ::swap() instead, that would exchange two
> lists? The advantage of swap(), in my view, is that it's a more
> commonly used pattern. swap() can be used in all your use cases,
> plus in a variety of others.
>
The only use cases in the existing code was to temporarily grab
a list and take ownersip of it, and empty() the original owner.
swap() is a well known idiom of course, but that functionality
is not needed in the current codebase, so I implemented 'ownership change'
instead.
>> + void move_elements(base_ilist* new_owner) {
>
> Opening bracket should belong to an own line.
oops, sorry, old habits.
>
>> + DBUG_ASSERT(new_owner->is_empty());
>> + new_owner->first= first;
>> + new_owner->last= last;
>> + empty();
>> + }
>> +
>> + friend class base_ilist_iterator;
>> + private:
>> + /* Declare, but don't define copy CTOR and assignment operator. */
>> + base_ilist(const base_ilist&);
>> + void operator=(const base_ilist& rhs);
>
> & goes with rhs: const base_ilist &rhs, or just drop the parameter
> name altogether.
That's another item for the style guide I think.
btw, in google we used a standardized macros for this:
// A macro to disallow the copy constructor and operator= functions
// This should be used in the private: declarations for a class
#define DISALLOW_COPY_AND_ASSIGN(TypeName) \
TypeName(const TypeName&); \
void operator=(const TypeName&)
declared in a common header file.
Do we have a natural place for thinks like this in MySQL?
>
>> };
>>
>>
>> @@ -573,6 +587,9 @@ public:
>> inline void push_back(T* a) { base_ilist::push_back(a); }
>> inline T* get() { return (T*) base_ilist::get(); }
>> inline T* head() { return (T*) base_ilist::head(); }
>> + inline void move_elements(I_List<T>* new_owner) {
>
> * goes with new_owner, I_List<T> *new_owner
>
>> + base_ilist::move_elements(new_owner);
>> + }
>> #ifndef _lint
>> friend class I_List_iterator<T>;
>> #endif
>>
>> === modified file 'sql/sql_prepare.cc'
>> --- a/sql/sql_prepare.cc 2009-06-06 00:02:04 +0000
>> +++ b/sql/sql_prepare.cc 2009-06-16 13:17:15 +0000
>> @@ -3429,7 +3429,8 @@ Prepared_statement::execute_server_runna
>> Statement stmt_backup;
>> bool error;
>> Query_arena *save_stmt_arena= thd->stmt_arena;
>> - Item_change_list save_change_list= thd->change_list;
>> + Item_change_list save_change_list;
>> + thd->change_list.move_elements(&save_change_list);
>>
>> state= CONVENTIONAL_EXECUTION;
>>
>> @@ -3439,7 +3440,6 @@ Prepared_statement::execute_server_runna
>> thd->set_n_backup_statement(this, &stmt_backup);
>> thd->set_n_backup_active_arena(this, &stmt_backup);
>> thd->stmt_arena= this;
>> - thd->change_list.empty();
>>
>> error= server_runnable->execute_server_code(thd);
>>
>> @@ -3454,9 +3454,7 @@ Prepared_statement::execute_server_runna
>> thd->restore_backup_statement(this, &stmt_backup);
>> thd->stmt_arena= save_stmt_arena;
>>
>> - DBUG_ASSERT(thd->change_list.is_empty());
>> - thd->change_list= save_change_list;
>> - save_change_list.empty();
>> + save_change_list.move_elements(&thd->change_list);
>
> OK. If you use swap(), please restore that assert back.
>
>>
>> /* Items and memory will freed in destructor */
>
> Have you run the regression tests?
>
I'm pretty fanatic about testing :-)
I always run submit tests,
both before sending things off for review, and then again before
submitting.
This is what I've done:
BUILD/compile-pentium-debug-max
time ./mtr --mem --force
....
The servers were restarted 723 times
Spent 3647.453 of 5034 seconds executing testcases
All 1242 tests were successful.
real 83m54.320s
user 27m32.900s
sys 5m38.904s
Without --mem it takes about five hours...
I would normally run in both dbg and opt, but the current build system
doesn't support that very well.
-- didrik
BTW: my bzr repository got corrupted, so I had to start all over
(fortunately I had a diff/patch lying around).
I'll commit this once more from a different branch, adjusted for your
comments of course.
