Optimize schema managers' ::listTables() methods

[morozov]

Q	A
Type	improvement

Fixes #2676
Closes #2766
Closes #4882

Change summary

If an entire schema needs to be introspected, instead of listing the tables and then introspecting each table individually, the new implementation fetches all schema objects of each type in one query and then groups them into tables. This reduces the number of queries used for introspection from O(N) (where N is the number of tables in the schema) to O(1).

Design considerations

From the design standpoint, schema introspection deserves a separate API. Similar to the wrapper connection and the platform classes, the schema manager is already a God object.

Introducing a schema introspection API is the next logical step in improving the quality of the codebase but I decided not to do it now:

1. The performance aspect (which is the problem the API consumers are facing) could be addressed without changing the API.
2. There is a lot of technical debt that makes the introduction of such an API non-trivial (although there's nothing impossible, here's a PoC).
3. The time I can spend on this right now is quite limited.

Proposed design

All common logic for introspecting the entire schema in one shot is implemented in the abstract schema manager class. In order to be able to introduce new abstract methods, we introduce a temporary internal AbstractIntrospectingSchemaManager and extend all concrete schema managers from it. This temporary class will be merged into AbstractSchemaManager in the next major release. See #5268 (comment).

Instead of using the now deprecated AbstractPlatform::getList*SQL() methods, the schema managers build the queries themselves. This has the following advantages:

1. The logic of building queries and fetching their results is now more coherent (located within the schema manager).
2. This allows for using prepared statements in the schema introspection queries.
3. In the future, it will allow removing the column aliases like SELECT COLUMN_NAME AS field from the schema introspection queries since each schema manager will process the result of the query that it builds itself for itself.

Performance considerations

From my previous experience with Oracle, the time a query against a schema introspection view takes doesn't depend that much on whether a single table or the entire schema is introspected. Introspecting N tables via an individual query each would take N times longer than introspecting them all at once.

In my development environment, the integration test suite takes ~40% less time to run with this change:

$ git checkout 3.4.x
$ phpunit -c oci8.phpunit.xml tests/Functional
PHPUnit 9.5.11 by Sebastian Bergmann and contributors.

...

Time: 01:50.276, Memory: 34.00 MB

$ git checkout optimize-list-tables
$ phpunit -c oci8.phpunit.xml tests/Functional

PHPUnit 9.5.11 by Sebastian Bergmann and contributors.

...

Time: 01:05.771, Memory: 34.00 MB

Compatibility considerations

Depending on whether the AbstractPlatform::getList*SQL() methods are considered part of the SPI, the proposed changes may imply a BC break. If an API consumer uses a bundled platform and a schema manager and overrides one of these methods, the overridden logic will be no longer taken into account since the schema managers now use their own implementation.

Until the next major release, the implementation of the AbstractPlatform::getList*SQL() methods remains intact, tested but no longer used.

Deprecations

The public Platform methods that generated SQL for introspecting a single table have been deprecated.

Implementation details

Changes in schema introspection logic:

1. IBM DB2:
   1. Compare COLSEQ when joining referencing and referenced columns of a foreign key constraint. Otherwise, the query would return a product of the two sets and result in columns being returned out of order:

      1) Doctrine\DBAL\Tests\Functional\Schema\Db2SchemaManagerTest::testCreatedCompositeForeignKeyOrderIsCorrectAfterCreation
      Failed asserting that two arrays are identical.
      --- Expected
      +++ Actual
      @@ @@
       Array &0 (
      -    0 => 'col2'
      -    1 => 'col1'
      +    0 => 'col1'
      +    1 => 'col2'
       )
      

      Before:

      dbal/src/Platforms/DB2Platform.php

      Lines 410 to 413 in 699aad8

      	JOIN SYSCAT.KEYCOLUSE AS pkcol
      	ON fk.REFKEYNAME = pkcol.CONSTNAME
      	AND fk.REFTABSCHEMA = pkcol.TABSCHEMA
      	AND fk.REFTABNAME = pkcol.TABNAME

      
      After:

      dbal/src/Schema/DB2SchemaManager.php

      Lines 350 to 354 in 699aad8

      	JOIN SYSCAT.KEYCOLUSE AS PKCOL
      	ON PKCOL.CONSTNAME = R.REFKEYNAME
      	AND PKCOL.TABSCHEMA = R.REFTABSCHEMA
      	AND PKCOL.TABNAME = R.REFTABNAME
      	AND PKCOL.COLSEQ = FKCOL.COLSEQ

   2. Added the WHERE TABSCHEMA = <database> clause to the column, index, etc. introspection queries. Otherwise, they might return objects from other schemas.
   3. Removed the "We do the funky subquery […]" logic. It existed to have a DISTINCT clause in the query (which is rarely a good idea) which in turn was used to select the columns that weren't used anywhere.

TODO:

• Document semantics of AbstractIntrospectingSchemaManager::normalizeIdentifier().
• Document the new API and the design decisions.

[greg0ire]

This looks great 👍

Please document deprecations in UPGRADE.md

[greg0ire]

Speaking of improvements makes sense in a commit message or PR, but in code, one might wonder what is the slow version of this.

Suggested change

	* Base class for schema managers that improves database schema introspection performance.
	* Base class for schema managers that provides good database schema introspection performance.

[mondrake]

Maybe, just avoid qualitative statements at all

Base class for schema managers that provides database schema introspection.

[morozov]

IMO, the original "improves" is more appropriate than "provides good". But I believe I want to get rid of this class entirely as follows:

1. Declare the new abstract methods in the existing class as non-abstract with the throw new NotImplemented() body.
2. Create temporary deprecated methods that leverage the new API (e.g. protected doListTableDetails()).
3. In those classes that want to opt into the new API (all bundled schema managers), implement the abstract methods (already done) and instead of inheriting the default implementation of listTableDetails() from the base class, call doListTableDetails().

Otherwise, if we want to do more rework like this in the next minor release (e.g. listing sequences, users, etc.), we'll have to introduce another temporary abstract class turning it into a mess.

[mondrake]

Well, this is an advancement! 🚀🚀🚀

Thank you