The documentation for both methods is almost the same and states:

// The information_schema table is very slow to query under MySQL 5.0.
// Instead, we try to select from the table and field in question. If it
// fails, the most likely reason is that it does not exist. That is
// dramatically faster than using information_schema.
// @link http://bugs.mysql.com/bug.php?id=19588
// @todo This override should be removed once we require a version of MySQL
// that has that bug fixed.

The MySQL bug has been fixed on 12 Dec 2007 for versions 5.0 and 5.1. The minimum required version that D8 is currently supporting is version 5.5.

Let us do the todo and remove both override methods.

CommentFileSizeAuthor
#13 2797141-13.patch1.82 KBandypost
#2 2797141-1.patch1.74 KBdaffie

Issue fork drupal-2797141

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daffie created an issue. See original summary.

daffie’s picture

daffie CreditAttribution: daffie commented
Issue summary: View changes
FileSize
2797141-1.patch1.74 KB
daffie’s picture

daffie CreditAttribution: daffie commented
Status: Active » Needs review

For the testbot.

daffie’s picture

daffie CreditAttribution: daffie commented

If I compare the testbot runtimes, the ones with the patch are quicker then the ones from automatic testing:
- PHP5.5: 35 min and 37 min;
- PHP5.5: 33 min and 35 min;
- PHP5.5: 18 min and 19 min.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#748340: MySQL-optimized tableExists() implementation

This issue is basically a follow-up to #748340: MySQL-optimized tableExists() implementation, which added the overrides and the comment, so I am adding that as a related issue. I read the discussion there, and the intention is clearly to do what @daffie suggests here.

The one thing I would like to see is a test on a shared host, like Comment 15 on #748340. The comparison in #4 on this issue is encouraging, but the real test comes from a database server "with 2000ish databases". As @crell put it (Comment 41 on #748340), the criterion for removing these methods is "when MySQL stops sucking," and I am not sure this has been met.

To be clear, the bug filed against MySQL has been closed, and I have no evidence that MySQL "still sucks," so I am marking this issue RTBC.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
Title: Remove the methods Drupal\Core\Database\Driver\mysql\Schema::tableExists() and Drupal\Core\Database\Driver\mysql\Schema::fieldExists() » Remove the methods tableExists() and fieldExists() from Drupal\Core\Database\Driver\mysql\Schema

Make the title more DRY. Maybe "the MySQL driver" would be easier to read than the fully qualified class name, but I will leave that as is for now.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Performance

This change looks sensible but reading the last comments on https://bugs.mysql.com/bug.php?id=19588 gives me pause. Some people are reporting it is not truly fixed :(

Can someone do some more research as to whether this is safe to do?

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
2797141-13.patch1.82 KB

reroll to test on current mysql versions in CI

Ghost of Drupal Past’s picture

Ghost of Drupal Past
he/him
CreditAttribution: Ghost of Drupal Past at Smartsheet commented

Check https://stackoverflow.com/a/38778589/308851 for (surprising) benchmarks.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

@chx thanks for link but it using mysql 5.0.77... I used to find fresher ones without success
Exception costs as well so querying information schema looks more predictable

Ghost of Drupal Past’s picture

Ghost of Drupal Past
he/him
CreditAttribution: Ghost of Drupal Past at Smartsheet commented

Yes, it's 5.0.77 but if 5.0.77 was already good with over 11 000 tables then we should be good here.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Driskell’s picture

Driskell CreditAttribution: Driskell as a volunteer and at Other Media commented
Related issues: +#3267207: MySQL schema tableExists treats connection errors incorrectly as meaning table does not exist

I've closed https://www.drupal.org/project/drupal/issues/3267207 as a duplicate of this, and added it as related.

There are further benefits to merging this beyond the performance enhancement as noted in the above. There are cases where tableExists is relied upon to check for database connection failure (see Drupal\Core\Installer\InstallerRedirectTrait::shouldRedirectToInstaller). Because of the exception capture in tableExists here it treats any connection failure as a successful "Table does not exist". This causes a redirect to the installer page if there is any loss of connection within a Drupal page load - which is not ideal - and further compounds with another issue where the redirect can get permanently cached in page_cache....

So yes it would be extremely good, both performance wise, and reliability wise, to proceed with this.

Driskell opened merge request !1913

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Performance

We have the 2 overrides for the MySQL database driver because of a bug in MySQL 5.0.
We have dropped support for that version a very long time ago.
The default has been to use the "information_schema" to test schema data, which the overrides do not do.

Somebody on stackoverflow did in 2016 a performance comparison:

 - MySQL 5.0.77, on a db that has about 11,000 tables.
 - Selecting a non-recently-used table so it's not cached.
 - Averaged over 10 tries each. (Note: done with different tables to avoid caching).

322ms: show tables like 'table201608';

691ms: select 1 from table201608 limit 1;

319ms: SELECT count(*) FROM information_schema.TABLES WHERE (TABLE_SCHEMA = 'mydb') AND (TABLE_NAME = 'table201608');

Using SELECT 1 FROM table is clearly the slowest and that is what the overriddes are using.
See: https://stackoverflow.com/questions/8829102/check-if-mysql-table-exists-...

This issue is "blocking" #3267203: Interruption to the database can cache a redirect to the installer page for all users, #3181013: Faulty permanent config cache has been set to the cache backend on failed sql server connection and #2847855: Incorrect statement class caused by race condition when closing and opening the connection.

The patch was already once RTBC and to me the performance bit has been addressed, therefor changing it back to RTBC.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

I wonder why are we keeping an abstract implementation for ::tableExists() and ::fieldExists().

That seems MySql specific. PgSql and Sqlite have their own. Oracle would have it own. Dunno about MSSql.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 9.4.x-dev » 9.3.x-dev
Category: Task » Bug report
Status: Reviewed & tested by the community » Fixed

@mondrake that's a good point but one that could be addressed in a follow-up in my opinion. It's also a far larger change to make \Drupal\Core\Database\Schema::tableExists() abstract. Let's do this to fix the bugs and consider the API ramifications of #25 - I'd guess \Drupal\Core\Database\Schema::findTables() is another thing where an implementation in the abstract class is driver specific.

Committed and pushed 9e5dd04fac to 10.0.x and 30b2a9daa7 to 9.4.x. Thanks!

As this is helping to fix bugs I re-classified the issue and backported to 9.3.x. Committed 115c991 and pushed to 9.3.x. Thanks!

  • alexpott committed 9e5dd04 on 10.0.x 
    Issue #2797141 by Driskell, daffie, andypost, Charlie ChX Negyesi,...

  • alexpott committed 30b2a9d on 9.4.x 
    Issue #2797141 by Driskell, daffie, andypost, Charlie ChX Negyesi,...

  • alexpott committed 0a4fbd4 on 9.3.x 
    Issue #2797141 by Driskell, daffie, andypost, Charlie ChX Negyesi,...
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

#26 sure. Filed #3267759: Move driver-specific Schema methods implemented in the abstract class to the driver modules.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.