_block_rehash() deletes all blocks

I think "in_database" shoudl be renamed, it is pretty confusing.

Personally, I like the old approach better. I'm not a fan of the extra 'commands' that you tag onto the object. The proper solution would be to use transactions.

Ok, this won't work, it won't delete blocks from modules that have been deleted in the meantime.

opps

Opps,

Here it is again.

I found a bug, will upload a new patch

Patch doesn't apply to HEAD.

Mmm, I don't see how this patch fixes the problem. There would still be a race conditation, just a smaller window (less risky to loose data)?

I've reviewed the block module code and didn't find a problem that would occur if you had two rows for the same block. All the SELECT queries that matter have a DISTINCT and the Updates will update both copies, should they exist. Hell, i even duplicated all my entries and don't get double blocks.

Adding a unique key on (theme, module, delta) fails since keys are restricted to 1000 bytes.

This is a definately a good direction to be moving in. The old system isn't good. What can be done to move this along? Put more locks in?

bump

I was leaving this one for Dries to do final review.

Ok, let's rephrase this: There is a race condition (as on nearly every Drupal page, if you save it) but it doesn't matter. :P

No, we don't _fix_ it with a table lock, we only make it inconsequential, nothing bad will happen if there is actually a race. The same is true for Gordon's code.

First of all: the real problem is that the current code deletes user data if they are unlucky.

Second, the table lock doesn't really fix the problem of two people editing the form at the same time. It just makes sure there is not db error by making sure the two POST events are put into the database in sequence. For the first user this still overwrites his data. Of course, this is true for all admin forms.

21:04 <Natrak> killes: if you can use REPLACE for blocks you can avoid the locks i guess.. but imo it makes more sense to deal with them individually. Make the blocks patch use locks commit it, then if the no locks patch goes in re-work it to use db_query_replace (which seems a bit weird to me, but we will see)

I like the approach of this and is another improvement for _block_rehash(). I didn't like the massive DELETE. Haven't tested the patch, but the approach is a +1. So is the next step to throw locks around a portion of the code or should this be set back to review?

Not sure if I understand the race condition completely, but here's a patch. This what you want?

This is a relatively large refactoring. I'm going to say 6.0, but feel free to debate if yo have good arguments.

Subscribing, and I think it it should be tagged as critical.

I noticed this condition a little while back, but somehow it got lost in my brain to upload a patch. This is a small change to fix the race condition. I re-rolled removing two trailing whitespace chars. +1

Umm, you probably meant "Neil". But yes, I think this should be in D4.7 too.

Oh, and while I agree that this fixes most of the problem, I still think that the "delete and then re-insert" part is crappy code. :p

Rolling back 5.

"When you use LOCK TABLES, you must lock all tables that you are going to use in your statements."

So if we go the lock-moving-route, we want to:
1. Get the big list of blocks from module_invoke
2. Lock it
3. Do that first SELECT on blocks
4. Iterate through it and do whatever that big loop does.
5. That delete and insert thing.
6. Unlock.

Added an initial db_error check to the rename table functions for additional safety.

This looks really good. This will remove an race conditions.

Added prefixing to src.

Ok. Major bug with the latest patch, which otherwise looks like an elegant solution to this and probably other race conditions in core.

@ admin/build/block, click on one of the other themes that's not the current one, click back to the current theme.

All your blocks are gone. ;)

Would help if I would not every single issue I work on, here.

I don't think messing with the DDL to update block data is a good idea. This patch may be better than what we have, but neither option is "safe."

I agree that this patch doesn't solve the underlying problem. This patch only reduces the window for problems, it doesn't fix it.

This patch doesn't solve the underlying problem. This patch only reduces the window for problems, it doesn't fix it.

So what Dries is saying, as David Strauss explained to me is that the old code deleted only blocks for one theme, this one moves every record so there might be a very small window indeed (much smaller than the old). Given that ... and what dangers this carries as nnewton explained... I am killing this solution.

And I do not think this is critical.

typofixed.

Applied patch, enabled statistics module, got this result:

* notice: Undefined variable: blocks in /Users/joshk/Sites/drupal-HEAD/modules/block/block.module on line 258.
* warning: usort() [function.usort]: The argument should be an array in /Users/joshk/Sites/drupal-HEAD/modules/block/block.admin.inc on line 20.
* warning: Invalid argument supplied for foreach() in /Users/joshk/Sites/drupal-HEAD/modules/block/block.admin.inc on line 49.
* warning: Invalid argument supplied for foreach() in /Users/joshk/Sites/drupal-HEAD/modules/block/block-admin-display-form.tpl.php on line 45.

Marking as needs work. :(

Oh, one nice addition: I now pass around the theme_key in hook_block so theoretically you can have different blocks defined per theme. Between module preprocess and this I am now quite content of how modules can intervene to the theme.

Noone cares about schema defaults, they are usually whatever 'empty' is in the context, in this case, that's a zero. There is no intention behind it. It's a whole another issue whether this needs to be fixed. Schema changes are in the patch for install and for update as well.

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