make autoincrement=True less surprising in composite PK setups...somehow

[Adrian]

E.g. in this example:

class Foo(Base):                                   
    __tablename__ = 'foo'
    id1 = Column(Integer, primary_key=True, nullable=False)
    id2 = Column(Integer, primary_key=True, nullable=False)

I believe it does not make much sense to create id1 as autoincrement/serial while id2 is a plain integer columns.

Unless there are database engines which actually support multiple serial columns, I would either disable autoincrement for both columns in that case or raise an error, forcing the user to enable/disable autoincrement explictly.

[Mike Bayer]

mysql> create table foo (id1 integer auto_increment, id2 integer, primary key (id1, id2));
Query OK, 0 rows affected (0.01 sec)

mysql> insert into foo(id2) values (2);
Query OK, 1 row affected (0.00 sec)

mysql> select  * from foo;
+-----+-----+
| id1 | id2 |
+-----+-----+
|   1 |   2 |
+-----+-----+
1 row in set (0.00 sec)

mysql> 

I'm pretty sure a lot of people want this.

[Adrian]

But doesn't it make sense to be explicit about this if you actually want this behavior? Usually you have one autoincrementing primary key or no autoincrementing field at all...

[Mike Bayer]

maybe if it were version 0.2 but we're heading into 1.0 and i don't know that this change makes anyone's life better. there are definitely users that do something like an "autoincrement + a version counter" type of thing who would be surprised. But also, the way "autoincrement" is, it's a flag set that defaults to True. It makes the autoincrement thing happen any time it's valid, and in this case, it is valid. if we made it not do this, then we need to change autoincrement to a three-value field in order to make this possible, and then it's yet another major API change that might throw people off. it affects literally every integer primary key column out there and I don't know offhand what the impacts of that are going to be, given alembic, sqlalchemy-migrate, etc. we're at that phase where I am constantly amazed at how the most innocuous changes completely blow up openstack and other major projects that are using something in not quite the way you expected.

[Adrian]

True, probably not a good idea to change the API in such a way.

[Mike Bayer]

ill see if i can think of a way to make this less surprising.

[Mike Bayer]

I dont have an idea here right now, so pushing this out for the moment.

[honglei jiang]

I really hate the default behavior of current 'autoincrement' , like the following, I need write " autoincrement=False" time and time again in dozens of tables.

class PhoneNumberConfig(Base):                                   
    __tablename__ = 'foo'
    plan= Column(Integer, primary_key=True, autoincrement=False, nullable=False)
    terminal = Column(Integer, primary_key=True, autoincrement=False, nullable=False)
    port = Column(SmallInteger, primary_key=True, autoincrement=False, nullable=False)
    phone_number = Column(Integer, primary_key=True, autoincrement=False, nullable=False)

Why not just like the SQL language, declare autoincrement=True if user wants. For API compatibility, it may gave user a way to change such defaut behavior like some global config.

[Mike Bayer]

because it would be extremely backwards incompatible. global config is not an option because many individual parts of an application may be using SQLAlchemy separately. Also I'd not advise storing phone numbers as integers.

For any situation in Python where you don't want to type something, make yourself a def:

def Column(*arg, **kw):
    kw.setdefault('autoincrement', False)
    return sqlalchemy.Column(*arg, **kw)

[Mike Bayer]

good news everyone! I got bit by this stupid issue now. but bad news! I still have no idea how to solve it without breaking tons of environments.

[Mike Bayer]

• new approach to autoincrement, towards references #3216
• We add a new value of "autoincrement" called "auto". This is now the default for all columns, no longer True.
• the work of determining _autoincrement_column now moves to PrimaryKeyConstraint itself, as "autoincrement" is explictly a pk-only concept
• pkc detects autoincrement at the same time Table did, based on memoized_property. attempts to make this a more "active" setting are more complicated because during table construction with or without reflection the columns within the pkc are in flux and determiniations can't really be made without looking at all columns.
• with the difference between "auto" and True, we now can add validation rules when True is set: 1. only one column may have autoincrement=True 2. autoincrement=True requires an Integer type, error is raised otherwise 3. column default has to be None or a Sequence 4. column server_default has to be None or server-reflected
• when autoincrement=True is not set, we're looking for autoincrement='auto'. We only care about 'auto' when there is only one column in the PK. E.g. we no longer set any column to "autoincrement" implicitly if the key is composite, this requires "autoincrement=True". This is the core of
• we get rid of the awkward rule that we create "KEY idx_autoinc_foo" for a MySQL InnoDB column that is explicit autoincrement in a composite PK and not the first column. that was a terrible idea. when the non-first column in a composite PK is autoincrement=True, we render it first in the PRIMARY KEY constraint. Problem solved, no more implicit index, no more guessing if we're innodb (this guess was wrong in the case of innodb set on the server anyway). this is again a proposal for a big change in behavior; positives include that a surprise KEY is no longer created, won't show up in alembic autogen and generates a more efficient structure.
• databases where "autoincrement" is entirely implicit, e.g. SQLite, should raise an error now when autoincrement=True is set and can't be satisfied; SQLite dialect now raises this if autoincrement=True on a composite primary key since SQLite never does this (note that as always, this is distinct from SQLite's "sqlite_autoincrement" keyword which we support distinctly).

→ <<cset 8f81f07917a7>>

[Mike Bayer]

okey doke

[Mike Bayer]

• The system by which a :class:.Column considers itself to be an "auto increment" column has been changed, such that autoincrement is no longer implicitly enabled for a :class:.Table that has a composite primary key. In order to accommodate being able to enable autoincrement for a composite PK member column while at the same time maintaining SQLAlchemy's long standing behavior of enabling implicit autoincrement for a single integer primary key, a third state has been added to the :paramref:.Column.autoincrement parameter "auto", which is now the default. fixes #3216
• The MySQL dialect no longer generates an extra "KEY" directive when generating CREATE TABLE DDL for a table using InnoDB with a composite primary key with AUTO_INCREMENT on a column that isn't the first column; to overcome InnoDB's limitation here, the PRIMARY KEY constraint is now generated with the AUTO_INCREMENT column placed first in the list of columns.

→ <<cset 414af7b61291>>

[Mike Bayer]

• update the mssql autoincrement reflection test to accommodate the new behavior of the autoincrement flag as per ref #3216

→ <<cset 6713817e1186>>

[Mike Bayer]

users rejecting the CompileError. change to a warning.

[Mike Bayer]

Change autoincrement compileerror to a warning

Users are complaining that IntegrityError is no longer raised.

Change-Id: I0855d5b7a98d4338f0910501b6e6d404ba33634d Fixes: #3216

→ <<cset 8a13957db790>>