Drupal 6's standard is to no longer use manually created sequences and to instead rely on the database's built-in functionality.

function webform_component_insert($component) {
  db_lock_table('webform_component');
//...
  db_unlock_tables('webform_component');
  return $component['cid'];
}

I'll see if I can create a patch to remove this as currently this is the only module I have installed out of ~90 that has any locks.

Comments

quicksketch’s picture

It's important to know that component IDs are unique per-node. So a typical auto-increment won't work with the way Webform currently operates. I certainly wouldn't mind fixing this problem if you find a way around it.

damienmckenna’s picture

The heart of this is actually an urge to remove the LOCK statements. Given that the statement would fail if there happened to be a collision (unlikely to happen), would it be sufficient to just have better error checking?

quicksketch’s picture

If a collision was unlikely to occur in your situation, you could think of this as saying "a request waiting for the lock is unlikely to happen" also. I know the urge to remove locks might be high, but is there any particular problem caused by these locks? If you're in a situation where hundreds of components are being created a minute, Webform will likely run into many other scalability problems than this single table lock. I don't personally see a problem with the approach that would cause any site noticeable penalties.

mark trapp’s picture

Having just run into this problem, I'm looking for a solution as well. The main issue I see with this is that Drupal 6 core doesn't require Lock privileges to be granted to a database (http://drupal.org/getting-started/6/install/create-database), so no contributed modules should, either. That is, I shouldn't have to modify my database environment just to get a contributed module to work correctly. I don't want to step on any toes by modifying the issue, but this issue seems more like a bug report than a feature request.

damienmckenna’s picture

Amorfati: I think at the heart of this is that:

  • Right now it works just fine for 99.x% of users.
  • To change it would involve a chunk of work.
  • There's little incentive because of the above.

I don't think quicksketch would complain if someone were to submit a patch which resolved this, and I'm more than happy to test one, but it's far from a reasonable priority for anyone right now.

Personally speaking, the only reason I created the issue is because I had a DBA complain "there shouldn't be any manual locks", but Quicksketch's explanation works fine for us, especially given the scenarios we're using it in.

glennpratt’s picture

If this isn't going to change, then someone needs to make it clear in documentation and on the download page. You might also throw up an explanation as an error or to the Site Status report.

I'm hoping someone can patch this, because right now people who install Webform on a properly configured Drupal install will receive errors.

greggles’s picture

I think it's surprising more than a problem, and the error message is just annoying, not a real problem. Perhaps webform can detect whether the account has this permission and, if not, just skip the lock. That hides the error which eliminates the "problem."

Alternative solutions:
1. Create webform's own locking mechanism using a semaphore of some kind (blech)
2. Create an index on nid and component ID (if there isn't one) and leverage that to enforce the uniqueness
3. A hook_requirements that checks for the permission and, if not there, disables the module until the permission is granted which seems bad b/c for most sites the lack of this lock isn't actually a problem for the module to be able to function properly

quicksketch’s picture

Version: 6.x-2.6 »

Just a note that Drupal 7 has a built-in semaphore system which would suit this purpose. Considering the table locking isn't a problem for most sites, we'll probably leave it as-is in Drupal 6 and implement it in the 3.x version for D7. In the mean time I've update the project page to reflect the requirement.

markDrupal’s picture

The lock seems to prevent 2 users working on the same form from submitting a new component at the same time.

Views has some sort of locking similar to what you need here. I don't believe Views allows 2 sessions to work on the same view at the same time.

quicksketch’s picture

The lock seems to prevent 2 users working on the same form from submitting a new component at the same time.

The lock is what makes 2 users working on the same form possible. There is just a slight delay before a user's request is processed. So if 2 users clicked "Save" at the exact same time, one user's request would be processed first, while the other has to wait 5ms more until the lock is released. The lock actually exists specifically to make it so that this situation does not cause a problem, otherwise users could end up overwriting each other's data.

crutch’s picture

Attempting to add a recipient to the form is causing this WSOC but shows error on screen, d6.15 >> Warning: Table 'watchdog' was not locked with LOCK TABLES query: INSERT INTO watchdog (uid, type, message, variables, severity, link, location, referer, hostname, timestamp) VALUES (1, 'php', '%message in %file on line %line.', 'a:4:{s:6:\"%error\";s:12:\"user warning\";s:8:\"%message\";s:334:\"Unknown column 'excluded_components' in 'field list'\nquery: INSERT INTO webform_emails (nid, eid, email, subject, from_name, from_address, template, excluded_components) VALUES (8, 1, 'me@mysite.com', 'default', 'default', 'default', 'default', '')\";s:5:\"%file\";s:99:\"C:\\TWAMPd_2010\\htdocs\\sites\\all\\modules\\webform\\includes\\webform.emails.inc\";s:5:\"%line\";i:465;}', 3, '', 'http://localhost/node/ in C:\TWAMPd_2010\htdocs\includes\database.mysql.inc on line 128

--------

Then to go back and visit the form this error is shown, but the theme comes back >> user warning: Unknown column 'excluded_components' in 'field list' query: INSERT INTO webform_emails (nid, eid, email, subject, from_name, from_address, template, excluded_components) VALUES (8, 1, 'me@mysite.com', 'default', 'default', 'default', 'default', '') in C:\TWAMPd_2010\htdocs\sites\all\modules\webform\includes\webform.emails.inc on line 465.

crutch’s picture

Version: » 6.x-3.0-beta1
quicksketch’s picture

The last warning is probably the one you need to be careful about. Did you run update.php since you last updated? It sounds like excluded_components is not a column in your webform_emails database table. I think that is the only issue here and it's not related to the table locking.

crutch’s picture

Thanks QS. I was just starting to build a site and decided to use 3.

I did an initial install with all my standard modules I had locally including 2.9. I then updated a few one at a time doing update.php after each one. Then I saw webform 3 was available so I gave it a shot by downloading, and doing update.php. Everything seemed to work great and I like the new layout.

I was able to create the form and move around well but when going to add the recipient I begin encountering the error loop where I can never add the first recipient email address. I since started over with 2.9.

What I'll try to do is once I get the site build with Webform 2.9, I'll create a copy of the site and then try and upgrade to 3.

quicksketch’s picture

Status: Active » Closed (won't fix)

Moving this to won't fix. Anyone interested in providing a patch may do so, but it doesn't seem like anyone thinks this is a high enough priority to fix since it's almost entirely a philosophical (rather than practical) issue about avoiding table locks.

dstol’s picture

Subscribe

ben.bunk’s picture

subscribe

dstol’s picture

Status: Closed (won't fix) » Needs review
StatusFileSize
new1.99 KB
new2.18 KB

@quicksketch, it seems that the philosophical has become reality in my case.

Patches against 6.x-2.x and 6.x-3.x.

dstol’s picture

Version: 6.x-3.0-beta1 » 6.x-3.x-dev
Category: feature » bug

Really more of a bug report as we're getting time outs while editing webform components.

quicksketch’s picture

Thanks dstol, this looks great! The lock_* functions only became available in some middle version of Drupal 6 (like 6.12 or something), so we'll need to proceed with documentation about this requirement. Great patch though, I'll take a look next time I'm reviewing patches.

quicksketch’s picture

Status: Needs review » Fixed
StatusFileSize
new1.52 KB
new3.34 KB

Ah, feels nice to finally fix this issue. I've committed these two patches to the 3.x branches. No new releases of 2.x will be made.

There was a bit of a bug in the 3.x patches with this typo: lock_acquire('webofmr_component_insert'), but while fixing it I decided to make these locks per-node anyway. So now users have to be editing the exact same node to run into this problem as opposed to having two completely separate nodes locking each other.

In Drupal 6 I also updated the README.txt and added a hook_requirements() to make sure users have Drupal 6.16, in which the locking framework was added. For reference, see #251792: Implement a locking framework for long operations.

quicksketch’s picture

Title: webform_components.inc shouldn't lock the table » Webform shouldn't lock the "webform_components" table on insert, use locking framework instead

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.