-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fix create table if not exists when indexes already exist #6249
Fix create table if not exists when indexes already exist #6249
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A rewording suggestion. Please also add a note to the upgrade guide. That could wait until we decide on the parameter removal.
@sclubricants Thanks for splitting this out! Your issue and this very specific PR makes it much easier to understand what is happening. I like this approach, I think the only drawback is a possible race condition but since forge operations should not ever be under load in production I think the benefits outweigh that slim possibility. |
I don't agree that changing any existing method signature in I don't see anything particularly wrong with this PR, but it changes the behavior (SQL statements will change). |
@michalsn any chance you have time for a review? |
2479d07
to
0aa6530
Compare
0aa6530
to
3bddc85
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed changes are reasonable and will solve the initial problem.
I can't see any real downsides maybe except for some performance decrease. But since the Forge
class is mainly used during testing, we should be fine.
We just need some small tweaks.
Co-authored-by: kenjis <kenji.uui@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
LGTM!
@kenjis I just realized a couple more changes. _createTable() no longer will return a bool. /**
* @return string
*
* @deprecated $ifNotExists is no longer used, and will be removed.
*/
protected function _createTable(string $table, bool $ifNotExists, array $attributes)
{ And remove from createTable() logic to handle bool from _createTable(). if (is_bool($sql)) {
$this->reset();
if ($sql === false) {
if ($this->db->DBDebug) {
throw new DatabaseException('This feature is not available for the database you are using.');
}
return false;
}
return true;
} |
@sclubricants You are correct. Good catch! |
This fixes #6248
This removes all code for CREATE TABLE table
IF NOT EXISTS
. Because all platforms do not support this statement the current implementation queries the database first to determine if the table exists. If it does then it doesn't execute the create table statement. The problem occurs when the create indexes statements are executed after the create table statement and the indexes already exists. Rather then try and apply CREATE INDEX index IF NOT EXISTS and carry through the logic, this PR removes completely the IF NOT EXISTS regime and instead relies on the query table lookup. If the table exists then there is need to execute anything. This way the code is simpler and all the problems of index creation is averted.All the magic happens in (base) Forge here:
Checklist: