RE: bzr commit into mysql-6.0-falcon branch (vvaintroub:2772) Bug#22165

[Vladislav Vaintroub]

Hi Olav,
Thanks for the comments.

Order of updates of triggers is  no problem for MySQL - in MySQL they belong
to server and not to storage engine.
It is  possible I screwed netfrastructure slightly, but I also doubt it -
Table::updateRecords does something completely
unrelated to records itself and seems to be just another kind of trigger.
Basically, all operations are already done to all 
involved records and this code does nothing else just updates the
cardinalities (and in Netfrastructure inserts Images(?) into the
hash table - if you read the  code path from Table::updateRecords() you'll
can be amazed).


>   * Question: You have moved updates of cardinalities outside of the
> region where you had a lock on
> transactionManager->committedTransactions.syncObject. Does this
> increase
> the likelihood that the cardinalities can be updated concurrently by
> multiple threads? (and if yes, does it really matter?)

Possibly it increases the likelihood , but it does not really matter.
Those cardinalities are for optimizer and it is ok if they are off by 1 or 2
(the optimizer really needs only an estimation)


Thanks,
Vlad



> -----Original Message-----
> From: Olav.Sandstaa@stripped [mailto:Olav.Sandstaa@stripped]
> Sent: Wednesday, August 13, 2008 8:17 AM
> To: Vladislav Vaintroub
> Cc: commits@stripped
> Subject: Re: bzr commit into mysql-6.0-falcon branch (vvaintroub:2772)
> Bug#22165
> 
> Hi Vlad,
> 
> Thanks for fixing this high priority bug. The patch looks very good and
> I have only minor comments:
> 
> Table.cpp:
> 
>   * The new method waitForWriteComplete() is declared to not return any
> value but in the implementation you have included a return statement.
> 
> Transaction.cpp:
> 
>   * Question: Before your change to Transaction::commit() all records
> were updated first and then all triggers were fired. After your change
> you do both in a single loop. This is an area where I neither know the
> code or what is correct but I have some vague memories about that an
> AFTER trigger should run "after all operations have been done to the
> involved records". Does your change here change semantic for triggers?
> As I said this is something I do not know very much about so I am just
> asking...
> 
>   * Question: You have moved updates of cardinalities outside of the
> region where you had a lock on
> transactionManager->committedTransactions.syncObject. Does this
> increase
> the likelihood that the cardinalities can be updated concurrently by
> multiple threads? (and if yes, does it really matter?)
> 
>   * Transaction;;hadRecords: the have text passed to the Sync object is
> "Transaction::releaseDependency". Should this be
> "Transaction::hasRecords" instead?
> 
> TransactionManager.cpp:
> 
>   * In the method waitForWriteComplete() I the string added to the Sync
> object should be "TransactionManager::waitForWriteComplete", not just
> "waitForWriteComplete".
> 
> If you think the answer to the question I had about order of updates
> and
> execution of triggers is "no problem" then this patch is "OK to push".
> 
> Regards,
> Olav
> 
> Vladislav Vaintroub wrote:
> > #At file:///C:/bzr/mysql-6.0-falcon-team/
> >
> >  2772 Vladislav Vaintroub	2008-08-11
> >       Bug#22165 - crash while doing ALTER in parallel with another
> transactions
> >       Eliminated races.
> >
> >       Race condition 1.
> >       ALTER/DROP/TRUNCATE have to ensure that table has no committed
> records
> >       that are not yet "write complete". Otherwise crash is possible
> if table is
> >       deleted, but then accessed during Transaction::writeComplete()
> in Record::poke()
> >
> >       It is now solved, Storage::external_lock will wait for all
> transaction on
> >       the table  to become write complete, if there is no uncommited
> records.
> >
> >       Race condition 2.
> >       In Transaction::commit(), Transaction state is set to Committed
> to early
> >       while the transaction is still in the active list. This can
> produce incorrect
> >       results in Transaction::hasUncommitedRecords, DROP is not
> blocked.
> >       When later Transaction::commit() traverses the record list, it
> can crash
> >       accessing freed memory in Table::updateRecord().
> >
> >       This is fixed and transaction state is changed during the
> transition from
> >       active list to committed list.
> > modified:
> >   mysql-test/suite/falcon/t/disabled.def
> >   storage/falcon/Database.cpp
> >   storage/falcon/Database.h
> >   storage/falcon/StorageTable.cpp
> >   storage/falcon/StorageTable.h
> >   storage/falcon/Table.cpp
> >   storage/falcon/Table.h
> >   storage/falcon/Transaction.cpp
> >   storage/falcon/Transaction.h
> >   storage/falcon/TransactionManager.cpp
> >   storage/falcon/TransactionManager.h
> >   storage/falcon/ha_falcon.cpp
> >
> > per-file messages:
> >   storage/falcon/Database.cpp
> >     new function waitForWriteComplete
> >   storage/falcon/Database.h
> >     new function waitForWriteComplete
> >   storage/falcon/StorageTable.cpp
> >     new function waitForWriteComplete
> >   storage/falcon/StorageTable.h
> >     new function waitForWriteComplete
> >   storage/falcon/Table.cpp
> >     new function waitForWriteComplete
> >   storage/falcon/Table.h
> >     new function waitForWriteComplete
> >   storage/falcon/Transaction.cpp
> >     Transaction state was set to Committed to early, allowing for
> DROP to process in paralell
> >     moved state change to later point, when transaction is removed
> from active list and added
> >     to committed list.
> >
> >     Also, removed unnecessary loops : record list is now traversed
> >     only once in Transaction::commit() ,not 2 or 3 times
> >
> >     Additionally, Transaction::hasRecords is now lock-protected to
> >     avoid races with writeComplete() and dropTable()
> >
> >     Renamed hasUncommittedRecords() to hasRecords() - we're sometimes
> interested
> >     in committed no-yet-write-complete transactions
> >   storage/falcon/Transaction.h
> >     hasUncommittedRecords->hasRecords
> >   storage/falcon/TransactionManager.cpp
> >     new function TransactionManager::waitForWriteComplete() to
> >     wait for all transactions referencing records from given
> >     table to become "write complete"
> >   storage/falcon/TransactionManager.h
> >     new function waitForWriteComplete()
> >   storage/falcon/ha_falcon.cpp
> >     in StorageInterface::external_lock, if there is no uncommitted
> records
> >     for the table, also wait while committed records will become
> write
> >     complete. This is done to avoid race with
> Transaction::writeComplete()
> > === modified file 'mysql-test/suite/falcon/t/disabled.def'
> > --- a/mysql-test/suite/falcon/t/disabled.def	2008-08-07 21:11:55
> +0000
> > +++ b/mysql-test/suite/falcon/t/disabled.def	2008-08-11 13:22:53
> +0000
> > @@ -12,4 +12,3 @@
> >
> >  falcon_bug_28095_I  : Bug#xxxxx 2008-04-22 hakank Disabled until
> soft restart is in main tree
> >  falcon_bug_28095_II : Bug#xxxxx 2008-03-22 hakank Disabled until
> soft restart is in main tree
> > -falcon_bug_22165 : Bug#22165 2008-08-07 wlad Disabled temporarily,
> because it crashes. Work in progress
> >
> > === modified file 'storage/falcon/Database.cpp'
> > --- a/storage/falcon/Database.cpp	2008-07-17 13:52:17 +0000
> > +++ b/storage/falcon/Database.cpp	2008-08-11 13:22:53 +0000
> > @@ -2282,6 +2282,11 @@ bool Database::hasUncommittedRecords(Tab
> >  	return transactionManager->hasUncommittedRecords(table,
> transaction);
> >  }
> >
> > +void Database::waitForWriteComplete(Table *table)
> > +{
> > +	transactionManager->waitForWriteComplete(table);
> > +}
> > +
> >  void Database::commitByXid(int xidLength, const UCHAR* xid)
> >  {
> >  	serialLog->commitByXid(xidLength, xid);
> >
> > === modified file 'storage/falcon/Database.h'
> > --- a/storage/falcon/Database.h	2008-05-09 19:58:50 +0000
> > +++ b/storage/falcon/Database.h	2008-08-11 13:22:53 +0000
> > @@ -200,6 +200,7 @@ public:
> >  	virtual void	createDatabase (const char *filename);
> >  	void			renameTable(Table* table, const char*
> newSchema, const char* newName);
> >  	bool			hasUncommittedRecords(Table* table,
Transaction
> *transaction);
> > +	void			waitForWriteComplete(Table *table);
> >  	void			validateCache(void);
> >  	int				recoverGetNextLimbo(int xidSize,
unsigned
> char *xid);
> >  	void			commitByXid(int xidLength, const UCHAR*
xid);
> >
> > === modified file 'storage/falcon/StorageTable.cpp'
> > --- a/storage/falcon/StorageTable.cpp	2008-08-04 15:53:52 +0000
> > +++ b/storage/falcon/StorageTable.cpp	2008-08-11 13:22:53 +0000
> > @@ -565,6 +565,11 @@ int StorageTable::alterCheck(void)
> >  	return 0;
> >  }
> >
> > +void StorageTable::waitForWriteComplete(void)
> > +{
> > +	share->table->waitForWriteComplete();
> > +}
> > +
> >  void StorageTable::unlockRow(void)
> >  {
> >  	if (recordLocked)
> >
> > === modified file 'storage/falcon/StorageTable.h'
> > --- a/storage/falcon/StorageTable.h	2008-08-04 15:53:52 +0000
> > +++ b/storage/falcon/StorageTable.h	2008-08-11 13:22:53 +0000
> > @@ -61,6 +61,7 @@ public:
> >  	void			transactionEnded(void);
> >  	void			setRecord(Record* record, bool locked);
> >  	int				alterCheck(void);
> > +	void			waitForWriteComplete();
> >  	void			clearAlter(void);
> >  	bool			setAlter(void);
> >
> >
> > === modified file 'storage/falcon/Table.cpp'
> > --- a/storage/falcon/Table.cpp	2008-08-07 21:11:55 +0000
> > +++ b/storage/falcon/Table.cpp	2008-08-11 13:22:53 +0000
> > @@ -3331,6 +3331,11 @@ bool Table::hasUncommittedRecords(Transa
> >  	return database->hasUncommittedRecords(this, transaction);
> >  }
> >
> > +void Table::waitForWriteComplete()
> > +{
> > +	return database->waitForWriteComplete(this);
> > +}
> > +
> >  RecordVersion* Table::lockRecord(Record* record, Transaction*
> transaction)
> >  {
> >  	Record *current = fetch(record->recordNumber);
> >
> > === modified file 'storage/falcon/Table.h'
> > --- a/storage/falcon/Table.h	2008-04-22 21:10:23 +0000
> > +++ b/storage/falcon/Table.h	2008-08-11 13:22:53 +0000
> > @@ -185,6 +185,7 @@ public:
> >  	int			getFormatVersion();
> >  	void		validateAndInsert(Transaction *transaction,
> RecordVersion *record);
> >  	bool		hasUncommittedRecords(Transaction* transaction);
> > +	void		waitForWriteComplete();
> >  	void		checkAncestor(Record* current, Record* oldRecord);
> >  	int64		estimateCardinality(void);
> >  	void		optimize(Connection *connection);
> >
> > === modified file 'storage/falcon/Transaction.cpp'
> > --- a/storage/falcon/Transaction.cpp	2008-07-31 10:04:30 +0000
> > +++ b/storage/falcon/Transaction.cpp	2008-08-11 13:22:53 +0000
> > @@ -275,38 +275,43 @@ void Transaction::commit()
> >  		releaseRecordLocks();
> >
> >  	database->serialLog->preCommit(this);
> > -	state = Committed;
> >  	syncActive.unlock();
> >
> > +
> > +
> >  	for (RecordVersion *record = firstRecord; record; record =
> record->nextInTrans)
> > -		if (!record->isSuperceded() && record->state != recLock)
> > -			record->format->table->updateRecord (record);
> > +	{
> > +		Table * table = record->format->table;
> >
> > -	if (commitTriggers)
> > -		for (RecordVersion *record = firstRecord; record; record =
> record->nextInTrans)
> > -			if (!record->isSuperceded() && record->state !=
> recLock)
> > -				record->format->table->postCommit (this,
> record);
> > +		if (!record->isSuperceded() && record->state != recLock)
> > +			{
> > +			table->updateRecord (record);
> > +			if (commitTriggers)
> > +				table->postCommit (this, record);
> > +			}
> >
> > +		if (!record->getPriorVersion())
> > +			++table->cardinality;
> > +		if (record->state == recDeleted && table->cardinality > 0)
> > +			--table->cardinality;
> > +	}
> > +
> >  	releaseDependencies();
> >  	database->flushInversion(this);
> >
> > +	// Transfer transaction from active list to committed list, set
> committed state
> >  	Sync syncCommitted(&transactionManager-
> >committedTransactions.syncObject, "Transaction::commit(2)");
> > -	syncCommitted.lock(Exclusive);
> > -
> >  	Sync syncActiveTransactions(&transactionManager-
> >activeTransactions.syncObject, "Transaction::commit(3)");
> > +	syncCommitted.lock(Exclusive);
> >  	syncActiveTransactions.lock(Exclusive);
> > +
> >  	transactionManager->activeTransactions.remove(this);
> > -	syncActiveTransactions.unlock();
> > -
> > -	for (RecordVersion *record = firstRecord; record; record =
> record->nextInTrans)
> > -		{
> > -		if (!record->getPriorVersion())
> > -			++record->format->table->cardinality;
> > -		if (record->state == recDeleted && record->format->table-
> >cardinality > 0)
> > -			--record->format->table->cardinality;
> > -		}
> >  	transactionManager->committedTransactions.append(this);
> > +	state = Committed;
> > +
> > +	syncActiveTransactions.unlock();
> >  	syncCommitted.unlock();
> > +
> >  	database->commit(this);
> >
> >  	delete [] xid;
> > @@ -925,8 +930,11 @@ void Transaction::truncateTable(Table* t
> >  			ptr = &rec->nextInTrans;
> >  }
> >
> > -bool Transaction::hasUncommittedRecords(Table* table)
> > +bool Transaction::hasRecords(Table* table)
> >  {
> > +	// This lock is to avoid race with writeComplete
> > +	Sync sync(&syncIndexes, "Transaction::releaseDependency");
> > +	sync.lock(Exclusive);
> >  	for (RecordVersion *rec = firstRecord; rec; rec = rec-
> >nextInTrans)
> >  		if (rec->format->table == table)
> >  			return true;
> >
> > === modified file 'storage/falcon/Transaction.h'
> > --- a/storage/falcon/Transaction.h	2008-07-24 08:45:03 +0000
> > +++ b/storage/falcon/Transaction.h	2008-08-11 13:22:53 +0000
> > @@ -105,7 +105,7 @@ public:
> >  	State		waitForTransaction (Transaction *transaction,
TransId
> transId, bool *deadlock);
> >  	void		dropTable(Table* table);
> >  	void		truncateTable(Table* table);
> > -	bool		hasUncommittedRecords(Table* table);
> > +	bool		hasRecords(Table* table);
> >  	void		writeComplete(void);
> >  	void		releaseDependency(void);
> >  	int			createSavepoint();
> >
> > === modified file 'storage/falcon/TransactionManager.cpp'
> > --- a/storage/falcon/TransactionManager.cpp	2008-07-24 08:45:03
> +0000
> > +++ b/storage/falcon/TransactionManager.cpp	2008-08-11 13:22:53
> +0000
> > @@ -27,6 +27,7 @@
> >  #include "Log.h"
> >  #include "LogLock.h"
> >  #include "Synchronize.h"
> > +#include "Thread.h"
> >
> >  static const int EXTRA_TRANSACTIONS = 10;
> >
> > @@ -168,12 +169,40 @@ bool TransactionManager::hasUncommittedR
> >  	syncTrans.lock (Shared);
> >
> >  	for (Transaction *trans = activeTransactions.first; trans; trans
> = trans->next)
> > -		if (trans != transaction && trans->isActive() && trans-
> >hasUncommittedRecords(table))
> > +		if (trans != transaction && trans->isActive() && trans-
> >hasRecords(table))
> >  			return true;
> >
> >  	return false;
> >  }
> >
> > +// Wait until all committed records for a table are purged by
> gophers
> > +// (their transaction become write complete)
> > +void TransactionManager::waitForWriteComplete(Table* table)
> > +{
> > +	for(;;)
> > +		{
> > +		bool again = false;
> > +		Sync committedTrans (&committedTransactions.syncObject,
> > +			"waitForWriteComplete");
> > +		committedTrans.lock (Shared);
> > +
> > +		for (Transaction *trans = committedTransactions.first;
> trans;
> > +			 trans = trans->next)
> > +			{
> > +				if (trans->hasRecords(table)&& trans-
> >writePending)
> > +				{
> > +				again = true;
> > +				break;
> > +				}
> > +			}
> > +
> > +		if(!again)
> > +			return;
> > +
> > +		committedTrans.unlock();
> > +		Thread::getThread("waitForWriteComplete")->sleep(500);
> > +		}
> > +}
> >  void TransactionManager::commitByXid(int xidLength, const UCHAR*
> xid)
> >  {
> >  	Sync sync (&activeTransactions.syncObject,
> "TransactionManager::commitByXid");
> >
> > === modified file 'storage/falcon/TransactionManager.h'
> > --- a/storage/falcon/TransactionManager.h	2008-04-24 17:26:34
> +0000
> > +++ b/storage/falcon/TransactionManager.h	2008-08-11 13:22:53
> +0000
> > @@ -35,6 +35,7 @@ public:
> >  	void			dropTable(Table* table, Transaction*
> transaction);
> >  	void			truncateTable(Table* table, Transaction*
> transaction);
> >  	bool			hasUncommittedRecords(Table* table,
> Transaction* transaction);
> > +	void			waitForWriteComplete(Table *table);
> >  	void			commitByXid(int xidLength, const UCHAR*
xid);
> >  	void			rollbackByXid(int xidLength, const UCHAR*
xid);
> >  	void			print(void);
> >
> > === modified file 'storage/falcon/ha_falcon.cpp'
> > --- a/storage/falcon/ha_falcon.cpp	2008-08-04 15:53:52 +0000
> > +++ b/storage/falcon/ha_falcon.cpp	2008-08-11 13:22:53 +0000
> > @@ -1893,6 +1893,7 @@ int StorageInterface::external_lock(THD
> >  					DBUG_RETURN(error(ret));
> >  					}
> >  				}
> > +				storageTable->waitForWriteComplete();
> >  				break;
> >  			default:
> >  				break;
> >
> >
> >