People/rporres/sqlite autoincrement

[rporres]

As stated in the SQLite docs, the behaviour of autoincrement changes when the column is a primary key.

http://www.sqlite.org/autoinc.html

SQLite generator doesn't do anything with autoincrement in primary keys.

Trying to generate a table with this code:

   my $table = SQL::Translator::Schema::Table->new(
       name => 'foo_auto_increment',
   );
   $table->add_field(
       name => 'id',
       data_type => 'integer',
       is_nullable => 0,
       is_auto_increment => 1,
   );
   $table->primary_key('id');

   my $result =  SQL::Translator::Producer::SQLite::create_table($table, { no_comments => 1 });

would create an "id" row such as

"id" INTEGER PRIMARY KEY NOT NULL

instead of

"id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL

as expected.

Heres a summary of the changes included:

• I have patched SQL::Translator::Generator::DDL::SQLite to add the word AUTOINCREMENT to the primary keys column is detected as primary key.
• I've also patched 30sqlt-new-diff-sqlite.t and 48xml-to-sqlite.t as the new Generator broke some of the tests
• I've added a couple of tests in 56-sqlite-producer.t to prove the new feature.
• I've added my name and mail in the AUTHORS file as suggested by a Peter Rabbitson message in sqlfairy-developers mailing list

Please do take into account that I'm very new to SQL::Translator and I don't know if the changes and the implementation of them are in sync with the common practices of SQL::Translation.

Regards,

Rafa

PS. BTW, thanks for SQL::Translator. It's been VERY helpful ;-)

[frioux]

This is a little scary of a change, but it won't break anything and the cause is moderately better correctness (more unique PK's in the face of deleted rows.)

[frioux]

(merged)

[ribasushi]

It does break a lot of the DBIC test suite, investigating what is the exact fallout...

[ribasushi]

In essence the change is "more correct", but even the cited docs discourage use of the AUTOINCREMENT keyword. I am not sure what the correct way forward is here at this point...

In any case this is a grossly incompatible change, for which I believe we needed an at least couple month warning cycle, and a downstream CPAN smoke round. Now none of this is an option anymore. Ideas?

[frioux]

I couldn't see what it would actually, seriously break other than different
DDL. If it causes real problems let's just revert it, rerelease, and add
deprecation cycle.

Also, fwiw I agree re not having two issue trackers. I like github but
having two trackers just blows.
On Oct 31, 2013 4:52 AM, "Peter Rabbitson" notifications@github.com wrote:

In essence the change is "more correct", but even the cited docs *
discourage* use of the AUTOINCREMENT keyword. I am not sure what the
correct way forward is here at this point...

In any case this is a grossly incompatible change, for which I _believe_we needed an at least couple month warning cycle, and a downstream CPAN
smoke round. Now none of this is an option anymore. Ideas?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/26#issuecomment-27473286
.

[ribasushi]

