Skip to content
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

Postfix for #5385 (CORE-5101): Fix slow database restore when Classic server mode is used #7233

Merged

Conversation

ilya071294
Copy link
Contributor

CCH_flush has dbb->dbb_ast_flags & DBB_shutdown_single check to avoid PIO_flush call when a database is restoring. But a restore attachment didn't update dbb->dbb_ast_flags while setting a shutdown mode in a database header. This optimization worked fine for Super server mode because it has Garbage Collector and Cache Writer attachments which read the header and update flags in a shared dbb.

…hen Classic server mode is used

CCH_flush has dbb->dbb_ast_flags & DBB_shutdown_single check to avoid PIO_flush call when a database is restoring. But a restore attachment didn't update dbb->dbb_ast_flags while setting a shutdown mode in a database header. This optimization worked fine for Super server mode because it has Garbage Collector and Cache Writer attachments which read the header and update flags in a shared dbb.
@dyemanov
Copy link
Member

dyemanov commented Oct 11, 2022

This commit surely helps the described situation. I still have some doubts though. DBB_shutdown_* flags are normally set via notify_shutdown() -> SHUT_blocking_ast() -> shutdown() call chain. It works when the database goes online due to delay == -1 (see SHUT_blocking_ast()). It usually works for the database shutdown because of notify_shutdown() being finally (after timeout) called with isc_dpb_shut_force flag. However, if the first call of notify_shutdown() returns true (which means we already have an exclusive access), then DBB_shutdown_* flags are not set properly. This is probably the original problem. So I'd suggest to review (and test) the alternative approach:

	// Database is being shutdown. First notification gives shutdown type and delay in seconds.

	bool exclusive = notify_shutdown(tdbb, flag, delay, guard);
	bool successful = exclusive;

	if (exclusive)
	{
		// Ensure we have the proper DBB_shutdown_* flags in place
		shutdown(tdbb, flag, false);
	}
	else
	{
		// Try to get exclusive database lock periodically up to specified delay. If we
		// haven't gotten it report shutdown error for weaker forms. For forced shutdown
		// keep notifying until successful.

		SSHORT timeout = delay ? delay - 1 : 0;

		do
		{
			if (!(dbb->dbb_ast_flags & (DBB_shut_attach | DBB_shut_tran | DBB_shut_force)))
				break;

			if ((flag & isc_dpb_shut_transaction) && !TRA_active_transactions(tdbb, dbb))
			{
				successful = true;
				break;
			}

			if (timeout && CCH_exclusive(tdbb, LCK_PW, -1, guard))
			{
				exclusive = true;
				break;
			}
		}
		while (timeout--);
	}

@dyemanov
Copy link
Member

A workaround could also be to add isc_dpb_shut_force option to gbak, but I'd rather avoid that.

@ilya071294
Copy link
Contributor Author

However, if the first call of notify_shutdown() returns true (which means we already have an exclusive access), then DBB_shutdown_* flags are not set properly. This is probably the original problem. So I'd suggest to review (and test) the alternative approach

I agree, this approach is better. I've tested it and it works fine.

@ilya071294 ilya071294 requested a review from dyemanov July 13, 2023 11:58
Copy link
Member

@dyemanov dyemanov left a comment

Choose a reason for hiding this comment

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

No objections, you may merge this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants