* renamed 'boxes' to 'blocks_custom'
* changed the 'bid' column of that table to 'delta' and removed auto increment in favor of the usual db_next_id
* made associated changes in code to avoid 'box' and 'bid'
* removed unique constriant on custom box titles (http://drupal.org/node/11724)
* put the delete link for custom blocks back in, it was apparently lost in another update

CommentFileSizeAuthor
#16 block_primary_key.patch1.12 KBcatch
#6 block.module_3.diff20.41 KBdrumm
block.module_2.diff9.54 KBdrumm

Comments

jhriggs’s picture

jhriggs commented

I think the only thing I would question is the renaming of bid. It seems inconsistent with the rest of drupal. We use nid, rid, uid, etc., etc. So, this should be a block ID, bid.

drumm’s picture

drumm
he/him
Location NY, US
commented

Delta is the best name since this is what hook_block() calls it.

Adding a bid (b as in block) to the blocks table is a good idea, but I wouldn't want to confuse things with a patch that is renaming bid in one context and adding bid in a related but different context.

dries’s picture

dries commented

I prefer bid. I'd rather change delta to bid, instead of changing bid to delta. It is more consistent. Otherwise the patch looks good.

drumm’s picture

drumm
he/him
Location NY, US
commented

Delta really isn't a good id. It isn't unique. The closest thing to a primary key is the blocks table is (module, delta). I think bid should be added as a real primary key on the blocks table. If there is a good word to use in place of delta that isn't bid, then that would be good to use.

drumm’s picture

drumm
he/him
Location NY, US
commented
Title: Clean up boxes table » Clean up block module tables

Updated patch which adds:
* Added index to weight column in blocks table
* Added primary key bid to blocks table
* Replaced module/delta construct with bid where possible

In additon to the previous:
* renamed 'boxes' to 'blocks_custom'
* changed the 'bid' column of that table to 'delta' and removed auto increment in favor of the usual db_next_id
* made associated changes in code to avoid 'box' and 'bid'
* removed unique constriant on custom box titles (http://drupal.org/node/11724)
* put the delete link for custom blocks back in, it was apparently lost in another update

drumm’s picture

drumm
he/him
Location NY, US
commented
StatusFileSize
new block.module_3.diff20.41 KB

And the patch too

dries’s picture

dries commented

The upgrade path gave me problems. I got a fatal error:

  $ret[] = update_sql('ALTER TABLE {blocks_custom} DROP INDEX title');

The index did not exist on my setup. Weird.

I got several hundred warnings about this line:

  foreach ($blocks as $delta => $block) {

It's somewhat tricky, because you can't execute that update twice. My user and block tables are pretty unusable now. :)

(Is the block->info field still being used? It looks quite redundant to me.)

dries’s picture

dries commented

I ran this update on a copy of the drupal.org database and it seems to work. The reason my previous attempt failed, is because of the way error reporting was configured. My copy of the drupal.org database doesn't have an index on 'blocks_custom.title' either, but it did not result in a fatal error nor did it cause the upgrade script to halt. Warning are suppressed so they are simply not shown.

In short, depending on how error reporting is configured, the upgrade path might or might not work. If it doesn't work, you're screwed because you won't be able to run the upgrade script again. At least, not without intimate knowledge of Drupal's internals.

TDobes’s picture

TDobes commented

It should be noted that the patch here contains a fix for a CRITICAL bug in block.module: At the moment, custom blocks cannot be deleted. A patch containing the fix alone (without other improvements) is located in another issue, which was marked as a duplicate quite a while ago.

Since we're now in a feature freeze, perhaps I should reopen the other issue so the deletion fix alone can be committed?

TDobes’s picture

TDobes commented

Oops... looks like Steven already committed a fix for the problem I noted... it just hasn't shown up in the CVS logs yet. Sorry for the noise.

dries’s picture

dries commented

I guess this patch needs to be updated then?

drumm’s picture

drumm
he/him
Location NY, US
commented

Yes, it should be updated, but probably not for at least a few days. Marking as active for now.

drumm’s picture

drumm
he/him
Location NY, US
commented

Sorry for the noise, but do we want something to allow more then one custom block with a blank title in before Drupal 4.6? This is a database change. Basically simply removing

UNIQUE KEY title (title),

from

CREATE TABLE boxes (

People seem to want to have blocks with no titles.

catch’s picture

catch
he/him
Primary language English
commented
Title: Clean up block module tables » Clean up block module tables - remove bid
Version: x.y.z » 6.x-dev
Category: task » bug
Priority: Normal » Critical

This is still valid. Splitting this out of: http://drupal.org/node/164532 since it's a schema change rather than simply adding an index. Updated patch is available at: http://drupal.org/node/147298 - will reroll the relevant parts into a new one for this issue.

Not sure if this can go into D6 but leaving it as such until someone says otherwise. Also bumping to critical

moshe weitzman’s picture

moshe weitzman commented

why is this critical. and is the title still accurate? please summarize what this issue is currently about.

catch’s picture

catch
he/him
Primary language English
commented
Title: Clean up block module tables - remove bid » Remove {block} bid, replace with primary key
Version: 6.x-dev » 7.x-dev
Priority: Critical » Normal
Status: Active » Needs work
StatusFileSize
new block_primary_key.patch1.12 KB

No problem.

In #140666, David Strauss said:

Creating extra autoincrement columns to be easy PRIMARY keys provides no protection against duplicates. You end up having to create a UNIQUE key on what should be the PRIMARY key and have the database maintain at least two BTREEs for the table.

That issue was marked as critical to reverse changes made by a patch that had been committed, it was closed in favour of #147298, which was closed in favour of #164532 - all critical. Since #164532 is now dealing only with indexes, not autoincrement columns, I figured it'd be a lot cleaner to move the block module changes back to this grandfather issue, which was initially about fixing the block->bid.

Left as critical simply due to the history. It's obviously been like this for a long time, so the existing implementation isn't breaking anything, but blocks are quite an expensive query that are known to kill certain sites, and the new caching solution won't help everyone.

I've attached the most recent version of the changes from those other issues I could find, re-rolled against HEAD. Looking at block.module though, it'll require major refactoring to use the primary key, so leaving this as needs work, normal and against D7.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Wait. What?

This original issue is from 2004. From the description, back in Drupal 0.004 we used to have an auto_increement bid column on blocks, apparently. :P At some point this was changed to a module/delta combo, and during the 6.x dev cycle, the current "bid" primary key was added in. I assume because of query performance, but I'm not sure.

So why are we deciding to go back once more and do WHERE module = 'foo' AND delta = 'bar' rather than WHERE bid = 3? That seems to make no sense to me.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Closed (works as designed)

I have a feeling it'd been a long day on the issue queue that day. Going let this one rest peacefully again.