The function path_set_alias() might cause database errors in high traffic environments.

It uses the results of two select statements to distinguish between inserting or updating a row. But the data might be changed by a concurent process between the selection and the manipulation of the data.

I attached a patch to solve this issue by setting a database lock.

CommentFileSizeAuthor
#5 path.module.patch1.44 KBmkalkbrenner
path.module.patch1.51 KBmkalkbrenner

Comments

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

This patch doesn't appear to be correct. You say you set a database lock yet the patch is removing database locks and unlocks. However, I don't see locks as the answer since MyISAM locks the entire table and not a row. There is a push for D7 to support InnoDB engine instead so that transactional row locking can take place. But, if two users are updating the same url_alias then the one who updates last will be the winner regardless of the lock.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented

Yes, after the patch the complete table url_alias will be locked in line 142 and the lock will be released after all operations on this table.

The database error I described occurs if two threads try to set the same new alias at the same time.
In both threads $alias_count equals '0'. So both threads try to INSERT a row. Regarding the primary key 'pid' there's no problem because it's an auto_increment but the UNIQUE KEY constraint on column 'dst' causes the second INSERT to fail with a database error.

BTW from my point of view using an auto_increment for 'pid' violates drupal's architecture to use a sequence table for ids.

Anonymous’s picture

Anonymous (not verified) commented
Priority: Normal » Minor

The patch was still created in reverse. I.E. the new was specified as the old and vice versa.

--- path.module	(revision 17567)
+++ path.module	(revision 14634)

The sequence table is gone in D6. Locking entire tables as MyISAM would do for your patch isn't a good thing for this issue. Doing a ON DUPLICATE KEY UPDATE is better but maybe won't be a good fit for this.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented

You're right. I created the patch in reverse direction. I'll attach a corrected one soon.

ON DUPLICATE KEY UPDATE was introduced in MySQL 4.1. According to http://drupal.org/requirements drupal 5 supports MySQL since 3.23.17.
Another Option is REPLACE INTO but I think this one isn't portable to other databases.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
StatusFileSize
new path.module.patch1.44 KB

Here's the right patch, sorry.

dpearcefl’s picture

dpearcefl commented
Status: Needs work » Closed (won't fix)

Considering the time elapsed between now and the last comment plus the fact that D5 is no longer supported, I am closing this ticket.

dpearcefl’s picture

dpearcefl commented
Status: Closed (won't fix) » Postponed

Reopening this issue until I can check to see if this is useable in newer Drupal.

codi’s picture

codi commented
Status: Postponed » Closed (won't fix)

old issue and drupal 5 is no longer supported.