Skip to content
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

Add explanation about implicit indexes #4271

Merged
merged 1 commit into from
Sep 20, 2020

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Sep 11, 2020

Q A
Type docs
BC Break no
Fixed issues n/a

Summary

I'm unsure about the target branch: I wouldn't want this to block the next 2.10 release, which should be the last.
Also, I did not document the known issue about not supporting usage on table created by another tool that does not create implicit indexes where DBAL does not itself create some, because I think it will be fixed soonish, but tell me if you feel it would be worth mentioning

Also, I have named the parent directory explanation in to the divio naming, but I am also noticing there is a design (https://github.com/doctrine/dbal/tree/2.11.x/docs/design)
directory up one level that contains md files that make the docs website strangely (one can see these documents pop up in the search, but lead to 404s). Maybe that directory should be merged with explanation and its contents be merged with this one? Or maybe is it more of a cookbook? Anyway it would be great to decide beforehand if explanation is right name, or if we should name it design. I did not have a look at packages to see if either name might be already used.

And finally, does anyone know how I can build the docs locally?

Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it's a really well-written piece of explanation, I'm not sure how it's expected to help the DBAL users. It could be an explanation of how DBAL abstracts out the differences between platforms but it doesn't do that.

As far as I understand, DBAL will always create an explicit index that corresponds to the FK regardless of whether the underlying platform automatically creates it, so it's always covered.

I may be wrong about the facts, but the structure that I'd expect to see is the following:

  1. Different databases handle FK indexes differently.
  2. Our intent is to make sure that the indexes are always created (explicitly or implicitly, this is the implementation detail that the developer shouldn't be bothered about because of DBAL).
  3. Internally, if the platform creates explicit indices, this is what will happen, if it creates implicit indexes, then this, and if it doesn't, then this.

P.S.: 2.11.x looks good as the target branch.

lib/Doctrine/DBAL/Schema/Table.php Outdated Show resolved Hide resolved
@greg0ire
Copy link
Member Author

While it's a really well-written piece of explanation, I'm not sure how it's expected to help the DBAL users. It could be an explanation of how DBAL abstracts out the differences between platforms but it doesn't do that.

Good point about reducing the differences, I will try to add something about that

As far as I understand, DBAL will always create an explicit index that corresponds to the FK regardless of whether the underlying platform automatically creates it, so it's always covered.

I'm not sure about that. Consider the following piece of code:

foreach ($this->_indexes as $existingIndex) {
if ($indexCandidate->isFullfilledBy($existingIndex)) {
return;
}
}

When you introspect a table created with MySQL, won't the RDBMS-defined index be listed in $this->_indexes, and fulfill the index candidate? If not, it means we would have a duplicate DBAL-defined index for nothing.

I may be wrong about the facts, but the structure that I'd expect to see is the following:
Different databases handle FK indexes differently.
Our intent is to make sure that the indexes are always created (explicitly or implicitly, this is the implementation detail that the developer shouldn't be bothered about because of DBAL).
Internally, if the platform creates explicit indices, this is what will happen, if it creates implicit indexes, then this, and if it doesn't, then this.

Ok, I will try to rework this to match that structure.

@greg0ire greg0ire force-pushed the explanation-on-implicit-indexes branch from 7477d7e to d441c74 Compare September 12, 2020 08:32
@greg0ire
Copy link
Member Author

When you introspect a table created with MySQL, won't the RDBMS-defined index be listed in $this->_indexes, and fulfill the index candidate? If not, it means we would have a duplicate DBAL-defined index for nothing.

@guilhermeblanco hopefully you know about this? It looks like you are the most familiar with this feature.

@greg0ire
Copy link
Member Author

@morozov I overlooked your main question, let me address it.

I'm not sure how it's expected to help the DBAL users. It could be an explanation of how DBAL abstracts out the differences between platforms but it doesn't do that.

The goal I had in mind for this document was to answer the question "What are these weirdly-named indexes I can see appear when running migrations, and I did not ask for?". It's targeted at users, but also at us maintainers (it would have helped us understand one of the SQLite bugs faster).

Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these weirdly-named indexes I can see appear when running migrations, and I did not ask for?

This looks like the right question to me. Are those the implicit indexes in the title of the article or they are DBAL-defined? The other question is, will the user see them in the migration script or in the DB eventually? They will see DBAL-defined ones in the script but will only see the implicit ones in the deployed schema (e.g. on MySQL).

I apologize if it sounds like a criticism but if that is the primary question you want to answer, maybe the following structure could be used?

  1. Why am I seeing those indices? They are needed for FKs to perform well.
  2. Some platforms will take care of them and generate automatically. Then DBAL will not generate them in migration scripts and will rely on the platform. Maybe a code example (again, if that' the case, I don't know).
  3. Some platforms don't care. Then DBAL steps in: example.

I admit that I may misunderstand the purpose of the article. This is by no means an attempt to direct here.

docs/en/explanation/implicit-indexes.rst Outdated Show resolved Hide resolved
docs/en/explanation/implicit-indexes.rst Outdated Show resolved Hide resolved
docs/en/explanation/implicit-indexes.rst Outdated Show resolved Hide resolved
@greg0ire greg0ire force-pushed the explanation-on-implicit-indexes branch from d441c74 to bd33ad7 Compare September 12, 2020 16:34
@greg0ire greg0ire changed the title Add explanation about explicit indexes Add explanation about implicit indexes Sep 12, 2020
@greg0ire greg0ire force-pushed the explanation-on-implicit-indexes branch from bd33ad7 to 1c1821e Compare September 12, 2020 16:38
@greg0ire
Copy link
Member Author

greg0ire commented Sep 12, 2020

What are these weirdly-named indexes I can see appear when running migrations, and I did not ask for?
This looks like the right question to me. Are those the implicit indexes in the title of the article or they are DBAL-defined?

Weirdly-named indexes are DBAL-defined.

You can see here the name that MariaDB picks for an implicit index: https://dbfiddle.uk/?rdbms=mariadb_10.5&fiddle=11d4234c37b192c46cd6566efdb9799c (it picks trackartist, so… less weird?)

I admit that I may misunderstand the purpose of the article. This is by no means an attempt to direct here.

There are several purposes, the main purpose is to answer that first question, the secondary purposes would be to serve as a reference when a maintainer such as you and I see these indexes being talked about in bug reports, and don't remember what they are for, and also to explain the decisions that lead to this, so that people can better challenge these decisions if necessary. You did so a little IIRC, when you said you thought this might be a disservice to the user.

@greg0ire greg0ire force-pushed the explanation-on-implicit-indexes branch 2 times, most recently from 7e1beb8 to c179c01 Compare September 12, 2020 17:06
@greg0ire
Copy link
Member Author

greg0ire commented Sep 12, 2020

@morozov I had a doubt about how this works so I tried the demo symfony application (symfony new --demo my_project) with a mariadb server started with DB=mariadb.docker IMAGE=mariadb:10.3 bash ./tests/travis/docker-run-mysql-or-mariadb.sh
the connection string that can be changed in .env is mysql://root@127.0.0.1:33306/doctrine_tests, and sure enough, it also creates the DBAL-defined indexes for MariaDB. Using SHOW INDEXES FROM symfony_demopost shows the DBAL-defined index. If I comment out the code block responsible for that index, and recreate the schema, I still have 2 indexes, but this time the index is named FK_… (exactly like the foreign key) so this must be an RDBMS-defined index.

This does not work like I thought it would: the DBAL always creates that index regardless of whether the platform creates implicit indexes or not, and that makes sense since it can't read the table and detect that beforehand since it does not exist yet. Instead the DBAL leverages the feature that I describe in the last paragraph of the explanation:

Some RDBMS such as MySQL do something similar and can also drop implicit indexes that are fulfilled by user-defined indexes.

I will have to rework this but one thing is for sure, this explanation will have to be reviewed by people familiar with that feature otherwise misconceptions might slip in.

@morozov
Copy link
Member

morozov commented Sep 12, 2020

and can also drop implicit indexes

This still looks like a misconception to me. In my understanding, MySQL will ensure that the FK is covered by an index. If the index isn't provided by the user, it will create its own. I.e. it doesn't drop anything.

@greg0ire
Copy link
Member Author

If you take https://dbfiddle.uk/?rdbms=mariadb_10.5&fiddle=11d4234c37b192c46cd6566efdb9799c as an example, you can see an RDBMS-defined index.

Now if you add an index that fulfills the same needs, this is what you obtain:

https://dbfiddle.uk/?rdbms=mariadb_10.5&fiddle=d40ff6c0242591ff4163b96ecda23f3b

To me, it looks like MariaDB dropped the existing index

@greg0ire
Copy link
Member Author

greg0ire commented Sep 12, 2020

The MariaDB docs I linked to are not super clear about this:

If MariaDB automatically creates an index for the foreign key (because it does not exist and is not explicitly created), its name will be index_name

They make it sound like MariaDB decides to add an index because there is none, but don't say it will be dropped if you add one.

The Mysql docs I linked to are a bit more clear:

MySQL requires indexes on foreign keys and referenced keys so that foreign key checks can be fast and not require a table scan. In the referencing table, there must be an index where the foreign key columns are listed as the first columns in the same order. Such an index is created on the referencing table automatically if it does not exist. This index might be silently dropped later if you create another index that can be used to enforce the foreign key constraint. index_name, if given, is used as described previously.

@morozov
Copy link
Member

morozov commented Sep 12, 2020

Thanks for the details, @greg0ire. I misinterpreted the earlier statement. I believe the same logic that is explicitly defined for MySQL is also true for MariaDB.

Some RDBMS such as MySQL do something similar and can also drop implicit indexes that are fulfilled by user-defined indexes.

Probably this confusion could be avoided if we put it as “can drop an existing implicit index once it's fulfilled by a newly created user-defined index”.

@greg0ire greg0ire force-pushed the explanation-on-implicit-indexes branch from c179c01 to eea4d26 Compare September 12, 2020 21:21
@greg0ire
Copy link
Member Author

@morozov I just pushed a reworked version, the only thing I did not fix is the title, which, as you pointed out, is ambiguous: are we talking about DBAL-defined indexes, or about RDBMS-defined indexes? The document covers both, so I think it can stay as is.

@greg0ire greg0ire force-pushed the explanation-on-implicit-indexes branch 2 times, most recently from 810545e to 3e881b6 Compare September 12, 2020 22:02
Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Structure- and content-wise, looks good. A couple of minor notices.

docs/en/explanation/implicit-indexes.rst Outdated Show resolved Hide resolved
docs/en/explanation/implicit-indexes.rst Outdated Show resolved Hide resolved
@greg0ire greg0ire force-pushed the explanation-on-implicit-indexes branch from 3e881b6 to dd0c751 Compare September 13, 2020 07:06
Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@SenseException
Copy link
Member

but I am also notice there is a design (https://github.com/doctrine/dbal/tree/2.11.x/docs/design) directory up one level that contains md files that make the docs website strangely

Azure won't be much of an issue anymore starting with 3.0. The documentation already has a sharding section and because the last changes of these two files in design seem to be 3 years ago, I would suggest to delete the directory in a PR.

@greg0ire greg0ire added this to the 2.11.0 milestone Sep 20, 2020
@greg0ire greg0ire merged commit 685f24b into doctrine:2.11.x Sep 20, 2020
@greg0ire greg0ire deleted the explanation-on-implicit-indexes branch September 20, 2020 15:21
morozov pushed a commit that referenced this pull request Sep 21, 2020
rgrellmann added a commit to Rossmann-IT/dbal that referenced this pull request Mar 7, 2021
Release [2.11.0](https://github.com/doctrine/dbal/milestone/77)

2.11.0
======

- Total issues resolved: **7**
- Total pull requests resolved: **55**
- Total contributors: **8**

Improvement,Prepared Statements,SQL Server,Types,pdo_sqlsrv,sqlsrv
------------------------------------------------------------------

 - [4274: Support ASCII parameter binding](doctrine#4274) thanks to @gjdanis

Documentation
-------------

 - [4271: Add explanation about implicit indexes](doctrine#4271) thanks to @greg0ire

Deprecation,Error Handling
--------------------------

 - [4253: Deprecate DBAL\DBALException in favor of DBAL\Exception](doctrine#4253) thanks to @morozov

CI
--

 - [4251: Setup automatic release workflow](doctrine#4251) thanks to @greg0ire

Deprecation,Schema Managers
---------------------------

 - [4230: Deprecate the functionality of dropping client connections when dropping a database](doctrine#4230) thanks to @morozov

Deprecation,Platforms
---------------------

 - [4229: Deprecate more AbstractPlatform methods](doctrine#4229) thanks to @morozov
 - [4132: Deprecate AbstractPlatform::fixSchemaElementName()](doctrine#4132) thanks to @morozov

Improvement,Test Suite
----------------------

 - [4215: Remove test group configuration leftovers](doctrine#4215) thanks to @morozov
 - [4080: Update PHPUnit to 9.2](doctrine#4080) thanks to @morozov
 - [4079: Forward compatibility with PHPUnit 9.3](doctrine#4079) thanks to @morozov
 - [3923: Removed performance tests](doctrine#3923) thanks to @morozov

Deprecation,Schema
------------------

 - [4213: Deprecate the Synchronizer package](doctrine#4213) thanks to @morozov

Blocker,Improvement,PHP,Test Suite
----------------------------------

 - [4201: Update PHPUnit to 9.3](doctrine#4201) thanks to @morozov

Blocker,PHP,Test Suite
----------------------

 - [4196: The test suite uses the deprecated at() matcher](doctrine#4196) thanks to @morozov

Connections,Deprecation,Documentation
-------------------------------------

 - [4175: Additional deprecation note for PrimaryReplicaConnection::query()](doctrine#4175) thanks to @morozov

Connections,Deprecation,Prepared Statements
-------------------------------------------

 - [4165: Deprecated usage of wrapper components as implementations of driver-level interfaces](doctrine#4165) thanks to @morozov
 - [4020: Deprecated Connection::project(), Statement::errorCode() and errorInfo()](doctrine#4020) thanks to @morozov

Connections,Deprecation
-----------------------

 - [4163: Deprecate duplicate and ambiguous wrapper connection methods](doctrine#4163) thanks to @morozov

Error Handling,Improvement,Types
--------------------------------

 - [4145: Add TypeRegistry constructor](doctrine#4145) thanks to @morozov

Deprecation,Drivers,Improvement,pdo_mysql,pdo_oci,pdo_pgsql,pdo_sqlite,pdo_sqlsrv
---------------------------------------------------------------------------------

 - [4144: Deprecate classes in Driver\PDO* namespaces](doctrine#4144) thanks to @morozov

Connections,Documentation,Improvement
-------------------------------------

 - [4139: Mark connection constructors internal](doctrine#4139) thanks to @morozov

Deprecation,Drivers,Error Handling
----------------------------------

 - [4137: Deprecate driver exception conversion APIs](doctrine#4137) thanks to @morozov
 - [4112: Deprecate DriverException::getErrorCode()](doctrine#4112) thanks to @morozov

Configuration,Connections,Deprecation,Error Handling
----------------------------------------------------

 - [4134: Deprecate some DBALException factory methods](doctrine#4134) thanks to @morozov

Code Style,Documentation
------------------------

 - [4133: Fix more issues introduced by the deprecation of driver classes](doctrine#4133) thanks to @morozov

BC Break,Drivers,Error Handling,pdo_sqlsrv,sqlsrv
-------------------------------------------------

 - [4131: Restore the PortWithoutHost exception parent class](doctrine#4131) thanks to @morozov

Code Style,Improvement,Static Analysis
--------------------------------------

 - [4123: Remove the no longer needed error suppressions](doctrine#4123) thanks to @morozov

Deprecation,Drivers,Error Handling,Improvement
----------------------------------------------

 - [4118: Deprecate ExceptionConverterDriver](doctrine#4118) thanks to @morozov

Bug,Connections,Improvement,Prepared Statements
-----------------------------------------------

 - [4117: Fixes for the recently introduced driver-level deprecations](doctrine#4117) thanks to @morozov

Connections,Deprecation,Platform Detection
------------------------------------------

 - [4114: Deprecate ServerInfoAwareConnection#requiresQueryForServerVersion() as an implementation detail](doctrine#4114) thanks to @morozov

Deprecation,Prepared Statements,SQL Parser,oci8
-----------------------------------------------

 - [4110: Mark non-interface OCI8 driver methods internal](doctrine#4110) thanks to @morozov

Connections,Deprecation,Drivers,Improvement,Prepared Statements
---------------------------------------------------------------

 - [4100: Deprecate inconsistently and ambiguously named driver-level classes](doctrine#4100) thanks to @morozov

Connections,Improvement
-----------------------

 - [4092: Remove Connection::$isConnected](doctrine#4092) thanks to @morozov

Configuration,Connections
-------------------------

 - [4086: Mark Connection::getParams() internal](doctrine#4086) thanks to @morozov

Bug,Drivers,ibm_db2
-------------------

 - [4085: The IBM DB2 driver Exception class must implement the DriverException interface](doctrine#4085) thanks to @morozov

PHP
---

 - [4078: Bump PHP requirement to 7.3 as of DBAL 2.11.0](doctrine#4078) thanks to @morozov

Connections,Databases,Deprecation,Drivers
-----------------------------------------

 - [4068: Deprecate Driver::getDatabase()](doctrine#4068) thanks to @morozov

Deprecation,Improvement,Portability
-----------------------------------

 - [4061: Deprecated platform-specific portability mode constants](doctrine#4061) thanks to @morozov

 - [4054: &doctrine#91;doctrineGH-4052&doctrine#93; Deprecate MasterSlaveConnection and rename to PrimaryReplicaConnection](doctrine#4054) thanks to @beberlei and @albe
 - [4017: Improve help of dbal:run-sql command](doctrine#4017) thanks to @ostrolucky

Code Style,Improvement
----------------------

 - [4050: Update doctrine/coding-standard to 8.0](doctrine#4050) thanks to @morozov

Cache,Deprecation,Improvement,Prepared Statements
-------------------------------------------------

 - [4049: Replace forward-compatible ResultStatement interfaces with Result](doctrine#4049) thanks to @morozov

Cache,Improvement,Prepared Statements
-------------------------------------

 - [4048: Make caching layer not rely on closeCursor()](doctrine#4048) thanks to @morozov

Deprecation,Improvement,Prepared Statements
-------------------------------------------

 - [4037: Introduce Statement::fetchFirstColumn()](doctrine#4037) thanks to @morozov
 - [4019: Deprecated the concept of the fetch mode](doctrine#4019) thanks to @morozov

Bug,Documentation,Improvement,Prepared Statements
-------------------------------------------------

 - [4034: Additional changes based on the discussion in doctrine#4007](doctrine#4034) thanks to @morozov

Connections,Console,Improvement
-------------------------------

 - [3956: allow using multiple connections for CLI commands](doctrine#3956) thanks to @dmaicher

Deprecation,Logging
-------------------

 - [3935: Deprecate EchoSQLLogger](doctrine#3935) thanks to @morozov

Improvement,Packaging
---------------------

 - [3924: Actualize the content of the .gitattributes file](doctrine#3924) thanks to @morozov

Azure,Deprecation,Drivers,Drizzle,MariaDB,Platforms,PostgreSQL,SQL Anywhere,SQL Server,Sharding,pdo_ibm
-------------------------------------------------------------------------------------------------------

 - [3905: Deprecate the usage of the legacy platforms and drivers](doctrine#3905) thanks to @morozov

Deprecation,Query
-----------------

 - [3864: CompositeExpression and()/or() factory methods](doctrine#3864) thanks to @BenMorel
 - [3853: Deprecate calling QueryBuilder methods with an array argument](doctrine#3853) thanks to @BenMorel and @morozov

Deprecation,Tools
-----------------

 - [3861: Deprecated the usage of the Version class](doctrine#3861) thanks to @morozov

Improvement,Query
-----------------

 - [3852: First parameter of ExpressionBuilder::and/or() mandatory](doctrine#3852) thanks to @BenMorel

Deprecation,Improvement,Query
-----------------------------

 - [3851: Rename andX() / orX() methods](doctrine#3851) thanks to @BenMorel
 - [3850: Prepare CompositeExpression for immutability](doctrine#3850) thanks to @BenMorel

# gpg: Signature made Mon Sep 21 01:47:31 2020
# gpg:                using DSA key 1BEDEE0A820BC30D858F9F0C2C3A645671828132
# gpg: Can't check signature: No public key

# Conflicts:
#	README.md
#	lib/Doctrine/DBAL/Driver/AbstractOracleDriver.php
#	lib/Doctrine/DBAL/Driver/OCI8/Driver.php
#	lib/Doctrine/DBAL/Version.php
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants