Skip to content
/ server Public
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MDEV-34703 LOAD DATA INFILE using Innodb bulk load aborts #3751

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

Thirunarayanan
Copy link
Member

  • The Jira issue number for this PR is: MDEV-34703

Description

Problem:

  • During load statement, InnoDB bulk operation relies on temporary directory and it got crash when exhausted.

Solution:

  • For load statement, InnoDB bulk operation does the following
  1. Avoids creation of temporary file for clustered index.
  2. Use normal insert operation for clustered index
  3. Writes the undo log for first insert operation alone

How can this PR be tested?

./mtr innodb.bulk_load

Basing the PR against the correct MariaDB version

  • This is a new feature or a refactoring, and the PR is based against the main branch.
  • This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • x] For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

Sorry, something went wrong.

@Thirunarayanan Thirunarayanan requested a review from dr-m January 13, 2025 05:45
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@vaintroub
Copy link
Member

I gave it a try using the test case from the bug report https://jira.mariadb.org/browse/MDEV-34703. The results are good -
prior to the patch, the bulk optimization was slowing down import by approx 50 percent (i.e on Windows, where it did not crash, and with good IO)

Now the optimization actually works, and cuts load time by factor 2, i.e

No-bulk Bulk- prior to patch Bulk-patched
234.634 373.120 129.285

mysql-test/suite/innodb/t/bulk_load.test Show resolved Hide resolved
storage/innobase/include/row0merge.h Outdated Show resolved Hide resolved
storage/innobase/include/trx0trx.h Outdated Show resolved Hide resolved
Comment on lines 1173 to 1177
/* Avoid using bulk buffer for load statement */
if (index->is_clust() && it->second.load_stmt())
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The load in the name of the member function or data member is not descriptive. It’s really about skipping the merge sort, and possibly also skipping the building of the index one page at a time. The name needs to reflect that.

Based on the changes of row_merge_bulk_t::bulk_insert_buffered() it seems that skip_sort would be an apppropriate name.

Shouldn’t it suffice to test this flag only, and to add ut_ad(index->is_primary())? In that way, we’d execute fewer conditions in a release build.

storage/innobase/row/row0merge.cc Outdated Show resolved Hide resolved
storage/innobase/row/row0merge.cc Outdated Show resolved Hide resolved
storage/innobase/row/row0merge.cc Outdated Show resolved Hide resolved
trx->error_info= index;
else if (ind.is_primary() && index->table->persistent_autoinc)
btr_write_autoinc(index, 1);
err= btr_bulk.finish(err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be avoided if we got err!=DB_SUCCESS from the previous call?

Copy link
Member Author

@Thirunarayanan Thirunarayanan Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BtrBulk::finish() is called even when error exists. This internally calls pageAbort(page_bulk);

mysql-test/suite/innodb/r/bulk_load.result Show resolved Hide resolved
storage/innobase/row/row0merge.cc Outdated Show resolved Hide resolved
storage/innobase/row/row0merge.cc Outdated Show resolved Hide resolved
storage/innobase/row/row0merge.cc Outdated Show resolved Hide resolved
storage/innobase/row/row0merge.cc Outdated Show resolved Hide resolved
storage/innobase/include/trx0trx.h Outdated Show resolved Hide resolved
storage/innobase/include/trx0trx.h Outdated Show resolved Hide resolved
storage/innobase/include/trx0trx.h Outdated Show resolved Hide resolved
storage/innobase/include/row0merge.h Outdated Show resolved Hide resolved
storage/innobase/include/row0merge.h Outdated Show resolved Hide resolved
Copy link
Contributor

@dr-m dr-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. This is rather simple. I would suggest some clarification to the comments.

storage/innobase/include/row0merge.h Outdated Show resolved Hide resolved
storage/innobase/include/trx0trx.h Outdated Show resolved Hide resolved
Comment on lines 5362 to 5367
/** During load bulk, InnoDB does build the clustered index
by one record at a time and doesn't use bulk buffer.
So skip the clustered index while applying the buffered
bulk operation */
if (i)
index= UT_LIST_GET_NEXT(indexes, index);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment should start with /* and not /**. It would be good to mention load_one_row() here, as well as row_ins_index_entry(), which will insert subsequent rows by invoking row_ins_clust_index_entry().

While I was figuring out the logic from the code, I noticed that row_ins_index_entry() is not ideal:

	if (index->is_btree()) {
		if (auto t= trx->check_bulk_buffer(index->table)) {
			/* MDEV-25036 FIXME:
			row_ins_check_foreign_constraint() check
			should be done before buffering the insert
			operation. */
			ut_ad(index->table->skip_alter_undo
			      || !trx->check_foreigns);
			return t->bulk_insert_buffered(*entry, *index, trx);
		}
	}

	if (index->is_primary()) {
		return row_ins_clust_index_entry(index, entry, thr, 0);

is_clust() should be equivalent to is_primary() here, because the code is not being invoked for ibuf.index. Also, is_primary() should imply is_btree(). We seem to be unnecessarily traversing trx->mod_tables for each index. The check had better be done in row_ins(), once for all indexes, and then for the primary clustered index separately. However, refactoring this is outside the scope of this ticket. Can you please file a separate MDEV for this, and only revise the comment here?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
problem:
=======
- During load statement, InnoDB bulk operation relies on temporary
directory and it got crash when tmpdir is exhausted.

Solution:
========
During bulk insert, LOAD statement is building the clustered
index one record at a time instead of page. By doing this,
InnoDB does the following
1) Avoids creation of temporary file for clustered index.
2) Writes the undo log for first insert operation alone
@Thirunarayanan Thirunarayanan merged commit 2d42e9f into 10.11 Jan 16, 2025
13 of 14 checks passed
@Thirunarayanan Thirunarayanan deleted the 10.11-MDEV-34703 branch January 16, 2025 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants