There have been build a database incompatibility inside node_style. Please read http://api.drupal.org/api/4.7/function/db_next_id for the correct API way... for fixing this the DB table must be altered, function db_next_id used and the auto increment deactivated for node_style_schemes.sid field. And finaly the install file need to be extended for a one time transition...

The bug is inside the following lines where the db_next_id is missing:

function _node_style_add_scheme($name, $scheme) {
  db_query("INSERT INTO {node_style_schemes} (name, variables) VALUES ('%s', '%s')", $name, serialize($scheme));
}

Comments

Zen’s picture

Zen commented
Status: Active » Closed (works as designed)

This is by design. Please search the core issue queue for "auto_increment" to bring up all the discussions on this matter. If you also check the Drupal core DB, you will find that some tables do use auto_increment.

Furthermore, pgSQL's "SERIAL" attribute does the same thing. There is no incompatibility.

Thanks :)
-K

hass’s picture

hass commented
Status: Closed (works as designed) » Active

For compatibility reasons, Drupal does not use auto-numbered fields in its database tables. Instead, this function is used to return a new unique ID of the type requested. If necessary, a new sequence with the given name will be created.

i wonder... maybe there are some tables from priior years not changed yet, but i think you should follow the API way... i don't know how old this autoincrement fields are and when the sequences table has been introduced. all i saw until now is the sequences table is used by all modules and core that need to increment a table row. if we have APIs for such a task we should use them, isn't it? changing this behaiviour is not difficult and clean up possible db incompatibilities. Other DBs will come and then this change is required - don't think only with mysql and pqsql - oracle and mysql support is one the way. So following the API way is the clean and supported way.

if i search for this auto_increment string i can only find cases marked as "bugs" that will be fixed... should i really read all of them?

hass’s picture

hass commented

Drupal core gurus discussion... "remove all auto-increment fields" http://drupal.org/node/3684

Additional, there is another problem, you possibly not aware about. If you have 10 webservers and PHP using concurrent connections and you are using autoincrement, you are totaly lost. You possibly don't get the correct number with SELECT @@identity after an INSERT. this is why this sequences table is a very big benefit and the only clean solution. think big :-).

if you really need a more detailed example what can happen in MSSQL i can try to explain.

Zen’s picture

Zen commented
Status: Active » Closed (works as designed)

The key word here is "performance" and the key word with relation to performance is "locking" not to mention consistency. Please feel free to read all you want and consequently, try to fix core before this module. This discussion has been beaten to death many times and I'm not going to be drawn into another debate.

Concurrent connections: From the POV of MySQL, this is incorrect and also inapplicable in our case. The auto_increment operation in MySQL is connection specific. I believe that this is also the case for pgSQL's SERIAL. Whether either of them actually withstand 100s of concurrent connections is another matter altogether.

Cheers,
-K

hass’s picture

hass commented
Title: database incompatbility » database inconsistencies will occour
Status: Closed (works as designed) » Active

You get inconsistencies - for sure. if you do two concurrent

INSERT INTO blah (test) VALUES ('test')
SELECT @@identity

on two different connections you possibly get for e.g. a number "5" on connection A from @@IDENTITY. The problem is - if another connection B is open to database and doing the an insert you possibly get the wrong "@@IDENTITY" 6 on connection A - where data has been inserted into 5 and a no 5 on connection B where the data has been inserted into recordset 6!!! so you get data inconsistencies with your way.

So if you use the returning value from @@identity to insert data into another table with a reference to the priior inserted record, the reference is wrong! This have nothing to do with performance discussions. however i'm sure someone missed to explain this possibilty. now you are aware of this, and you should fix this bug.

If not - you should only mark this "won't fix" and we document this as BAD and WRONG DESIGN from API side and not "by design". I cannot follow your arguments if you say i should fix core first. something should be fixed if wrong and not set by design, while some other people done mistakes in past and haven't fixed their bugs yet, but going to fix them in future.

Zen’s picture

Zen commented
Status: Active » Closed (works as designed)

a) Whether you are right or wrong, it does not matter in the case of this module and this table in particular.
b) MySQL is not MSSQL. There is no global @@identity value.
c) Please read the documentation for mysql_insert_id [ http://in2.php.net/mysql_insert_id ] and equivalents. As mentioned previously, auto_increments are connection specific. You insert a record, and use the mysql_insert_id value for subsequent operations on other tables for that connection session. This will always be the last inserted ID on _that_ table in _that_ connection. There is no race condition here.
d) Please google for something like 'auto_increment race condition' for some light reading.
e) Core will, IMHO, very likely start using auto_increments as soon as somebody gets a little motivation to do so.

Please feel free to take this up further on the development mailing list, not in this issue. It does no actually make any difference to this module.

Thanks,
-K

Zen’s picture

Zen commented

er, correction. mysql_insert_id is not _table_ specific.. just connection specific. But that's pretty much all that's required.

-K