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

[5.5] Improved mysql drop all tables #20536

Merged

Conversation

lkmadushan
Copy link
Contributor

@lkmadushan lkmadushan commented Aug 13, 2017

current implementation runs N queries when database has N number of tables. this PR only run 2 queries to drop all tables.

Previous:

SHOW FULL TABLES WHERE table_type = 'BASE TABLE'
DROP TABLE alpha
DROP TABLE beta
DROP TABLE gamma

Now:

SHOW FULL TABLES WHERE table_type = 'BASE TABLE'
DROP TABLE alpha,beta,gamma

this implementation already exists for other database drivers. but mysql missed that somehow.

@lkmadushan lkmadushan changed the title Improved mysql drop all tables [5.5] Improved mysql drop all tables Aug 13, 2017
@laurencei
Copy link
Contributor

On the PostGres version we escaped the database tables:

 DROP TABLE "alpha","beta","gamma"

We should probably do that here too?

@lkmadushan
Copy link
Contributor Author

@laurencei I tested this and it's working. we can't use double quotes but we can use backtick. Is it necessary?

@koenhoeijmakers
Copy link
Contributor

@lkmadushan it's necessary to use reserved words as identifiers.

@@ -43,12 +43,34 @@ public function getColumnListing($table)
*/
public function dropAllTables()
{
$this->disableForeignKeyConstraints();
$tables = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to go further if $this->getAllTables() is empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Will fix that 👍

@lkmadushan lkmadushan force-pushed the improved-mysql-drop-all-tables branch 2 times, most recently from 55aa8ac to 52060d4 Compare August 13, 2017 12:40
*/
public function compileDropAllTables($tables)
{
return 'drop table `'.implode('`,`', $tables).'`';
Copy link
Member

Choose a reason for hiding this comment

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

Why are you not using our already existing "wrap" functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@lkmadushan lkmadushan force-pushed the improved-mysql-drop-all-tables branch 2 times, most recently from c265a3d to 6261227 Compare August 13, 2017 18:52
@lkmadushan lkmadushan force-pushed the improved-mysql-drop-all-tables branch from 6261227 to ea3b259 Compare August 13, 2017 20:06
@taylorotwell taylorotwell merged commit 608732a into laravel:master Aug 14, 2017
@lkmadushan lkmadushan deleted the improved-mysql-drop-all-tables branch August 15, 2017 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants