Update getNextAutoincrement to not use showTableStatus

[jonashrem]

Description (*)

update getNextAutoincrement to not use showTableStatus for performants reasons.

Related Pull Requests

magento/magento2#33433
#1712

Fixed Issues (if relevant)

/

Manual testing scenarios (*)

test import/export

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]

Looks great, what is the oldest version of MySQL where this code will work?

[fballiano]

was this code tested on OM?

[colinmollenhour]

I confirmed that the "SELECT AUTO_INCREMENT FROM INFORMATION_SCHEMA.TABLES" query works on MySQL 5.6 and up and MariaDb 10.3 and up. (probably some earlier versions, these are just the oldest I checked)

I've not tested an import in Magento, @jonashrem did you test an import with the latest version of this PR?

[fballiano]

I've rebased this and tested it briefly creating new customer/addresses/products, the autoincrements work fine.

I've no idea if this is really a performance improvement or if it could create a problem.

But now it's ready to be merged, or it should be closed.

[jonashrem]

Thanks

[colinmollenhour]

Approve, but I didn't perform a end-to-end test.

@jonashrem Please confirm this has been tested in it's current state. Thanks!

[jonashrem]

sorry this got under.

I did not test the current variant with up to date OpenMage.

Do you have some specific test szenario in mind?

[fballiano]

I also will test this out as soon as possible

[kiatng]

Tested. It is faster by about 50% to sub mini-sec from 1.5ms.

Mage::getResourceHelper('importexport')->getNextAutoincrement('sales_flat_order_item');

[fballiano]

is it possible that the result of this call gets cached somewhere?

on my DB I've an autoincrement value of 65:

but when I run this:

I get 63:

[colinmollenhour]

is it possible that the result of this call gets cached somewhere?

Very odd, I don't see how that is possible.. Are you 100% certain the two tests were run against the same database?

[elidrissidev]

is it possible that the result of this call gets cached somewhere?

I encountered this issue the first time I tested this PR but forgot to post it. It seems performance_schema is not guaranteed to contain the most up-to-date value, sometimes ANALYZE TABLE is necessary to get accurate results. Maybe just a simple MAX() + 1 query would do the job here.

[fballiano]

@colinmollenhour I'm sure it is the same database

@elidrissidev AFAIK MAX+1 is not safe in some cases, I don't remember exactly on top of my head, zf1 autoincrement function uses the "last insert id", would that be better at that point? or just leave everything is it is now?

[colinmollenhour]

Ahh, I still don't see how essentially the same query via the console and via PHP can have different results, but it appears the information schema data is indeed cached within MySQL for an entire 24 hours by default. This can be controlled by information_schema_stats_expiry. This can be set as a session variable just before the query and yield correct results which isn't a terrible approach, but not sure if that is really an improvement in performance at that point.

Perhaps your IDE is setting this session variable behind the scenes and that is why the results are different?

https://stackoverflow.com/a/65996809/187780

The MAX(id) seems to be just as fast. It might not be accurate if there were deletions or if there were simultaneous transactions that incremented the auto increment but have not been committed yet or were rolled back (I'm not 100% sure on this but somewhat certain). However, that may not be an issue or even a regression - is this method going to be used in a multi-threaded environment?

[fballiano]

the problem I see is when I run from the PHP CLI, not from the Mysql client (I use SequelAce) which anyway has an expire of 0

[fballiano]

but actually if I run php cli I get an expire of 24h

weird... I'll try to find where is this set...

[colinmollenhour]

It's probably just a feature of SequelAce so it makes sense now. :)

[fballiano]

If I do something like this:

then it works ok.

not sure about all of this :-\

[colinmollenhour]

In my tests information_schema method takes about half the time of show table status, but considering you have to set a session variable first it requires two round trips at least on the first use. And then there is possibly a performance penalty of disabling the information schema cache which I have no idea how to measure. The MAX(id)+1 method is the fastest by far. This is on a very big and active table from a production database.

So I think the MAX()+1 method is a very good suggestion as long as this method isn't used with tables that have deleted rows. If that is not a safe assumption to make (probably isn't) then I think the old method should be kept and no change made since there is not that much benefit for increased complexity and uncertainty.

Perhaps the better change would be to update where this method is used to only call the method once or use MAX(id)+1 instead but leave getNextAutoincrement alone?

[colinmollenhour]

I don't think stats cache expiry should be set globally if it is not absolutely necessary - that should be up to the DBA to decide ideally. It could be set just before querying information schema, perhaps using a static variable so that it is only done once.

[fballiano]

sure, that's why I was testing it in the initStatement of the openmage connection, if we remove the GLOBAL it still works

[fballiano]

I have to block the merge because of #1721 (comment)

what are we doing about this?

[kiatng]

I think we should close it.

[fballiano]

me too, unless something completely different comes out, this is too risky