List:Commits« Previous MessageNext Message »
From:Christopher Powers Date:June 24 2009 12:34am
Subject:bzr commit into mysql-6.0-falcon-team branch (christopher.powers:2741)
Bug#37565
View as plain text  
#At file:///home/cpowers/work/dev/dev-04/mysql/

 2741 Christopher Powers	2009-06-23
      Bug #37565 "Crash in Field_blob::pack"
      
      Online ALTER ADD COLUMN creates a new record format that has
      more fields than the existing rows.
      
      Previously, the disparity between the old record formats and
      the newer, larger record format was handled strictly by
      StorageInterface::decodeRecord(), which set the undefined
      fields to NULL prior to returning the decoded record to
      the server. 
      
      This was generally sufficient except when the older records
      were updated. Although Table::update() assigned the new format
      to the record and Record::setEncodedRecord() resized the field
      vector, the new fields in the record were never initialized to NULL.
      
      StorageInterface::encodeRecord() now initializes undefined fields
      to NULL prior to storing the encoded record.
      modified:
        storage/falcon/ha_falcon.cpp
        storage/falcon/ha_falcon.h

per-file messages:
  storage/falcon/ha_falcon.cpp
    StorageInterface::encodeRecord() initializes undefined fields
    to NULL.
    
    Created FieldMap class.
  storage/falcon/ha_falcon.h
    Created FieldMap class to store field mappings
    between the server and Falcon.
