579096 - add an autoinc for collections insertions

Status: RESOLVED WONTFIX

(Cloud Services Graveyard :: Server: Sync, defect)

Description

[Tarek Ziadé (:tarek)]

To avoid the select max() done before an insertion, let's use an autoinc in collections.  MySQL will increment a distinct counter per userid as long as collectionid is an autoincrement and part of the composed key.

CREATE TABLE collections (
    userid INT NOT NULL,
    collectionid INT NOT NULL AUTO_INCREMENT,
    name CHAR(30) NOT NULL,
    PRIMARY KEY (userid,collectionid)
) ENGINE=MyISAM;

This alter could be done in the current version, and the PHP server changed consequently (remove the select(max) done before the insert).

Comment 1

[Toby Elliott [:telliott]]

This seems fine, but be aware that I believe it makes MySQL5 a requirement for external installations, since I don't think this was a supported feature in 4.

What would probably be better actually would be to have a mysql_mozilla storage type and a mysql storage type that doesn't bother with the extra collections table. Most people don't need this scaling optimization.

Comment 2

[Mike Connor [:mconnor]]

MySQL 5 is what, five years old now?  I don't think that's an unfair baseline, for people who aren't running the minimal server.

Comment 3

[Mike Connor [:mconnor]]

Tarek, can you fix this?  I think MySQL 5 is a perfectly cromulent min req.

Comment 4

[Toby Elliott [:telliott]]

The barrier here isn't the fix, which is pretty easy - it's that it's not backwards-compatible with the various installs already out there. We should defer it to the 1.5 push.

Comment 5

[Tarek Ziadé (:tarek)]

Changing the milestone accordingly -- The patch follows

Comment 6

[Tarek Ziadé (:tarek)]

The patch is written, but it turns out this feature only works for MyISAM. InnoDB does not have this. What engine does our production uses for this table ? Since inserting new collections is quite a rare event, MyIsam should be fine.

In any case, until we get different client-side behaviors, all reads occur in the hardcoded array with the current clients, so it doesn't make a big difference right now. This change is more future-proof though, in case we have more arbitrary collection creations.

Now about backward-compat for the masses: I propose that the next version includes in the Database schema a version number, so we're able to provide DB upgrade scripts like what we'll have in Python. The next version of the minimal and full PHP servers can provide a first upgrade script.

This will also make it simpler for people to move from one of the PHP servers to the Python server without losing their data: the python server will recognize the DB version and upgrade it if needed.

Comment 7

[Tarek Ziadé (:tarek)]

Attachment: Patch for auto_increment in the collections table

This makes the code uses MyIsam multiple keys autoincrement feature. No DB upgrade script yet, waiting for Toby/Mike feedback before diving into this

Comment 8

[Toby Elliott [:telliott]]

Comment on attachment 475477 [details] [diff] [review]
Patch for auto_increment in the collections table

This will work, but it does have all the dangers you've already noted. I would suggest that we hold it for the python version, since that provides a nice break where the user is going to have to do a bunch of stuff anyway, and this isn't an area where the inefficiency is causing us problems right now.

Comment 9

[Tarek Ziadé (:tarek)]

This is a minor improvement as adding per-user collections is not done by the clients. I am marking it as won't fix