rewrite isTableExists for performance reasons

[jonashrem]

Description (*)

implementation of isTableExists without the use of "show table status" to gain more performance.

See magento/magento2#28516 for details

This versions comes without the DDL cache, as magento1 is not using this function as much as magento 2.

Related Pull Requests

#1712

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)

[joshua-bn]

Have you seen any performance issues from this? Just curious.

[jonashrem]

in Magento1 I have not seen any specific issues with is, with an up to date cache implementation. Some old ones had some impact that was otherwise fixed.

In M2 this makes quite some difference.

[joshua-bn]

Thanks. I read your write up on M2. That was very detailed. Awesome report.

Seems they're doing a ton of checking if tables exist. Seems like a bad implementation if you can't trust that your tables exist. If they don't exist, throw some errors instead of having bad performance.

[fballiano]

This PR seems to have enough reviews to be merged.

[github-actions]

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
6 runs  ±0  4 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit bf76ce1. ± Comparison against base commit 7f84c55.

[Ruko2010]

@jonashrem there seams to be a typo in the function on line 2643

$ddl = $this->rawFetchRow($sql, 'tbl_exists');

i'll get a function not found error. The function in my codebase should be

$ddl = $this->raw_FetchRow($sql, 'tbl_exists');

[tmotyl]

checking the code base, I can confirm @Ruko2010 findings. We have a regression here.

[Ruko2010]

Just asking...should something like this not be found with automated tests? The missing funktion broke my whole OpenMage instantly 😃.

[fballiano]

created a super quick PR to fix the regression

[addison74]

That we are still talking these days about changing the RFC. This PR is proof that a solution must not be approved just because you read what it solves, but must be tested intensively. In this case the code was not complex and yet the typo was visible. It solved one problem and generated a much more serious OM malfunction. Fortunately teamwork should be appreciated.

[fballiano]

@addison74 the regression wasn't release to public, I don't feel like this is such a tremendous issue. I think nobody has the power to run intensive tests on anything sadly, but still the community is working extremely fine.

[tmotyl]

issue like this could be detected by phpstan.
the problem is that when somebody "Approves" a patch, we don't know whether its "looks good by reading" or "verified, it works" or both.

[jonashrem]

Just asking...should something like this not be found with automated tests? The missing funktion broke my whole OpenMage instantly smiley.

I'm sorry about that.

[addison74]

@tmotyl - Maybe when we approve a PR we should leave a few words there. LGTM, tested, ... When I approved a few PR's I implemented them on a test environment before and I tried to find any issues.