backport showTableStatus from Magento2

[jonashrem]

Description (*)

At the moment, Magento2 has a special code path inside showTableStatus() when MySQL8 is used.

This is needed, to flush information_schema_stats Cache introuded in MySQL 8.

As some modules (for example https://github.com/mash2/cobby-magento/blob/master/src/app/code/community/Mash2/Cobby/Helper/Resource.php ) , are using showTableStatus to recieve autoinvrement ID, this ones will break with MySQL 8 when information_schema_stats_expiry is > 0.

This PR will allow the use of showTableStatus without the need to disable status information_schema_stats Cache.

Related Pull Requests

/

Fixed Issues (if relevant)

/

Manual testing scenarios (*)

/

Questions or comments

/

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)

[colinmollenhour]

I'm pretty strongly opposed to this patch.

Reading the table status to get the next autoincrement value within a transactional environment is an anti-pattern, at least in MySQL world as evidenced by the fact that you can't simply query "SHOW next auto_increment_value FROM table1". Engines use all sorts of special techniques to avoid locks on insert gap so as far as I know, no DBMS should ever be written to "predict" the next auto-increment value, that's completely counter to the "automatic" part of it! Also, running ANALYZE TABLE every time you get table stats seems like it could have a major performance impact. This method takes a read lock on the full table, clears table statistics in information schema and even writes to the binlog.

Lastly, the same could easily be accomplished using another method like "SELECT MAX(id) FROM table" or using a second ID column that isn't auto-incrementing.

[jonashrem]

I'm pretty strongly opposed to this patch.

Reading the table status to get the next autoincrement value within a transactional environment is an anti-pattern, at least in MySQL world as evidenced by the fact that you can't simply query "SHOW next auto_increment_value FROM table1". Engines use all sorts of special techniques to avoid locks on insert gap so as far as I know, no DBMS should ever be written to "predict" the next auto-increment value, that's completely counter to the "automatic" part of it! Also, running ANALYZE TABLE every time you get table stats seems like it could have a major performance impact. This method takes a read lock on the full table, clears table statistics in information schema and even writes to the binlog.

Lastly, the same could easily be accomplished using another method like "SELECT MAX(id) FROM table" or using a second ID column that isn't auto-incrementing.

well, yes I agree that this is not a good way to do this, but apart from plugins like cobby, magento itself is using this method.

See https://github.com/OpenMage/magento-lts/blob/1.9.4.x/app/code/core/Mage/ImportExport/Model/Resource/Helper/Mysql4.php#L62

