-
Notifications
You must be signed in to change notification settings - Fork 14
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
Generate a table hash that take account of the AUTO_INCREMENT changes #10
base: master
Are you sure you want to change the base?
Conversation
Even though this is probably mostly an edge case, it could be useful. @nojimage can you please rebase your PR? |
I confirm that the issue is relevant! Case: The empty table hash has not changed, but the autoincrement has already changed. In this case, the table is not cleared. Please accept this request. P.S. If necessary, I can do a rebase. |
@krugerman007 yes, please re-create this PR (with proper CS) based on your other PR, once they are merged. |
@krugerman007 can you please also check what a difference in speed peformance this change means? |
Sure: Original for 1372 Tests: With this fix: The difference is 4:27 - 3:32 = 0:55. It is 25% slower... |
$autoIncrementSth = $db->execute('SELECT `AUTO_INCREMENT` FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA=:schema AND TABLE_NAME=:table;', [ | ||
'schema' => $db->config()['database'], | ||
'table' => $this->table, | ||
]); | ||
$autoIncrementResult = $autoIncrementSth->fetch('assoc'); | ||
|
||
return (string) $autoIncrementResult['AUTO_INCREMENT']; |
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.
To prevent performance degredation, we could try to execute both queries in one go.
- Getting the table checksum
- Fetching the
AUTO_INCREMENT
info
Because apart from the additional queries to execute, the biggest part of the performance degredation of this change comes most likely from the PHP side.
And since it sems it not possible to query the table checksum as a sub query, we can only try the idea above.
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.
What do you mean with "both queries in one go"? We cannot combine it like this:
SELECT
(SELECT `AUTO_INCREMENT` ...) as auto_increment,
(CHECKSUM TABLE ...) as checksum;
Because the second query is not a select.
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.
Yeah, I tried that already.
But can we execute two queries with one execute()
call by splitting them like SELECT ...;CHECKSUM TABLE;
?
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.
Unfortunately I didn't find any way to get the result from the second query. Only the first query is returned:
// Debugging
$sth = $db->execute(
"CHECKSUM TABLE " . $this->table . ';' .
'SELECT `AUTO_INCREMENT` FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA=:schema AND TABLE_NAME=:table;',
[
'schema' => $db->config()['database'],
'table' => $this->table,
]
);
// How to get the result of the second query?
// $sth->fetch('assoc'); returns the result of the first one.
Another idea to prevent performance degredation is to add a configuration setting which enables/disables the additoinal query for either the whole plugin (all fixtures), for each fixture individually or even fo each test class individually. |
CHECKSUM TABLE
calculates a hash value for the current table record.This hash value does not include the value of AUTO_INCREMENT, it is not possible to track changes when only the value of AUTO_INCREMENT changes.
For example, if you add a record in a test case and delete that record.
If do not check the change of AUTO_INCREMENT, it will be occurred mismatch problem when assert the record id in the test case.