=== modified file 'storage/falcon/ha_falcon.cpp'
--- a/storage/falcon/ha_falcon.cpp	2009-06-23 13:20:36 +0000
+++ b/storage/falcon/ha_falcon.cpp	2009-06-24 00:34:01 +0000
@@ -1,4 +1,4 @@
-/* Copyright � 2006-2008 MySQL AB, 2008-2009 Sun Microsystems, Inc.
+/* Copyright � 2006-2008 MySQL AB, 2008-2009 Sun Microsystems, Inc.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -471,7 +471,7 @@ StorageInterface::StorageInterface(handl
 	activeBlobs = NULL;
 	freeBlobs = NULL;
 	errorText = NULL;
-	fieldMap = NULL;
+	fieldMap.storageInterface = this;
 	indexOrder = false;
 	
 	if (table_arg)
@@ -483,7 +483,6 @@ StorageInterface::StorageInterface(handl
 	tableFlags = default_table_flags;
 }
 
-
 StorageInterface::~StorageInterface(void)
 {
 	if (storageTable)
@@ -510,7 +509,7 @@ StorageInterface::~StorageInterface(void
 		storageConnection = NULL;
 		}
 
-	unmapFields();
+	fieldMap.unmapFields();
 }
 
 int StorageInterface::rnd_init(bool scan)
@@ -585,7 +584,7 @@ int StorageInterface::open(const char *n
 
 	// Map fields for Falcon record encoding
 	
-	mapFields(table);
+	fieldMap.mapFields(table);
 	
 	// Map server indexes to Falcon internal indexes
 	
@@ -607,7 +606,7 @@ int StorageInterface::close(void)
 	if (storageTable)
 		storageTable->clearCurrentIndex();
 
-	unmapFields();
+	fieldMap.unmapFields();
 
 	// Temporarily comment out DTrace probes in Falcon, see bug #36403
 	// FALCON_CLOSE();
@@ -925,7 +924,7 @@ int StorageInterface::create(const char 
 
 	// Map fields for Falcon record encoding
 	
-	mapFields(form);
+	fieldMap.mapFields(form);
 	
 	// Map server indexes to Falcon indexes
 	
@@ -2585,6 +2584,8 @@ int StorageInterface::addColumn(THD* thd
 	if ((ret = storageTable->upgrade(gen.getString(), incrementValue)))
 		return (error(ret));
 
+	fieldMap.remapFields(alteredTable);
+	
 	return 0;
 }
 
@@ -2977,15 +2978,27 @@ void StorageInterface::encodeRecord(ucha
 	storageTable->preInsert(&dataStream);
 	my_ptrdiff_t ptrDiff = buf - table->record[0];
 	my_bitmap_map *old_map = dbug_tmp_use_all_columns(table, table->read_set);
-	FieldFormat *fieldFormat = storageShare->format->format;
-	int maxId = storageShare->format->maxId;
+
+	// Default format for the table, possibly newer than the record format
+	
+	FieldFormat *tableFormat = storageShare->format->format;
+
+	// Maximum field definitions for the table
 	
-	for (int n = 0; n < maxId; ++n, ++fieldFormat)
+	int tableMaxId = storageShare->format->maxId;
+	
+	// Maximum field definitions for the current record
+	
+	int recordMaxId = (updateFlag ? storageTable->format->maxId : tableMaxId);
+	
+	for (int id = 0; id < tableMaxId; ++id, ++tableFormat)
 		{
-		if (fieldFormat->fieldId < 0 || fieldFormat->offset == 0)
+		if (tableFormat->fieldId < 0 || tableFormat->offset == 0)
 			continue;
 			
-		Field *field = fieldMap[fieldFormat->fieldId];
+		// Get the field definition
+		
+		Field *field = fieldMap[tableFormat->fieldId];
 		ASSERT(field);
 		
 		if (ptrDiff)
@@ -2993,8 +3006,17 @@ void StorageInterface::encodeRecord(ucha
 
 		if (updateFlag && !bitmap_is_set(table->write_set, field->field_index))
 			{
-			const unsigned char *p = storageTable->getEncoding(n);
-			dataStream.encodeEncoding(p);
+			// Online ALTER ADD COLUMN may have extended the default record format
+			// of the table beyond that of the existing rows. If the current field
+			// is not defined for this record, set the field to NULL.
+			
+			if (id >= recordMaxId)
+				dataStream.encodeNull();
+			else
+				{
+				const unsigned char *p = storageTable->getEncoding(id);
+				dataStream.encodeEncoding(p);
+				}
 			}
 		else if (field->is_null())
 			dataStream.encodeNull();
@@ -3180,40 +3202,33 @@ void StorageInterface::decodeRecord(ucha
 
 	// Format of this record
 
-	FieldFormat *fieldFormat = storageTable->format->format;
+	FieldFormat *recordFormat = storageTable->format->format;
 	int maxId = storageTable->format->maxId;
 	
-	// Current format for the table, possibly newer than the record format
+	// Default format for the table, possibly newer than the record format
 	
 	int tableMaxId = storageTable->share->format->maxId;
-	FieldFormat *tableFieldFormat = storageTable->share->format->format;
+	FieldFormat *tableFormat = storageTable->share->format->format;
 	
-	for (int n = 0; n < tableMaxId; ++n, ++fieldFormat, ++tableFieldFormat)
-		{
-		// Online ALTER ADD COLUMN creates a new record format for the table
-		// that will have more fields than the older formats associated with
-		// existing rows.
-		//
-		// Currently, online ALTER ADD COLUMN only supports nullable columns and
-		// no default value. If the format of this record has fewer fields
-		// than the default format of the table, then there are no fields to
-		// decode beyond maxId, so set them to NULL.
-		
-		if (n >= maxId)
-			{
-			Field *newField = fieldMap[tableFieldFormat->fieldId];
-			newField->set_null();
-			newField->reset();
-			continue;
-			}
+	// Update the field map if it was changed on another thread
+
+	if (fieldMap.maxFields < tableMaxId)
+		fieldMap.remapFields(table);
 	
-		// If the format doesn't have an offset, the field doesn't exist in the record
-		
-		if (fieldFormat->fieldId < 0 || fieldFormat->offset == 0)
-			continue;
-			
+	for (int id = 0; id < tableMaxId; ++id, ++recordFormat, ++tableFormat)
+		{
 		dataStream.decode();
-		Field *field = fieldMap[fieldFormat->fieldId];
+
+		Field *field = NULL;
+		
+		// Get the field definition id from the format associated with the current record.
+		// If the field is undefined for this record, then get the field id from the
+		// default table format and set the field to NULL.
+		
+		if (id < maxId)
+			field = fieldMap[recordFormat->fieldId];
+		else
+			field = fieldMap[tableFormat->fieldId];
 
 		// If we don't have a field for the physical field, just skip over it and don't worry
 		
@@ -3950,17 +3965,41 @@ int StorageInterface::recover (handlerto
 	DBUG_RETURN(count);
 }
 
+FieldMap::FieldMap(StorageInterface *storageInt, TABLE *srvTable)
+					: storageInterface(storageInt)
+{
+	if (!srvTable || !storageInterface)
+		{
+		fields = NULL;
+		maxFields = 0;
+		return;
+		}
+		
+	mapFields(srvTable);
+}
+
+FieldMap::~FieldMap()
+{
+	unmapFields();
+}
+
+Field* FieldMap::operator[](int id)
+{
+	return ((fields && id < maxFields) ? fields[id] : NULL);
+}
+
 // Build a record field map for use by encode/decodeRecord()
 
-void StorageInterface::mapFields(TABLE *srvTable)
+void FieldMap::mapFields(TABLE *srvTable)
 {
-	if (!srvTable)
+	if (!srvTable || !storageInterface)
 		return;
 	
-	maxFields = storageShare->format->maxId;
+	StorageTableShare *storageShare = storageInterface->storageShare;
 	unmapFields();
-	fieldMap = new Field*[maxFields];
-	memset(fieldMap, 0, sizeof(fieldMap[0]) * maxFields);
+	maxFields = storageShare->format->maxId;
+	fields = new Field*[maxFields];
+	memset(fields, 0, sizeof(fields[0]) * maxFields);
 	char nameBuffer[256];
 
 	for (uint n = 0; n < srvTable->s->fields; ++n)
@@ -3970,17 +4009,24 @@ void StorageInterface::mapFields(TABLE *
 		int id = storageShare->getFieldId(nameBuffer);
 		
 		if (id >= 0)
-			fieldMap[id] = field;
+			fields[id] = field;
 		}
 }
 
-void StorageInterface::unmapFields(void)
+void FieldMap::unmapFields(void)
 {
-	if (fieldMap)
+	if (fields)
 		{
-		delete []fieldMap;
-		fieldMap = NULL;
+		delete[] fields;
+		fields = NULL;
 		}
+	maxFields = 0;
+}
+
+void FieldMap::remapFields(TABLE *srvTable)
+{
+	unmapFields();
+	mapFields(srvTable);
 }
 
 static MYSQL_SYSVAR_STR(serial_log_dir, falcon_serial_log_dir,

=== modified file 'storage/falcon/ha_falcon.h'
--- a/storage/falcon/ha_falcon.h	2009-01-15 20:29:54 +0000
+++ b/storage/falcon/ha_falcon.h	2009-06-24 00:34:01 +0000
@@ -13,6 +13,7 @@
    along with this program; if not, write to the Free Software
    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA */
 
+class StorageInterface;
 class StorageConnection;
 class StorageTable;
 class StorageTableShare;
@@ -32,6 +33,22 @@ struct TABLE_SHARE;
 class StorageIndexDesc;
 struct StorageBlob;
 
+class FieldMap
+{
+public:
+	FieldMap(void) : storageInterface(NULL), fields(NULL), maxFields(0) {};
+	FieldMap(StorageInterface *storageInt, TABLE *srvTable = NULL);
+	~FieldMap(void);
+	void mapFields(TABLE *srvTable);
+	void unmapFields(void);
+	void remapFields(TABLE *srvTable);
+	Field* operator[](int id);
+
+	StorageInterface *storageInterface;
+	Field **fields;
+	int maxFields;
+};
+
 class StorageInterface : public handler
 {
 public:
@@ -145,10 +162,10 @@ public:
 	void			decodeRecord(uchar *buf);
 	void			unlockTable(void);
 	void			checkBinLog(void);
-	int			scanRange(const key_range *startKey,
+	int				scanRange(const key_range *startKey,
 					  const key_range *endKey,
 					  bool eqRange);
-	int			fillMrrBitmap();
+	int				fillMrrBitmap();
 	void			mapFields(TABLE *table);
 	void			unmapFields(void);
 
@@ -194,12 +211,12 @@ public:
 	StorageConnection*	storageConnection;
 	StorageTable*		storageTable;
 	StorageTableShare*	storageShare;
-	Field					**fieldMap;
+	FieldMap			fieldMap;
 	const char*			errorText;
 	THR_LOCK_DATA		lockData;			// MySQL lock
 	THD					*mySqlThread;
 	TABLE_SHARE			*share;
-	uint					recordLength;
+	uint				recordLength;
 	int					lastRecord;
 	int					nextRecord;
 	int					indexErrorId;
@@ -207,12 +224,12 @@ public:
 	int					maxFields;
 	StorageBlob			*activeBlobs;
 	StorageBlob			*freeBlobs;
-	bool					haveStartKey;
-	bool					haveEndKey;
-	bool					tableLocked;
-	bool					tempTable;
-	bool					lockForUpdate;
-	bool					indexOrder;
+	bool				haveStartKey;
+	bool				haveEndKey;
+	bool				tableLocked;
+	bool				tempTable;
+	bool				lockForUpdate;
+	bool				indexOrder;
 	key_range			startKey;
 	key_range			endKey;
 	uint64				insertCount;

Thread
bzr commit into mysql-6.0-falcon-team branch (christopher.powers:2741)Bug#37565Christopher Powers24 Jun