MDEV-37994 Race condition between checkpoint and .ibd file creation

It was possible that a log checkpoint was completed and the server killed
between the time that fil_ibd_create() durably wrote a FILE_CREATE record,
and the initialization of the tablespace. This could lead to a failure
to start up after the server was killed during TRUNCATE TABLE or any
table-rebuilding operation such as OPTIMIZE TABLE.

In the case of TRUNCATE TABLE, an attempt to rename a file #sql-ibNNN.ibd
(the contents of the table before truncation) to tablename.ibd would fail,
because both files existed and the file tablename.ibd would have been
filled with NUL bytes. It was possible to resume from this error by
deleting the file tablename.ibd and restarting the server.

We will prevent this class of errors by ensuring that both the FILE_CREATE
record and the records written by fsp_header_init() will be part of the
same atomic transaction, which must be durably written before any file
is created or allocated.

NOTE: There is another possible crash recovery problem, which we are not
attempting to solve here and which will be covered by the subsequent
change (MDEV-38026). If fil_ibd_create() fails to create the file and
the server were killed, recovery would not even attempt to create the file
at all.

fil_space_t::create(): Remove the DBUG_EXECUTE_IF fault injection that
was the only cause of return nullptr. This allows us to simplify
several callers.

fil_space_t::set_stopped(), fil_space_t::clear_stopped(): Accessor
functions for fil_ibd_create() for preventing any concurrent access
to an incompletely created tablespace.

fil_ibd_create(): In a single atomic mini-transaction, write the
FILE_CREATE record as well as the log for initializing the tablespace.
After durably writing the log, create the file in the file system
Only after the file has been successfully created and allocated,
open the tablespace for business. Finally, release the exclusive page
latches so that the header pages may be written to the file.

fil_ibd_open(): Move some fault injection from fil_space_t::create()
to a higher level, to the place where an existing file is being opened.

Reviewed by: Thirunarayanan Balathandayuthapani
Tested by: Saahil Alam

Author: dr-m

extra/mariabackup/xtrabackup.cc

@@ -5461,16 +5461,12 @@ xb_delta_open_matching_space(
 	ut_ad(fil_space_t::physical_size(flags) == info.page_size);
 
 	mysql_mutex_lock(&fil_system.mutex);
-	fil_space_t* space = fil_space_t::create(uint32_t(info.space_id),
-						 flags, false, 0,
-						 FIL_ENCRYPTION_DEFAULT, true);
+	std::ignore = fil_space_t::create(uint32_t(info.space_id),
+					  flags, false, 0,
+					  FIL_ENCRYPTION_DEFAULT, true);
 	mysql_mutex_unlock(&fil_system.mutex);
-	if (space) {
-		*success = xb_space_create_file(real_name, info.space_id,
-						flags, &file);
-	} else {
-		msg("Can't create tablespace %s\n", dest_space_name);
-	}
+	*success = xb_space_create_file(real_name, info.space_id,
+					flags, &file);
 
 	goto exit;
 }

mysql-test/suite/innodb_fts/r/innodb-fts-ddl.result