Ouch, sorry for the reference spam, never realized github will do that gah. Noting more stuff "to never do" :(

[ribasushi]

@frioux On the incompat - basically this changes all autoinc columns in new deployments from "guaranteed unique but possibly decreasing" to "guaranteed monotonically increasing including inability to wrap around".

Additionally the SQLite docs state in http://www.sqlite.org/autoinc.html :

These are important properties in certain applications. But if your application does not need these properties, you should probably stay with the default behavior since the use of AUTOINCREMENT requires additional work to be done as each row is inserted and thus causes INSERTs to run a little slower.

So I don't know what is a sane way forward... In any case if we go with the change it has ramifications to any test suite that deletes rows, then inserts more, and expects a particular PK value.

[rporres]

Hi,

Sorry for not having commented anything before. I've just seen the discussion and the commit reversion. The way it is now leaves people like me with problems when we want to check that an autoincrement primary key always grows, as we see it in MySQL with InnoDB and MyISAM engines. Being no expert at all in Postgres, I've installed locally and seems to behave as MySQL using SERIAL datatype instead of autoincrements.

The problem comes from SQLite autoincrementing even when it is not told so. If you create a table in SQLite without an autoincrement in the primary key as in the following example

CREATE TABLE test (
  id integer NOT NULL,
  a integer NOT NULL,
  b integer NOT NULL,
  PRIMARY KEY  (id)
);

it will automagically autoincrement values for id when you insert

sqlite> insert into test (a, b) values (1,2);
sqlite> insert into test (a, b) values (1,2);
sqlite> select * from test;
1|1|2
2|1|2

which is something quite particular to SQLite, as you won't see that behaviour in other databases such as MySQL or Postgres. The documentation warns that it will perform a bit slower and that you should only use it when needed.

This problem came as I was expecting increasing values of autoincrement primary keys in SQLite, as I run my test suite against SQLite when developing but it can also work against MySQL with an env variable, which is quite convenient for me.

There should be a way to tell SQL-Translator to behave as other databases do with autoincrement primary keys even if we want the default behaviour to remain as it is now (ignoring is_auto_increment as SQLite will do it for you even if you didn't ask for it). Maybe we could do it with another property like sqlite_auto_increment that would only make sense for SQLite as it would be ignored by other producers as far as I know.

If this approach makes sense to you, I can try to implement it and submit a pull request.

[frioux]

Two things:

• We understand the value of this change, it just changes the way SQLT generated schemata work in a slightly backwards incompatible way
• Your change still would not have fixed the case you mention, because SQLite will autoincrement PK integers whether you ask it to or not. So while your fix is good and makes SQLite act more like other DB's when you ask for an autoinc, it does not change SQLite when you do not ask for autoinc.

[rporres]

Hi,

I'm afraid I don't understand how my change would not have fixed the case I mentioned. What I needed is SQLite to behave as other DB's with autoinc PKs and that is exactly what I achieved. Anyway, you're quite right that any change in SQLite generator should give a proper general solution that includes the case that you don't want autoinc with integers and my pull request didn't take that into account, so I understand it cannot be merged as it is now.

SQLite documentation suggests here that the only thing that you have to do is to set the data_type to INT rather than INTEGER. I've tested it and it seems to work. I've created the following table:

CREATE TABLE test_no_auto_int (
  id INT PRIMARY KEY NOT NULL,
  a TEXT
);

and it won't accept null values for id

sqlite> insert into test_no_auto_int (a) values (1);
SQL error: test_no_auto_int.id may not be NULL

as it would accept it if we had created with INTEGER

CREATE TABLE test_no_auto_integer (
  id INTEGER PRIMARY KEY NOT NULL,
  a TEXT
);

sqlite> insert into test_no_auto_integer (a) values (1);
sqlite> select * from test_no_auto_integer;
1|1

We cannot create autoinc PKs with an INT datatype as in the following example

CREATE TABLE test_int_auto (
  id INT PRIMARY KEY AUTOINCREMENT NOT NULL,
  a TEXT
);

as SQLite would complain:

SQL error near line 1: AUTOINCREMENT is only allowed on an INTEGER PRIMARY KEY

I would say that my pull request should also contain modifications on SQL::Translator::Generator::DDL::SQLite::_ipk subroutine to be correct, as INT and INTEGER are not equally considered by SQLite when creating tables.

I could take a look to that, amend the pull request and reopen it so that you can check if you're more comfortable with the solution. Or if you want to take another route and I can be of any help, please let me know.

Regards

[ribasushi]

@rporres Just chiming in briefly to restate the problem, as doesn't seem to have been voiced in this discussion:

• By default SQLite provides autoincrement via an alias to ROWNUM - this is performant, guarantees uniqueness, and does NOT guarantee monotonic increase
• Your change makes all sqlite autoincs proper ANSI-sql autoincs - slower, unique and monotonically increasing

The problem: there is likely a vast amount of software both on CPAN and on Darkpan which has tests and business logic assumptions based on the ROWNUM-aliased semantics. Hence this is a very disruptive change that had to be quickly backed out.

The possible solutions all circle around these criteria:

• A silent change of behavior like this is not an option. Folks need to have at least a year of clear warning that the behavior is going to be changing
• The behavior needs to be switchable - folks should be able to select the less correct but more performant behavior (it is even recommended by the SQLite documentation). I suggest some sort of general SQLite-producer specific producer_flag. Something like autoinc_method => [ rownum | sequence ], with the default being 'rownum' and being changed in a year to 'sequence'
• The breakage (behavior change) should be introduced a bit sooner on AUTOMATED_TESTING=1 so that cpantesters can highlight the downstream fallout

I think this should make for a smooth transition towards an "everybody wins" scenario.

Cheers

[ribasushi]

Correction - s/rownum/rowid/g. I've had too much Oracle I suppose.

[rporres]

Hi,

I definitely agree on the criteria you've written. I also proposed something similar in a previous comment although less elaborated :)

I also wonder if we should take into account the way SQLite handles INT and INTEGER datatypes, as we may also need to produce schemas with integers in a PK that are not autoincremented. This change could also be a source of incompatibilities with existing software.

Cheers

[ribasushi]

I suspect this is a good idea, but I do not have enough experience with the INT/INTEGER distinction to be sure of which way it should be handled. I am aware of the SQLite documented AUTOINC heuristic, and while it is unambiguous it is exceedingly contrived, so hard to wrap head around ;)

Anyhow - if we agree on a way forward I suppose the next step is for you to resubmit the patch with the discussed changes, and we will go from there.

[rporres]

That's fine by me