I think there's a bug in db_last_insert_id()

By valk on 8 Jun 2010 at 10:51 UTC

UPDATE: the issue has reappeared, so I opened a bug here.

Hello good people,

Can anyone confirm this 6.x core bug?

This code returns a wrong ID.

db_query("INSERT INTO {boxes} (body, info, format) VALUES ('%s', '%s', %d)", "Some body here...", "A title is here", 1);
  $delta = db_last_insert_id('boxes', 'bid');

Now delta should have the maximud id of the {boxes} table, right?

Well this is wrong.

Currently the problematic function looks like that:

function db_last_insert_id($table, $field) {
  return db_result(db_query('SELECT LAST_INSERT_ID()'));
}

As you can see there's no use of the two input parameters.

I had to rewrite the function as follows.

function db_last_insert_id($table, $field) {
	$result = db_query("SELECT $field FROM {$table} ORDER BY $field DESC LIMIT 1");
	$row = db_fetch_object($result);
//	print($row->$field);
	return $row->$field;
}

Now this works.

Please correct me if I did some mistake.

Thanks

Comments

valk commented 8 June 2010 at 10:57

.

cog.rusty commented 8 June 2010 at 11:49

That is not what I see in the source of http://api.drupal.org/api/function/db_last_insert_id/6

--- Edit:
Oops, there is a comment which says that the parameters have no effect in MySQL. I'll take a look for any issues where this is discussed.

--- Edit2:
There are issue discussions like this one
http://drupal.org/node/369520
Which, if I am reading it correctly, means that the function is useless.

Your solution of running another query does not seem thread-safe, if other people are using the database at the same time, but I guess not many would do inserts in the "boxes" table.

Why useless? It does return

Heine commented 8 June 2010 at 12:02

Why useless? It does return the last inserted id for autoincrements.

.

cog.rusty commented 8 June 2010 at 13:37

I meant useless for what valk wanted to do, that is, to get the last ID of that specific table and not of watchdog or any other table.

------ Edit:

@valk, what value did you get and what were you expecting? And what MySQL version are you using?

I wonder why the 11 drupal functions which make calls to db_last_insert_id(), including user_save(), comment_save(), and drupal_write_record(), don't have a problem.

I don't see anything that could trigger another insert in your thread, between db_query() and db_last_insert_id(). Do you?

@cog.rusty, MySQL version is

valk commented 8 June 2010 at 14:51

@cog.rusty,

MySQL version is 5.1.41

I had this problem while debugging my app, and also when executing from drupalforfirebug. I cannot reproduce this again....

Still it's kind of risky, I'm not sure whether we should use the core code..

Maybe this is somehow related to the id field definition in MySQL?

.

cog.rusty commented 8 June 2010 at 15:03

Do you remember at least if the value you got was "almost right" or "way off"? That might be a clue for where it came from.

@cog.rusty, "Way Off". The

valk commented 8 June 2010 at 20:13

@cog.rusty,

"Way Off".
The value returned was 4-digits number - something like 4018. And then on every subsequent execution it returned the number incremented by 1, i.e. 4019, 4020. The expected value was 7, (it's a clean website, so there's not many blocks).

I've used it in modules

Jaypan commented 8 June 2010 at 14:56

I've used it in modules before, and I use mysql. I've never noticed any troubles with it so far, but I've also never looked at it closely - I just assumed it did what it's supposed to, and it's always seemed to do that.

Use drupal_write_record

da_solver commented 8 June 2010 at 13:12

Use drupal_write_record http://api.drupal.org/api/function/drupal_write_record/6 . You pass the second parameter by reference. Upon a successful insert operation drupal_write_record updates the referenced object and populates it with the database generated serial field.

db_last_insert_id() is specific to the Postgres database engine. It's not a bug, it's just not applicable to mysql.

Hope that helps.

@da_solver, Thank you! :)

valk commented 8 June 2010 at 14:57

@da_solver,

Thank you! :)

Just a note - the same db_last_insert_id() is used in Add Block's _submit hook.

db_last_insert_id() is

Heine commented 8 June 2010 at 14:58

db_last_insert_id() is specific to the Postgres database engine. It's not a bug, it's just not applicable to mysql.

That's not true. See database.mysql-common.inc. It will ignore table and field params, which are required for postgresql compatibility.

Have you filed a bug?

goofus commented 8 June 2010 at 16:12

The documentation http://api.drupal.org/api/function/db_last_insert_id/6 lists the source code listing as
"includes/database.pgsql.inc, line 230". Did you file a bug yet?

.

cog.rusty commented 8 June 2010 at 16:18

@goofus, the same is true for all database functions, including db_query(). Finding the mysql implementations requires some digging.

Thanks

goofus commented 8 June 2010 at 17:27

I just meant since the mysql last_insert_id() is a per database connection instance value, it really could never be used logically the same way a postgres sequence generator is. The postgres sequence generator acts more like a read only table and has nothing to do with database connection instances. To me, those 2 implementations are really different (logically).

So even if you used the parameters and implied fields in the Drupal implementation

function db_last_insert_id($table, $field) {
  return db_result(db_query('SELECT LAST_INSERT_ID()'));
}

It simply would never return the expected results. Again, in mysql this is a function that returns a value to a specific database connection instance.

I'm new, not sure where to file the bug.

.

cog.rusty commented 8 June 2010 at 21:25

That is what we have been discussing. Bug reports have already been filed, for example

http://drupal.org/node/369520

but amazingly the MySQL implementation always returns the right ID without using the arguments in 11 important core functions, which call it too often for a bug to go unnoticed.

The only case in that bug report where it was demonstrated to return a result from the wrong table (watchdog) was a case where it was called later in a different function, where other INSERTs could have happened in the meantime.

So far, the bug that has been shown is consistency of usage: The documentation should say something like "In MySQL, this function can only be used after an INSERT query and before any other code is given a change to run any INSERT queries, withing your client connection."