SQLite - introspecting a composite primary key wrongly turns it into autoincrement

[fisharebest]

Bug Report

Q	A
Version	4.4.1 (also 4.5.x-dev)
Previous Version if the bug is a regression	x.y.z

Summary

In SQLite, a primary key consisting of an integer column is implicitly autoincrement.
However, a composite primary key does not have this.

This is demonstrated by this SQLite session, which shows the implicit behavour (table user) and that it doesn't apply to composite keys (table user_setting).

SQLite version 3.43.2 2023-10-10 13:08:14
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
sqlite> CREATE TABLE user (id integer not null primary key, username varchar not null, password varchar not null);
sqlite> INSERT INTO user (username, password) VALUES ('admin', 'secret');
sqlite> SELECT * FROM user;
1|admin|secret
sqlite> 
sqlite> CREATE TABLE user_setting (user_id integer not null, setting_name varchar not null, setting_value varchar not null, primary key (user_id, setting_name));
sqlite> INSERT INTO user_setting (setting_name, setting_value) VALUES ('role', 'admin');
Runtime error: NOT NULL constraint failed: user_setting.user_id (19)

Current behavior

Introspecting the user_setting.user_id column in the above example wrongly shows it as autoincrement.

Furthermore, the primary key is introspected as ONLY consisting of the integer column, and the second column is lost.

Expected behavior

The autoincrement attribute should only be set when the primary key consists of a single integer column.

How to reproduce

<?php

use Doctrine\DBAL\DriverManager;
use Doctrine\DBAL\Schema\ForeignKeyConstraint;
use Doctrine\DBAL\Schema\Schema;

require 'vendor/autoload.php';

$connection = DriverManager::getConnection(['path' => ':memory:', 'driver' => 'pdo_sqlite']);

$connection->executeStatement('CREATE TABLE user (id integer not null primary key, username varchar not null, password varchar not null)');
$connection->executeStatement('CREATE TABLE user_setting (user_id integer not null, setting_name varchar not null, setting_value varchar not null, primary key (user_id, setting_name))');

$platform       = $connection->getDatabasePlatform();
$schema_manager = $connection->createSchemaManager();
$comparator     = $schema_manager->createComparator();
$old_schema     = $schema_manager->introspectSchema();

// This column is not auto-increment
$autoincrement = $schema_manager->introspectSchema()->getTable('user_setting')->getColumn('user_id')->getAutoincrement();

var_dump($autoincrement);  // true, but should be false

// Modify the table (add a FK)
$new_schema = new Schema([
    $old_schema->getTable('user'),
    $old_schema->getTable('user_setting')
        ->edit()
        ->addForeignKeyConstraint(new ForeignKeyConstraint(['user_id'], 'user', ['id']))
        ->create(),
]);

$queries = $platform->getAlterSchemaSQL($comparator->compareSchemas($old_schema, $new_schema));

// Breaks the PK - tries to change it to JUST the numeric column.
foreach ($queries as $query) {
    echo $query, PHP_EOL;
}

Script output. Note the CREATE TABLE command turns the composite primary key into a single-column primary key.

bool(true)
CREATE TEMPORARY TABLE __temp__user_setting AS SELECT user_id, setting_name, setting_value FROM user_setting
DROP TABLE user_setting
CREATE TABLE user_setting (user_id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, setting_name VARCHAR NOT NULL, setting_value VARCHAR NOT NULL, FOREIGN KEY (user_id) REFERENCES user (id) NOT DEFERRABLE INITIALLY IMMEDIATE)
INSERT INTO user_setting (user_id, setting_name, setting_value) SELECT user_id, setting_name, setting_value FROM __temp__user_setting
DROP TABLE __temp__user_setting
CREATE INDEX IDX_C779A692A76ED395 ON user_setting (user_id)

[morozov]

This looks like a duplicate of #6844.

[fisharebest]

I do not think this is a duplicate.

#6844 says essentially:

When DBAL declares an SQLite column, it adds the AUTOINCREMENT keyword only if the column has the autoincrement attribute:

This is correct, and AFAICT that functionality works as expected.

My new issue concerns COMPOSITE primary keys. The autoincrement behaviour does not apply to composite primary keys - only to those consisting of a single integer column.

In my test case, we create a composite primary key primary key (user_id, setting_name).

dbal sees the integer column in a primary key, and sets it (wrongly) to be autoincrement.

Thus when we update the table (adding a FK in my example), dbal tries to recreate the PK with just one column user_id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL

[morozov]

Does #6847 look relevant?

When building the table DDL, if there is an auto-increment column, the DBAL will silently ignore other primary key columns

Please try testing the same scenario on 5.0.x, this issue may be fixed by one of the changes there.

[fisharebest]

Hi @morozov - yes, I can now see the issue is fixed in 5.x.

For 4.4.x and 4.5.x, I think I have found the problem. It's a simple two-line fix.

The code builds a list of the PK columns here. But it only includes INTEGER columns.

dbal/src/Schema/SQLiteSchemaManager.php

Line 470 in 207d910

if ($row['pk'] === 0 || $row['pk'] === '0' || $row['type'] !== 'INTEGER') {

Then here, it looks to see if the column under consideration matches the above list (of PK columns excluding non-integers).

dbal/src/Schema/SQLiteSchemaManager.php

Line 484 in 207d910

&& $pkColumnNamesByTable[$tableName] === [$columnName],

In the first line, we should remove || $row['type'] !== 'INTEGER' so that we build a list of all PK columns.

Then, in the second line, we should add && $row['type'] === 'INTEGER' so that we check whether the column under consideration matches the PK columns and it is also an integer.

If this change sounds right to you, then would you like me to create a PR?

[morozov]

In the first line, we should remove || $row['type'] !== 'INTEGER' so that we build a list of all PK columns.

This sounds reasonable. Excluding a column from the PK just because it's not of type INTEGER looks wrong.

we should add && $row['type'] === 'INTEGER' so that we check whether the column under consideration matches the PK columns and it is also an integer.

I think it's more subtle than that. IIRC SQLite will auto-increment the column only if it's the only column in the PK.

If this change sounds right to you, then would you like me to create a PR?

In general, this all sounds right, and this issue does need to be fixed (which is done in 5.0.x). I'm fine with attempting to fix it in an earlier version, but I'm not sure about the BC impact. I didn't want to deal with it, and that's why I fixed it in 5.0.x. I would definitely refrain from making this change in a patch release. The next minor is the best we can do.

You could try making this change and providing a BC impact analysis, if you want. I can't promise this change will be accepted though.

[fisharebest]

I think it's more subtle than that. IIRC SQLite will auto-increment the column only if it's the only column in the PK.

If it is the only column and it is an integer.

Which is what the code should do after this suggested fix.

I would definitely refrain from making this change in a patch release. The next minor is the best we can do.

So, 4.5.x then?

I can convert my informal tests into proper test scripts and create a PR in the next few days.

[morozov]

If it is the only column and it is an integer.

Yes.

So, 4.5.x then?

I can convert my informal tests into proper test scripts and create a PR in the next few days.

Sounds good.