This also is the case with Magento2 (see https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/ImportExport/Model/ResourceModel/Helper.php#L53 )

So the alternative to the patch would be, to change the getNextAutoincrement method. This would still break plugins bringing their own like cobby. Writing a better getNextAutoincrement function can of course still done independing.

The other alternative would be setting information_schema_stats_expiry to 0 but this can have even more impacts of performance.

[jonashrem]

To be sure, this is clear. The patch is a 1 to 1 port from Magento2. See https://github.com/magento/magento2/blob/2.4-develop/lib/internal/Magento/Framework/DB/Adapter/Pdo/Mysql.php#L1191

[colinmollenhour]

To be sure, this is clear. The patch is a 1 to 1 port from Magento2.

I fully realize that but IMHO Magento 2 devs just made a bad decision and I don't think that is a good reason on it's own for OM to do the same.

So my advice is to report this as a bug for Cobby when using MySQL 8 and propose that they should change their code to not depend on DDL statements for transactional purposes. The easiest and most backwards compatible way would probably be to change getNextAutoincrement to simply use the "MAX" query to basically have the same effect, but that's up to them to implement and verify. They wrote the code mis-using a MySQL feature that MySQL changed and I just don't think this is a justification for subjecting all OM users to performance regressions when there are plenty of other options.

Of course my opinion is not the end of the discussion and everyone is welcome to challenge and perhaps overrule it.

[jonashrem]

Maybe we should move the discussion to an issue as there are multiple possibilites here.

From my point of view, we have at the moment the following situation:

• currently, OM is using showTableStatus in way that breaks in MySQL8, so this should be resolved anyway ( https://github.com/OpenMage/magento-lts/blob/1.9.4.x/app/code/core/Mage/ImportExport/Model/Resource/Helper/Mysql4.php#L62 ). This can be done either by changing the showTableStatus function or by writing a better getNextAutoincrement function.
• plugins like cobby are using their on copy of that function that breaks. In case of cobby I don't know, why they have their own copy in the first place instead of using the one in ImportExport model (ping @dimajanzen )
• when showTableStatus is n ot changed, plugins can break with MySQL 8. While this can be easyly fixed with cobby, there migh be plugins with obfuscated code.
• in case, there will be a better implementation of getNextAutoincrement, it should also be ported to M2 from my opinion
• from my point of view, independend from the "analyze querry", the "show table status" querry should not be used at all in the beggining. (see also my and @leeps PR against M2 here Rewrote Magento\Framework\DB\Adapter\Pdo\Mysql->isTableExists magento/magento2#28516 where we removed it for isTableExists() )

So my conclusion would be:

• with this change here, compatibility with MySQL 8 will be increased without breaking changes
• additionally, getNextAutoincrement should be rewritten, to not use showTableStatus any more. (can be forward ported to M2)
• that one should also be used by plugin vendors when possible
• The same goes for isTableExists (can be backported from M2)

[dimajanzen]

We have made a copy of the getNextAutoincrement function from the Magento 1.5 ImportExport to support Magento 1.4 in cobby. :) Long time ago.

The showTableStatus method should be avoided, so we will create our own getNextAutoincrement.

For the next update of cobby, we have already changed the getNextAutoincrement function to use SELECT MAX(id) FROM table to support MySQL 8. ping @jonashrem

[jonashrem]

Very nice (and spooky) @dimajanzen

[colinmollenhour]

Great news @dimajanzen! Cobby looks like a really impressive system.

@jonashrem Are there any other known instances of this method being used to fetch the auto increment, particularly that are no longer maintained or are encoded? That is, now that Cobby is addressed is there a specific need for making a change here that cannot also be easily addressed?

Your M2 patch for isTableExists looks very good and I agree with using it. Minor aside: is it worth using the cache for this since it is so fast? Seems just running the query with no cache would be just as fast and less prone to stale cache problems and not require you to clear the ddl cache to guarantee the result is accurate.

I still think showTableStatus should run SHOW TABLE STATUS and nothing else. Nobody expects a simple SHOW TABLE STATUS to also query the db version, open a second database connection and run a low-level locking DDL statement in addition.

Agreed this could be bad news for some people when upgrading to MySQL 8 but the patch could also be bad news for other users. I see your points for maintaining BC, just not sure I agree. :)

[jonashrem]

@colinmollenhour I'm note aware of any specific other uses except the one in the stock Magento importer ( https://github.com/OpenMage/magento-lts/blob/1.9.4.x/app/code/core/Mage/ImportExport/Model/Resource/Helper/Mysql4.php#L62 ) which should be updated here.

In M2 the query has been used quite frequently with the introduction of MSI (like 100 times for a single page view), so in my opinion it was worth it bit I didn't run any benchmark to compare the cache to a version without the cache).

In M1 I haven't seen such a usage, so it shouldn't make a big difference here. On the other side, the change has been live in M2 since 2.4.2 without any stale issues I have heard.

I'll try to find some time tomorrow, to create a PR for back porting the isTableExists patch.

Thanks for your input in any case. This is very helpful.

[colinmollenhour]

the change has been live in M2 since 2.4.2 without any stale issues I have heard

I dunno about that, these kinds of issues are probably why M2 is so much slower than OM. :)

[jonashrem]

the change has been live in M2 since 2.4.2 without any stale issues I have heard

I dunno about that, these kinds of issues are probably why M2 is so much slower than OM. :)

So you'd prefer a version in OM without the cache?

[colinmollenhour]

So you'd prefer a version in OM without the cache?

Yes, if the underlying query takes only 1ms and returns a single integer then I don't think it is worth caching.

[jonashrem]

here you are: #1720

[Flyingmana]

And we can still do an improved version of it later, if it is actually causing issues.

• We should still make a new issue for the getNextIncrement. I also think its generally bad to use it, but who knows who used it, and they have to live with the issues of it... But we should add a clear warning in the doc block that it shouldnt be relied on.

• I see the showTableStatus also used in two places where its used to flush some cache.
  My guess would be, this is not needed anymore with newer mysql versions, but maybe someone has some more insights? Especially as it would only work with single server setups I guess.

• \Varien_Db_Adapter_Pdo_Mysql::modifyColumn
• \Varien_Db_Adapter_Pdo_Mysql::changeColumn

• probably the same cache thing:

• \Mage_Core_Model_Resource_Setup::setConfigData

        // this is a fix for mysql 4.1
        $this->getConnection()->showTableStatus($table);

• in \Varien_Db_Adapter_Pdo_Mysql::createTableByDdl its used to get the engine, I guess there is a better way.

        // Set additional options
        $tableData = $this->showTableStatus($tableName);
        $table->setOption('type', $tableData['Engine']);

• then the actual tableStatus is only used from \Mage_Backup_Model_Resource_Db::getTableStatus to estimate the size and number of rows of all tables.

[jonashrem]

getNextIncrement should work like this: #1721

I also send the same PR to M2 ( magento/magento2#33433 )

I think information schema should be used here instead of MAX id, as MAX id can be different when the latest element is deleted.

[fballiano]

@colinmollenhour what are your thoughts on this nowadays?

do we need to have a specialized code for mysql8?

[colinmollenhour]

@fballiano #1720 was merged and #1721 looks like a much better alternative so I think this PR should be rejected and #1721 considered for merge.