The old issue had a way too high s/n ratio. However, the patch was tested and since then only node creation has changed and I checked that. So it's ready. Note that currval of pgsql is per connection so that's not a problem.

CommentFileSizeAuthor
#10 hey_dude_wheres_my_id.patch2.59 KBchx
#3 db_last_insert_id_13.patch30.93 KBchx
db_last_insert_id_12.patch26.92 KBchx

Comments

chx’s picture

chx commented

Dries asked to link to the old issue. I am not doing that as it's a waste of time, instead I am listing the raised concerns and the solutions.

  1. Is this faster? For MySQL we run less queries, for pgsql we run the same amount. Also, we let the database engine to solve a problem belonging there which should also be faster than moving the problem to the app layer.
  2. What about InnoDB locking the table for the duration of the transaction containing an autoincrement using INSERT? This would be more of a serious problem if we would have extended INSERTs or long running transactions. Also, Drupal has used autoincrement for tables like watchdog for very long and those are the ones that are frequently written. The tables affected here are rarely written so we are not adding new problems here -- if this proves to be a problem a future patch can deal with it.
  3. Is this thread safe? Yes, it is both for pg and mysql engines.
  4. Why db_last_insert_id? Because most databases lets you read ahead of the time from a sequence -- MySQL lacks this capability but still. Also, to make it more familiar for people who use dMySQL before.
dries’s picture

dries commented

The fact that this issue was forked makes me reluctant to commit this patch. I still have to read up on the old issue -- regardless of the summary.

chx’s picture

chx commented
StatusFileSize
new db_last_insert_id_13.patch30.93 KB

Keeping up with HEAD and Dries asked to remove the already obsolete pconnect comment.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Not sure who to credit -- credit is in the other issue. Thanks all! :)

dww’s picture

dww
we/he/they
commented
Status: Fixed » Needs work

upgrade docs: db_next_id() is now gone, among other developer-visible changes of this patch.
http://drupal.org/node/114774/edit

jvandyk’s picture

jvandyk commented

Uh. The sequences table is not used only for incrementing database keys. It is also a persistent counter API in Drupal. Now that it's gone, what, people should bloat the variables table with counter variables and implement their own counting?

jvandyk’s picture

jvandyk commented

Having a persistent counter in Drupal is useful. Maybe I want to keep track of the number of times something happens. I can do that by calling db_next_id().

Just because Drupal's emulation of database autoincrementation used db_next_id() does not mean that db_next_id() should be taken out. It simply means that autoincrementation should be done another way.

It's like autoincrementation of database rows is a hamburger. db_next_id() is mustard. So I walk into the cafe and find the mustard gone. When I ask why they no longer have mustard I am told that they are not serving hamburgers anymore. But I was putting mustard on my fries, not on a hamburger.

If db_next_id() is no longer available, and I want to count something and have that count stick, I have the following options. (1) I can do a variable_set(). Great. But now that counter is getting pulled needlessly into memory on every request, even if I only need it sporadically. (2) I can have my own database table. This seems kind of silly. Why reimplement something as simple as a counter table when for years we've already had one in Drupal's core API -- the sequences table.

So what I would like to see is either (a) (preferred) bring back the sequences table and db_next_id(). Just don't use it to emulate autoincrementation of database rows -- use native database autoincrementation for that. (b) get back persistent counter functionality by having variables that are not loaded on every request (145164?) and a drupal_counter($counter_name) function in common.inc.

moshe weitzman’s picture

moshe weitzman commented

a) makes sense to me.

even worse than variables being called into memory on every load, a variable_set() is very expensive and can kill you if your burgers are very popular.

chx’s picture

chx commented
Status: Needs work » Needs review
chx’s picture

chx commented
StatusFileSize
new hey_dude_wheres_my_id.patch2.59 KB

So, what do you guys think of this?

chx’s picture

chx commented
Status: Needs review » Fixed

Following up on this, db_next_id was never a general counter API -- on postgresql where sequences are used, you need to create them either by creating a serial field in a table or explicitly creating the sequence. So, if we want a general counter API, that's totally independent of this issue -- please open another.

dww’s picture

dww
we/he/they
commented
Status: Fixed » Needs work

upgrade docs still need fixing. see #5 above.

hswong3i’s picture

hswong3i commented
Status: Needs work » Fixed

document updated: http://drupal.org/node/114774#db_last_insert_id

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)