doctrine/dbal — pull request #669 by sarcher (2014-08-25 20:53:37+00:00)
Title: Correct schema generation for altering PostgreSQL sequences

I wrote a detailed explanation here so it is hopefully easy to follow:

#### Background

With PostgreSQL there are three states for a column to receive a sequence generator: no generator, an internal shortcut towards generating an auto incrementing sequence (SERIAL), and a manually-created sequence. In its current state, DBAL accepts only a boolean "autoincrement" to its `getAlterTableSql()` method (via the passed-in `TableDiff`).

This results in the following scenario:

- ORM maps a column to "AUTO" which is treated as a sequence (autoincrement = false)
- DBAL `PostgreSqlSchemaManager` inspects existing table and notices a sequence (autoincrement = true)
- Diff logic in `getAlterTableSql()` will always detect that autoincrement has changed, and that the requested value is false, so it will always issue a `DROP DEFAULT` statement

This is clearly a bug, and can be proved via a unit test; run the `AbstractPostgreSqlPlatformTestCase::testAlterSchemaSequenceToSequence` test that I have committed against the current DBAL code and you will see it fail (based on what is currently passed in via the ORM; see Q&A below for detailed explanation).

#### Solution

There already existed a few references to a not-yet-implemented `sequence` property of a column definition, which would store the name of the sequence being used on the column. This makes sense and allows us to support all three column states with regards to sequence, while also preserving backwards compatibility. The default is always null, so it will result in no changes to functionality on other platforms.

So, this PR:

- Adds the `Column::_sequence` property
- Correctly sets that property during the `PostgreSqlSchemaManager::_getPortableTableColumnDefinition` method (it actually was already there, just not being used)
- Checks the value during the `Comparator::diffColumn` operation
- Uses the value to more correctly determine when to create/drop a sequence during a schema alter
- Adds the appropriate unit tests to verify the six different combinations here

#### Q&A

_Is this just an ORM bug? Could the ORM be changed to just set autoincrement = true for the AUTO and SEQUENCE strategies?_

This would solve the immediate problem, yes. However, it would leave no possible distinction between the three ORM mapping strategies of AUTO, SEQUENCE, and IDENTITY. Further, it would cause a break in `getAlterTableSql()` because setting autoincrement to true implies that we are using the database-generated identity sequence which has a specific name, thereby removing the ability for a user to define a sequence manually with a custom name and have it be used here. This strategy opens the door for addressing those cases later, and does not cause a BC break today.

_This seems incomplete; for example, this still doesn't handle the case where a sequence is requested to change from AUTO to a specifically-named sequence._

That is intentional. I think these other cases could and probably should be handled, but they would require changes to both the DBAL and ORM. For example, we pass a `Sequence` object to the create/alter/drop sequence functions, but we do not pass one to the alter function. As a result, the actual sequence name is not available here, so we would absolutely need to make a more thought-out ORM change to solve this. I think that should be separate work, or it could just be something we do not support.

_Won't this still require an ORM change to set the `sequence` property?_

Yes, the following needs to be added to the `SchemaTool::gatherColumn` method:

```php
if ($class->isIdGeneratorSequence() && $class->getIdentifierFieldNames() == array($mapping['fieldName'])) {
$options['sequence'] = $class->sequenceGeneratorDefinition['sequenceName'];
}
```

However, even without that change, this solves the problem on the DBAL side and opens the door to fixing the ORM to behave correctly here.

_How do things behave today and how do we want them to behave with respect to alters?_

This is what _should_ happen in each alter scenario:

| Existing State | Mapped to AUTO or SEQUENCE | Mapped to IDENTITY (Auto Increment) | Regular Column |
| -------------- | ----------------------- | ----------------------------------- | -------------- |
| **No Sequence** | Add Sequence; Default to `nextval(seqeuence)` | Add Serial (Identity Sequence) | No Change |
| **Default `nextval(seqeuence)`** | No Change | No Change | Drop Default |

This is what _does_ happen today:

| Existing State | Mapped to AUTO or SEQUENCE | Mapped to IDENTITY (Auto Increment) | Regular Column |
| -------------- | ----------------------- | ----------------------------------- | -------------- |
| **No Sequence** | No Change (Bad) | Add Serial (Identity Sequence) | No Change |
| **Default `nextval(seqeuence)`** | Drop Default (Bad) | No Change | Drop Default |