@@ -272,6 +272,7 @@ call mtr.add_suppression("InnoDB: Operating system error number .* in a file ope
 call mtr.add_suppression("InnoDB: Error number .* means");
 call mtr.add_suppression("InnoDB: Cannot create file");
 call mtr.add_suppression("InnoDB: Failed to create");
+call mtr.add_suppression("InnoDB: The file '.*test/FTS_[0-9a-f]*_BEING_DELETED\\.ibd' already exists");
 CREATE TABLE t1(a TEXT, FTS_DOC_ID BIGINT UNSIGNED NOT NULL UNIQUE) ENGINE=InnoDB;
 ALTER TABLE t1 ADD FULLTEXT(a), ALGORITHM=INPLACE;
 ERROR HY000: Got error 11 "Resource temporarily unavailable" from storage engine InnoDB

mysql-test/suite/innodb_fts/t/innodb-fts-ddl.test

@@ -365,6 +365,7 @@ call mtr.add_suppression("InnoDB: Operating system error number .* in a file ope
 call mtr.add_suppression("InnoDB: Error number .* means");
 call mtr.add_suppression("InnoDB: Cannot create file");
 call mtr.add_suppression("InnoDB: Failed to create");
+call mtr.add_suppression("InnoDB: The file '.*test/FTS_[0-9a-f]*_BEING_DELETED\\.ibd' already exists");
 
 let MYSQLD_DATADIR=`select @@datadir`;
 CREATE TABLE t1(a TEXT, FTS_DOC_ID BIGINT UNSIGNED NOT NULL UNIQUE) ENGINE=InnoDB;

storage/innobase/fil/fil0fil.cc

@@ -930,8 +930,6 @@ fil_space_t *fil_space_t::create(uint32_t id, ulint flags,
   ut_ad(fil_space_t::is_valid_flags(flags & ~FSP_FLAGS_MEM_MASK, id));
   ut_ad(srv_page_size == UNIV_PAGE_SIZE_ORIG || flags != 0);
 
-  DBUG_EXECUTE_IF("fil_space_create_failure", return nullptr;);
-
   fil_space_t** after= fil_system.spaces.cell_get(id)->search
     (&fil_space_t::hash, [id](const fil_space_t *space)
     { return !space || space->id == id; });
@@ -1840,16 +1838,12 @@ fil_ibd_create(
 	uint32_t	key_id,
 	dberr_t*	err) noexcept
 {
-	pfs_os_file_t	file;
-	bool		success;
-	mtr_t		mtr;
-	bool		has_data_dir = FSP_FLAGS_HAS_DATA_DIR(flags) != 0;
-
 	ut_ad(!is_system_tablespace(space_id));
 	ut_ad(!srv_read_only_mode);
 	ut_a(space_id < SRV_SPACE_ID_UPPER_BOUND);
 	ut_a(size >= FIL_IBD_FILE_INITIAL_SIZE);
-	ut_a(fil_space_t::is_valid_flags(flags & ~FSP_FLAGS_MEM_MASK, space_id));
+	ut_a(fil_space_t::is_valid_flags(flags & ~FSP_FLAGS_MEM_MASK,
+					 space_id));
 
 	/* Create the subdirectories in the path, if they are
 	not there already. */
@@ -1858,11 +1852,6 @@ fil_ibd_create(
 		return NULL;
 	}
 
-	mtr.start();
-	mtr.log_file_op(FILE_CREATE, space_id, path);
-	mtr.commit();
-	log_write_up_to(mtr.commit_lsn(), true);
-
 	static_assert(((UNIV_ZIP_SIZE_MIN >> 1) << 3) == 4096,
 		      "compatibility");
 #if defined _WIN32 || defined HAVE_FCNTL_DIRECT
@@ -1878,35 +1867,95 @@ fil_ibd_create(
 #else
 	constexpr auto type = OS_DATA_FILE;
 #endif
+	const bool is_compressed = fil_space_t::is_compressed(flags);
 
-	file = os_file_create(
+	if (fil_space_t::full_crc32(flags)) {
+		flags |= FSP_FLAGS_FCRC32_PAGE_SSIZE();
+	} else {
+		flags |= FSP_FLAGS_PAGE_SSIZE();
+	}
+
+	/* Create crypt data if the tablespace is either encrypted or user has
+	requested it to remain unencrypted. */
+	fil_space_crypt_t* crypt_data = (mode != FIL_ENCRYPTION_DEFAULT
+					 || srv_encrypt_tables)
+		? fil_space_create_crypt_data(mode, key_id)
+		: nullptr;
+
+	mysql_mutex_lock(&fil_system.mutex);
+	fil_space_t* space = fil_space_t::create(uint32_t(space_id),
+						 flags, false,
+						 crypt_data, mode, true);
+	fil_node_t* node = space->add(path, OS_FILE_CLOSED, size, false, true);
+	space->set_stopped();
+	mysql_mutex_unlock(&fil_system.mutex);
+
+	buf_block_t *header[2];
+	{
+		mtr_t mtr;
+		mtr.start();
+		mtr.log_file_op(FILE_CREATE, space_id, path);
+		mtr.set_named_space(space);
+		log_free_check();
+		ut_a(fsp_header_init(space, size, &mtr) == DB_SUCCESS);
+		ut_ad(mtr.get_savepoint() == 3);
+		header[0] = mtr.at_savepoint(1);
+		header[1] = mtr.at_savepoint(2);
+		ut_ad(header[0]->page.id() == page_id_t(space_id, 0));
+		ut_ad(header[1]->page.id() == page_id_t(space_id, 1));
+		/* Block any page writes before the file has been created. */
+		header[0]->page.lock.x_lock();
+		header[1]->page.lock.x_lock();
+		mtr.commit();
+		/* Durably write the FILE_CREATE record (as well as records for
+		the fsp_header_init() above before creating the file. */
+		log_write_up_to(mtr.commit_lsn(), true);
+	}
+
+	bool success;
+	pfs_os_file_t file = os_file_create(
 		innodb_data_file_key, path,
 		OS_FILE_CREATE | OS_FILE_ON_ERROR_NO_EXIT,
 		type, srv_read_only_mode, &success);
+	ut_ad(!success == (file == OS_FILE_CLOSED));
 
 	if (!success) {
+		*err = DB_ERROR;
 		/* The following call will print an error message */
 		switch (os_file_get_last_error(true)) {
 		case OS_FILE_ALREADY_EXISTS:
-			ib::info() << "The file '" << path << "'"
-				" already exists though the"
-				" corresponding table did not exist"
-				" in the InnoDB data dictionary."
-				" You can resolve the problem by removing"
-				" the file.";
+			sql_print_error("InnoDB: The file '%s'"
+					" already exists though the"
+					" corresponding table did not exist"
+					" in the InnoDB data dictionary."
+					" You can resolve the problem"
+					" by removing"
+					" the file.", path);
 			*err = DB_TABLESPACE_EXISTS;
 			break;
 		case OS_FILE_DISK_FULL:
 			*err = DB_OUT_OF_FILE_SPACE;
-			break;
+			/* fall through */
 		default:
-			*err = DB_ERROR;
+			sql_print_error("InnoDB: Cannot create file '%s'",
+					path);
 		}
-		ib::error() << "Cannot create file '" << path << "'";
-		return NULL;
+		goto err_exit_after_cleanup;
+err_exit:
+		os_file_delete(innodb_data_file_key, path);
+		os_file_close(file);
+err_exit_after_cleanup:
+		space->release();
+		fil_space_free(space_id, false);
+		space = nullptr;
+func_exit:
+		header[0]->page.lock.x_unlock();
+		header[1]->page.lock.x_unlock();
+		DBUG_EXECUTE_IF("checkpoint_after_file_create",
+				log_make_checkpoint(););
+		return space;
 	}
 
-	const bool is_compressed = fil_space_t::is_compressed(flags);
 #ifdef _WIN32
 	const bool is_sparse = is_compressed;
 	if (is_compressed) {
@@ -1917,68 +1966,38 @@ fil_ibd_create(
 		&& DB_SUCCESS == os_file_punch_hole(file, 0, 4096)
 		&& !my_test_if_thinly_provisioned(file);
 #endif
-
-	if (fil_space_t::full_crc32(flags)) {
-		flags |= FSP_FLAGS_FCRC32_PAGE_SSIZE();
-	} else {
-		flags |= FSP_FLAGS_PAGE_SSIZE();
-	}
-
-	/* Create crypt data if the tablespace is either encrypted or user has
-	requested it to remain unencrypted. */
-	fil_space_crypt_t* crypt_data = (mode != FIL_ENCRYPTION_DEFAULT
-					 || srv_encrypt_tables)
-		? fil_space_create_crypt_data(mode, key_id)
-		: nullptr;
-
 	if (!os_file_set_size(path, file,
 			      os_offset_t(size) << srv_page_size_shift,
 			      is_sparse)) {
 		*err = DB_OUT_OF_FILE_SPACE;
-err_exit:
-		os_file_close(file);
-		os_file_delete(innodb_data_file_key, path);
-		free(crypt_data);
-		return nullptr;
+		goto err_exit;
 	}
 
-	fil_space_t::name_type space_name;
+	DBUG_EXECUTE_IF("fil_space_create_failure",
+			*err = DB_OUT_OF_FILE_SPACE; goto err_exit;);
 
-	if (has_data_dir) {
+	if (FSP_FLAGS_HAS_DATA_DIR(flags)) {
 		/* Make the ISL file if the IBD file is not
 		in the default location. */
-		space_name = {name.m_name, strlen(name.m_name)};
+		fil_space_t::name_type space_name{name.m_name,
+						  strlen(name.m_name)};
 		*err = RemoteDatafile::create_link_file(space_name, path);
 		if (*err != DB_SUCCESS) {
 			goto err_exit;
 		}
 	}
 
-	DBUG_EXECUTE_IF("checkpoint_after_file_create",
-			log_make_checkpoint(););
-
 	mysql_mutex_lock(&fil_system.mutex);
-	if (fil_space_t* space = fil_space_t::create(uint32_t(space_id),
-						     flags, false,
-						     crypt_data, mode, true)) {
-		fil_node_t* node = space->add(path, file, size, false, true);
-		node->find_metadata(IF_WIN(,true));
-		mysql_mutex_unlock(&fil_system.mutex);
-		mtr.start();
-		mtr.set_named_space(space);
-		ut_a(fsp_header_init(space, size, &mtr) == DB_SUCCESS);
-		mtr.commit();
-		return space;
-	} else {
-		mysql_mutex_unlock(&fil_system.mutex);
-	}
-
-	if (space_name.data()) {
-		RemoteDatafile::delete_link_file(space_name);
+	space->clear_stopped();
+	node->handle = file;
+	node->find_metadata(IF_WIN(,true));
+	if (++fil_system.n_open >= srv_max_n_open_files) {
+		space->reacquire();
+		fil_space_t::try_to_close(space, true);
+		space->release();
 	}
-
-	*err = DB_ERROR;
-	goto err_exit;
+	mysql_mutex_unlock(&fil_system.mutex);
+	goto func_exit;
 }
 
 fil_space_t *fil_ibd_open(ulint id, ulint flags,
@@ -2162,7 +2181,9 @@ fil_space_t *fil_ibd_open(ulint id, ulint flags,
 			    || df_remote.is_open() != df_remote.is_valid()) {
 				goto corrupted;
 			}
+#ifndef DBUG_OFF
 error:
+#endif
 			local_err = DB_ERROR;
 			goto func_exit;
 		}
@@ -2186,6 +2207,8 @@ fil_space_t *fil_ibd_open(ulint id, ulint flags,
 	ut_a(valid_tablespaces_found == 1);
 
 skip_validate:
+	DBUG_EXECUTE_IF("fil_space_create_failure", goto error;);
+
 	const byte* first_page =
 		df_default.is_open() ? df_default.get_first_page() :
 		df_remote.get_first_page();
@@ -2199,11 +2222,6 @@ fil_space_t *fil_ibd_open(ulint id, ulint flags,
 	space = fil_space_t::create(uint32_t(id), flags,
 				    validate == fil_space_t::VALIDATE_IMPORT,
 				    crypt_data);
-	if (!space) {
-		mysql_mutex_unlock(&fil_system.mutex);
-		goto error;
-	}
-
 	/* We do not measure the size of the file, that is why
 	we pass the 0 below */
 
@@ -2508,11 +2526,6 @@ fil_ibd_load(
 	space = fil_space_t::create(uint32_t(space_id), flags, false,
 				    crypt_data);
 
-	if (space == NULL) {
-		mysql_mutex_unlock(&fil_system.mutex);
-		return(FIL_LOAD_INVALID);
-	}
-
 	ut_ad(space->id == file.space_id());
 	ut_ad(space->id == space_id);
 

storage/innobase/fsp/fsp0space.cc

@@ -112,6 +112,7 @@ Tablespace::open_or_create(bool is_temp)
 		/* We can close the handle now and open the tablespace
 		the proper way. */
 		it->close();
+		mysql_mutex_lock(&fil_system.mutex);
 
 		if (it == begin()) {
 			/* First data file. */
@@ -130,16 +131,9 @@ Tablespace::open_or_create(bool is_temp)
 				fsp_flags = FSP_FLAGS_PAGE_SSIZE();
 			}
 
-			mysql_mutex_lock(&fil_system.mutex);
 			space = fil_space_t::create(
 				uint32_t(m_space_id), fsp_flags,
 				false, nullptr);
-			if (!space) {
-				mysql_mutex_unlock(&fil_system.mutex);
-				return DB_ERROR;
-			}
-		} else {
-			mysql_mutex_lock(&fil_system.mutex);
 		}
 		space->add(it->m_filepath, OS_FILE_CLOSED, it->m_size,
 			   false, true);

storage/innobase/fsp/fsp0sysspace.cc

@@ -938,21 +938,13 @@ SysTablespace::open_or_create(
 			space = fil_space_t::create(
 				SRV_TMP_SPACE_ID, flags(), false, nullptr);
 			ut_ad(space == fil_system.temp_space);
-			if (!space) {
-				err = DB_ERROR;
-				break;
-			}
 			ut_ad(!space->is_compressed());
 			ut_ad(space->full_crc32());
 		} else {
 			ut_ad(space_id() == TRX_SYS_SPACE);
 			space = fil_space_t::create(
 				TRX_SYS_SPACE, it->flags(), false, nullptr);
 			ut_ad(space == fil_system.sys_space);
-			if (!space) {
-				err = DB_ERROR;
-				break;
-			}
 		}
 
 		uint32_t max_size = (++node_counter == m_files.size()

storage/innobase/include/fil0fil.h

@@ -593,6 +593,24 @@ struct fil_space_t final
     n_pending.fetch_and(~NEEDS_FSYNC, std::memory_order_release);
   }
 
+  /** Set the STOPPING flags when creating a file,
+  to block premature fil_crypt_find_space_to_rotate() */
+  inline void set_stopped() noexcept
+  {
+    ut_d(uint32_t n=)
+      n_pending.fetch_add(STOPPING + 1, std::memory_order_relaxed);
+    ut_ad(n == CLOSING);
+  }
+
+  /** Clear the STOPPING flags after creating a file */
+  inline void clear_stopped() noexcept
+  {
+    ut_d(uint32_t n=)
+      n_pending.fetch_sub(STOPPING + CLOSING + 1, std::memory_order_relaxed);
+    ut_ad(n & PENDING);
+    ut_ad((n & ~PENDING) == (STOPPING | CLOSING));
+  }
+
 private:
   /** Clear the CLOSING flag */
   void clear_closing() noexcept
@@ -959,8 +977,7 @@ struct fil_space_t final
   @param crypt_data      encryption information
   @param mode            encryption mode
   @param opened          whether the tablespace files are open
-  @return pointer to created tablespace, to be filled in with add()
-  @retval nullptr on failure (such as when the same tablespace exists) */
+  @return pointer to created tablespace, to be filled in with add() */
   static fil_space_t *create(uint32_t id, ulint flags, bool being_imported,
                              fil_space_crypt_t *crypt_data,
                              fil_encryption_t mode= FIL_ENCRYPTION_DEFAULT,