Issue status update for http://drupal.org/node/30801 Post a follow up: http://drupal.org/project/comments/add/30801 Project: Drupal -Version: 4.6.0 +Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Allie Micka Updated by: m3avrck Status: patch (code needs review) Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB) Awesome patch! Noticed the version was set wrong so changed this to CVS. Also, rerolled the patch, there was an extraneous $region in there no longer needed. Reducing 4 queries is definetly a great start in optimizing Drupal :) Take this patch along with this one (might need to be updated now with this query rewrite I'm thinking): http://drupal.org/node/27157 ... and we'll have Drupal's loading of blocks pretty much maxed out with optimizations, then on to the slooow alias ones... ;) m3avrck Previous comments: ------------------------------------------------------------------------ Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB) The blocks module is running a separate query for each region, e.g.: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='left' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='right' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='header' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='footer' ORDER BY weight, module Ultimately, we're going to need all of these so let's save a few queries. I have changed the query to: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER BY region, weight, module And set all results to the static $blocks array on the first pass.

Issue status update for http://drupal.org/node/30801 Post a follow up: http://drupal.org/project/comments/add/30801 Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Allie Micka Updated by: Dries Status: patch (ready to be committed) Has this been benchmarked? I would expect this to be faster, but it's good practice nonetheless. Feel free to throw that other block query patch into the mix. Dries Previous comments: ------------------------------------------------------------------------ Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB) The blocks module is running a separate query for each region, e.g.: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='left' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='right' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='header' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='footer' ORDER BY weight, module Ultimately, we're going to need all of these so let's save a few queries. I have changed the query to: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER BY region, weight, module And set all results to the static $blocks array on the first pass. ------------------------------------------------------------------------ Sun, 11 Sep 2005 23:50:53 +0000 : m3avrck Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB) Awesome patch! Noticed the version was set wrong so changed this to CVS. Also, rerolled the patch, there was an extraneous $region in there no longer needed. Reducing 4 queries is definetly a great start in optimizing Drupal :) Take this patch along with this one (might need to be updated now with this query rewrite I'm thinking): http://drupal.org/node/27157 ... and we'll have Drupal's loading of blocks pretty much maxed out with optimizations, then on to the slooow alias ones... ;) ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:24:56 +0000 : Souvent22 Gotta love when something is just logical. Makes sense, and the patch worked. I'd say this is ready. Get these blocks sped up a bit. This coupled with 'block primary key' patch should make the blocks solid.

Issue status update for http://drupal.org/node/30801 Post a follow up: http://drupal.org/project/comments/add/30801 Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Allie Micka Updated by: Dries Status: patch (ready to be committed) This patch reduces the SQL overhead but adds some CPU-overhead and memory-overhead. Hence, it's better to benchmark the number of requests/second (for example using ab or ab2) ... Dries Previous comments: ------------------------------------------------------------------------ Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB) The blocks module is running a separate query for each region, e.g.: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='left' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='right' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='header' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='footer' ORDER BY weight, module Ultimately, we're going to need all of these so let's save a few queries. I have changed the query to: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER BY region, weight, module And set all results to the static $blocks array on the first pass. ------------------------------------------------------------------------ Sun, 11 Sep 2005 23:50:53 +0000 : m3avrck Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB) Awesome patch! Noticed the version was set wrong so changed this to CVS. Also, rerolled the patch, there was an extraneous $region in there no longer needed. Reducing 4 queries is definetly a great start in optimizing Drupal :) Take this patch along with this one (might need to be updated now with this query rewrite I'm thinking): http://drupal.org/node/27157 ... and we'll have Drupal's loading of blocks pretty much maxed out with optimizations, then on to the slooow alias ones... ;) ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:24:56 +0000 : Souvent22 Gotta love when something is just logical. Makes sense, and the patch worked. I'd say this is ready. Get these blocks sped up a bit. This coupled with 'block primary key' patch should make the blocks solid. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:00 +0000 : Dries Has this been benchmarked? I would expect this to be faster, but it's good practice nonetheless. Feel free to throw that other block query patch into the mix. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:49:15 +0000 : m3avrck Dries, I ran some simple benchmarks with the devel.module. Queries with a default install were reduced from 125 to 121. Query times averaged around 50ms and this was reduced to roughly 47-48ms. It's not a *huge* savings, but on a a dev machine, with only one user, this is still good nonetheless. I imagine on a hard hit Drupal site this would start to scale up nicely with time/load savings. I'll reroll a patch that combines the two in a few.

Issue status update for http://drupal.org/node/30801 Post a follow up: http://drupal.org/project/comments/add/30801 Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Allie Micka Updated by: Allie Micka Status: patch (ready to be committed) This patch will not increase memory overhead unless a theme has one or more regions that are defined but not in use. Otherwise, the static $blocks array will be identical with or without the patch applied. The patch also does not increase CPU overhead because less code is processed. Without the patch, the entire query and parsing routine is executed 5 or more times (everything between if (!isset($blocks[$region])) { } ). With the patch, the same code executes only once. But the proof is in the pudding. I'm not sure what your standards are for ab, but I like to do 1 serial check and 1 check with concurrency so I can rule out performance gained by increased memory consumption, which bogs down concurrent requests. This patch appears to save an average of 9-12ms per request and increase performance by .05 requests per second. The relevant bits from my ab results are as follows: Before Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.32 [#/sec] (mean) Time per request: 430.20 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.16 [#/sec] (mean) Time per request: 4631.00 [ms] (mean) Time per request: 463.10 [ms] (mean, across all concurrent requests) After Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.37 [#/sec] (mean) Time per request: 421.08 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.22 [#/sec] (mean) Time per request: 4510.90 [ms] (mean) Time per request: 451.09 [ms] (mean, across all concurrent requests) Allie Micka Previous comments: ------------------------------------------------------------------------ Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB) The blocks module is running a separate query for each region, e.g.: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='left' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='right' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='header' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='footer' ORDER BY weight, module Ultimately, we're going to need all of these so let's save a few queries. I have changed the query to: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER BY region, weight, module And set all results to the static $blocks array on the first pass. ------------------------------------------------------------------------ Sun, 11 Sep 2005 23:50:53 +0000 : m3avrck Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB) Awesome patch! Noticed the version was set wrong so changed this to CVS. Also, rerolled the patch, there was an extraneous $region in there no longer needed. Reducing 4 queries is definetly a great start in optimizing Drupal :) Take this patch along with this one (might need to be updated now with this query rewrite I'm thinking): http://drupal.org/node/27157 ... and we'll have Drupal's loading of blocks pretty much maxed out with optimizations, then on to the slooow alias ones... ;) ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:24:56 +0000 : Souvent22 Gotta love when something is just logical. Makes sense, and the patch worked. I'd say this is ready. Get these blocks sped up a bit. This coupled with 'block primary key' patch should make the blocks solid. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:00 +0000 : Dries Has this been benchmarked? I would expect this to be faster, but it's good practice nonetheless. Feel free to throw that other block query patch into the mix. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:49:15 +0000 : m3avrck Dries, I ran some simple benchmarks with the devel.module. Queries with a default install were reduced from 125 to 121. Query times averaged around 50ms and this was reduced to roughly 47-48ms. It's not a *huge* savings, but on a a dev machine, with only one user, this is still good nonetheless. I imagine on a hard hit Drupal site this would start to scale up nicely with time/load savings. I'll reroll a patch that combines the two in a few. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:26:08 +0000 : Dries This patch reduces the SQL overhead but adds some CPU-overhead and memory-overhead. Hence, it's better to benchmark the number of requests/second (for example using ab or ab2) ... ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:35:54 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_14.patch (3.31 KB) Ok here is an updated patch, along with the blocks table modification of adding a PRIMARY KEY(module,delta). Spent sometime on #mysql discussing best practice for this table and adding a primary key is the best bet. Any indexes would not improve performance since this table will probably almost never have more than 30 rows in it. The primary key itself is slightly neglible as well, however, if any query were to perform advanced queries on this table, the primary key would benefit, so in best practice, we should have this as well. I'll try to get some benchmarks up soon backing these simple changes.

Issue status update for http://drupal.org/node/30801 Post a follow up: http://drupal.org/project/comments/add/30801 Project: Drupal Version: cvs Component: block.module Category: feature requests -Priority: normal +Priority: critical Assigned to: Anonymous Reported by: Allie Micka Updated by: m3avrck -Status: active +Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/database_3.patch (1.96 KB) Eeek!!! Forgot to test with multiple themes, this patches fixes the key definition. m3avrck Previous comments: ------------------------------------------------------------------------ Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB) The blocks module is running a separate query for each region, e.g.: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='left' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='right' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='header' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='footer' ORDER BY weight, module Ultimately, we're going to need all of these so let's save a few queries. I have changed the query to: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER BY region, weight, module And set all results to the static $blocks array on the first pass. ------------------------------------------------------------------------ Sun, 11 Sep 2005 23:50:53 +0000 : m3avrck Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB) Awesome patch! Noticed the version was set wrong so changed this to CVS. Also, rerolled the patch, there was an extraneous $region in there no longer needed. Reducing 4 queries is definetly a great start in optimizing Drupal :) Take this patch along with this one (might need to be updated now with this query rewrite I'm thinking): http://drupal.org/node/27157 ... and we'll have Drupal's loading of blocks pretty much maxed out with optimizations, then on to the slooow alias ones... ;) ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:24:56 +0000 : Souvent22 Gotta love when something is just logical. Makes sense, and the patch worked. I'd say this is ready. Get these blocks sped up a bit. This coupled with 'block primary key' patch should make the blocks solid. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:00 +0000 : Dries Has this been benchmarked? I would expect this to be faster, but it's good practice nonetheless. Feel free to throw that other block query patch into the mix. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:49:15 +0000 : m3avrck Dries, I ran some simple benchmarks with the devel.module. Queries with a default install were reduced from 125 to 121. Query times averaged around 50ms and this was reduced to roughly 47-48ms. It's not a *huge* savings, but on a a dev machine, with only one user, this is still good nonetheless. I imagine on a hard hit Drupal site this would start to scale up nicely with time/load savings. I'll reroll a patch that combines the two in a few. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:26:08 +0000 : Dries This patch reduces the SQL overhead but adds some CPU-overhead and memory-overhead. Hence, it's better to benchmark the number of requests/second (for example using ab or ab2) ... ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:35:54 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_14.patch (3.31 KB) Ok here is an updated patch, along with the blocks table modification of adding a PRIMARY KEY(module,delta). Spent sometime on #mysql discussing best practice for this table and adding a primary key is the best bet. Any indexes would not improve performance since this table will probably almost never have more than 30 rows in it. The primary key itself is slightly neglible as well, however, if any query were to perform advanced queries on this table, the primary key would benefit, so in best practice, we should have this as well. I'll try to get some benchmarks up soon backing these simple changes. ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:49:34 +0000 : Allie Micka This patch will not increase memory overhead unless a theme has one or more regions that are defined but not in use. Otherwise, the static $blocks array will be identical with or without the patch applied. The patch also does not increase CPU overhead because less code is processed. Without the patch, the entire query and parsing routine is executed 5 or more times (everything between if (!isset($blocks[$region])) { } ). With the patch, the same code executes only once. But the proof is in the pudding. I'm not sure what your standards are for ab, but I like to do 1 serial check and 1 check with concurrency so I can rule out performance gained by increased memory consumption, which bogs down concurrent requests. This patch appears to save an average of 9-12ms per request and increase performance by .05 requests per second. The relevant bits from my ab results are as follows: Before Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.32 [#/sec] (mean) Time per request: 430.20 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.16 [#/sec] (mean) Time per request: 4631.00 [ms] (mean) Time per request: 463.10 [ms] (mean, across all concurrent requests) After Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.37 [#/sec] (mean) Time per request: 421.08 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.22 [#/sec] (mean) Time per request: 4510.90 [ms] (mean) Time per request: 451.09 [ms] (mean, across all concurrent requests) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:04:27 +0000 : m3avrck Exactly running similar to what I was uncovering as well. So seems this patch is ready to go, time to commit :) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:27:32 +0000 : Dries Thanks for the measurements! Patch committed to HEAD. Thanks. ------------------------------------------------------------------------ Tue, 13 Sep 2005 07:37:26 +0000 : asimmonds With this patch committed, and updating my HEAD install with update.php, I get the error "Duplicate entry 'user-0' for key 1" from mysql. I have two themes configured and there is a primary key (module,delta) clash between the two themes.

Issue status update for http://drupal.org/node/30801 Post a follow up: http://drupal.org/project/comments/add/30801 Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: Allie Micka Updated by: mathias Status: patch (ready to be committed) I can recreate this bug when running update.php but I question the necessity of this index in the first place. I couldn't find one SELECT query in block.module that benefit from it. mathias Previous comments: ------------------------------------------------------------------------ Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB) The blocks module is running a separate query for each region, e.g.: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='left' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='right' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='header' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='footer' ORDER BY weight, module Ultimately, we're going to need all of these so let's save a few queries. I have changed the query to: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER BY region, weight, module And set all results to the static $blocks array on the first pass. ------------------------------------------------------------------------ Sun, 11 Sep 2005 23:50:53 +0000 : m3avrck Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB) Awesome patch! Noticed the version was set wrong so changed this to CVS. Also, rerolled the patch, there was an extraneous $region in there no longer needed. Reducing 4 queries is definetly a great start in optimizing Drupal :) Take this patch along with this one (might need to be updated now with this query rewrite I'm thinking): http://drupal.org/node/27157 ... and we'll have Drupal's loading of blocks pretty much maxed out with optimizations, then on to the slooow alias ones... ;) ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:24:56 +0000 : Souvent22 Gotta love when something is just logical. Makes sense, and the patch worked. I'd say this is ready. Get these blocks sped up a bit. This coupled with 'block primary key' patch should make the blocks solid. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:00 +0000 : Dries Has this been benchmarked? I would expect this to be faster, but it's good practice nonetheless. Feel free to throw that other block query patch into the mix. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:49:15 +0000 : m3avrck Dries, I ran some simple benchmarks with the devel.module. Queries with a default install were reduced from 125 to 121. Query times averaged around 50ms and this was reduced to roughly 47-48ms. It's not a *huge* savings, but on a a dev machine, with only one user, this is still good nonetheless. I imagine on a hard hit Drupal site this would start to scale up nicely with time/load savings. I'll reroll a patch that combines the two in a few. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:26:08 +0000 : Dries This patch reduces the SQL overhead but adds some CPU-overhead and memory-overhead. Hence, it's better to benchmark the number of requests/second (for example using ab or ab2) ... ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:35:54 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_14.patch (3.31 KB) Ok here is an updated patch, along with the blocks table modification of adding a PRIMARY KEY(module,delta). Spent sometime on #mysql discussing best practice for this table and adding a primary key is the best bet. Any indexes would not improve performance since this table will probably almost never have more than 30 rows in it. The primary key itself is slightly neglible as well, however, if any query were to perform advanced queries on this table, the primary key would benefit, so in best practice, we should have this as well. I'll try to get some benchmarks up soon backing these simple changes. ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:49:34 +0000 : Allie Micka This patch will not increase memory overhead unless a theme has one or more regions that are defined but not in use. Otherwise, the static $blocks array will be identical with or without the patch applied. The patch also does not increase CPU overhead because less code is processed. Without the patch, the entire query and parsing routine is executed 5 or more times (everything between if (!isset($blocks[$region])) { } ). With the patch, the same code executes only once. But the proof is in the pudding. I'm not sure what your standards are for ab, but I like to do 1 serial check and 1 check with concurrency so I can rule out performance gained by increased memory consumption, which bogs down concurrent requests. This patch appears to save an average of 9-12ms per request and increase performance by .05 requests per second. The relevant bits from my ab results are as follows: Before Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.32 [#/sec] (mean) Time per request: 430.20 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.16 [#/sec] (mean) Time per request: 4631.00 [ms] (mean) Time per request: 463.10 [ms] (mean, across all concurrent requests) After Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.37 [#/sec] (mean) Time per request: 421.08 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.22 [#/sec] (mean) Time per request: 4510.90 [ms] (mean) Time per request: 451.09 [ms] (mean, across all concurrent requests) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:04:27 +0000 : m3avrck Exactly running similar to what I was uncovering as well. So seems this patch is ready to go, time to commit :) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:27:32 +0000 : Dries Thanks for the measurements! Patch committed to HEAD. Thanks. ------------------------------------------------------------------------ Tue, 13 Sep 2005 07:37:26 +0000 : asimmonds With this patch committed, and updating my HEAD install with update.php, I get the error "Duplicate entry 'user-0' for key 1" from mysql. I have two themes configured and there is a primary key (module,delta) clash between the two themes. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:39:43 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_3.patch (1.96 KB) Eeek!!! Forgot to test with multiple themes, this patches fixes the key definition. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:41:46 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_4.patch (1.96 KB) Noticed a typo in the PGSQL.

Issue status update for http://drupal.org/node/30801 Post a follow up: http://drupal.org/project/comments/add/30801 Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: Allie Micka Updated by: m3avrck Status: patch (ready to be committed) Just chatted with mathias on IRC. I think there was some slight confusion and I apologize for this. The other issue is related to this issue indeed as it affects the block query and that is why Dries above mentioned to incorporate that fix into this one. Then both of these were rolled into the latest HEAD. However, a small oversight on my part, the key wasn't unique in all situations, and this above patch rectifies this situation. Now, in terms of performance, the key doesn't affect performance in any measurable way. Rather, it adds data integrity and structure to the table, and enhances other *unknown and future* queries. I chatted with a knowledgeable fellow on #mysql a while about this table and it was determined that a key *was* beneficial, even if any performance gains were not. In hindsight, maybe these patches shouldn't have been put together, but as such, they were, and this above patches fixes this issue and brings us back up to speed. So basically this patch "consolidates block queries *and* adds data integrity to the block table". :) m3avrck Previous comments: ------------------------------------------------------------------------ Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB) The blocks module is running a separate query for each region, e.g.: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='left' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='right' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='header' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='footer' ORDER BY weight, module Ultimately, we're going to need all of these so let's save a few queries. I have changed the query to: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER BY region, weight, module And set all results to the static $blocks array on the first pass. ------------------------------------------------------------------------ Sun, 11 Sep 2005 23:50:53 +0000 : m3avrck Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB) Awesome patch! Noticed the version was set wrong so changed this to CVS. Also, rerolled the patch, there was an extraneous $region in there no longer needed. Reducing 4 queries is definetly a great start in optimizing Drupal :) Take this patch along with this one (might need to be updated now with this query rewrite I'm thinking): http://drupal.org/node/27157 ... and we'll have Drupal's loading of blocks pretty much maxed out with optimizations, then on to the slooow alias ones... ;) ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:24:56 +0000 : Souvent22 Gotta love when something is just logical. Makes sense, and the patch worked. I'd say this is ready. Get these blocks sped up a bit. This coupled with 'block primary key' patch should make the blocks solid. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:00 +0000 : Dries Has this been benchmarked? I would expect this to be faster, but it's good practice nonetheless. Feel free to throw that other block query patch into the mix. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:49:15 +0000 : m3avrck Dries, I ran some simple benchmarks with the devel.module. Queries with a default install were reduced from 125 to 121. Query times averaged around 50ms and this was reduced to roughly 47-48ms. It's not a *huge* savings, but on a a dev machine, with only one user, this is still good nonetheless. I imagine on a hard hit Drupal site this would start to scale up nicely with time/load savings. I'll reroll a patch that combines the two in a few. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:26:08 +0000 : Dries This patch reduces the SQL overhead but adds some CPU-overhead and memory-overhead. Hence, it's better to benchmark the number of requests/second (for example using ab or ab2) ... ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:35:54 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_14.patch (3.31 KB) Ok here is an updated patch, along with the blocks table modification of adding a PRIMARY KEY(module,delta). Spent sometime on #mysql discussing best practice for this table and adding a primary key is the best bet. Any indexes would not improve performance since this table will probably almost never have more than 30 rows in it. The primary key itself is slightly neglible as well, however, if any query were to perform advanced queries on this table, the primary key would benefit, so in best practice, we should have this as well. I'll try to get some benchmarks up soon backing these simple changes. ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:49:34 +0000 : Allie Micka This patch will not increase memory overhead unless a theme has one or more regions that are defined but not in use. Otherwise, the static $blocks array will be identical with or without the patch applied. The patch also does not increase CPU overhead because less code is processed. Without the patch, the entire query and parsing routine is executed 5 or more times (everything between if (!isset($blocks[$region])) { } ). With the patch, the same code executes only once. But the proof is in the pudding. I'm not sure what your standards are for ab, but I like to do 1 serial check and 1 check with concurrency so I can rule out performance gained by increased memory consumption, which bogs down concurrent requests. This patch appears to save an average of 9-12ms per request and increase performance by .05 requests per second. The relevant bits from my ab results are as follows: Before Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.32 [#/sec] (mean) Time per request: 430.20 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.16 [#/sec] (mean) Time per request: 4631.00 [ms] (mean) Time per request: 463.10 [ms] (mean, across all concurrent requests) After Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.37 [#/sec] (mean) Time per request: 421.08 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.22 [#/sec] (mean) Time per request: 4510.90 [ms] (mean) Time per request: 451.09 [ms] (mean, across all concurrent requests) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:04:27 +0000 : m3avrck Exactly running similar to what I was uncovering as well. So seems this patch is ready to go, time to commit :) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:27:32 +0000 : Dries Thanks for the measurements! Patch committed to HEAD. Thanks. ------------------------------------------------------------------------ Tue, 13 Sep 2005 07:37:26 +0000 : asimmonds With this patch committed, and updating my HEAD install with update.php, I get the error "Duplicate entry 'user-0' for key 1" from mysql. I have two themes configured and there is a primary key (module,delta) clash between the two themes. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:39:43 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_3.patch (1.96 KB) Eeek!!! Forgot to test with multiple themes, this patches fixes the key definition. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:41:46 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_4.patch (1.96 KB) Noticed a typo in the PGSQL. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:00:19 +0000 : mathias I can recreate this bug when running update.php but I question the necessity of this index in the first place. I couldn't find one SELECT query in block.module that benefit from it. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:18:07 +0000 : Allie Micka For what it's worth, the benchmarks I submitted did not include the key/index patch, they only included the query consolidation patch. The performance impacts of the patch that hijacked this issue are unknown. I'd suggest moving this issue back to http://drupal.org/node/27157 , where you can assess performance impacts and the back out or correct that patch based on the results. Alternatively, continue using this issue but supply a few benchmarks to justify adding and/or removing the block table indexes. Thanks!

Issue status update for http://drupal.org/node/30801 Post a follow up: http://drupal.org/project/comments/add/30801 Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: Allie Micka Updated by: m3avrck Status: patch (code needs work) I1uv4t4r, I welcome your comments. However, this patch was thorougly tested on this end. PHP5.0.5 with MySQL 4.1.11 yields *no* errors at all. Everything works as expected. The reason for the first hiccup was that I only had 1 theme enabled, once I tried 2 themes, I ran into the duplicate key error, which was easily fixed by modifying the key. Running that fine on my machine here. Can you be more specific as to the problems you encountered and which version fo PHP / MySQL you are using? m3avrck Previous comments: ------------------------------------------------------------------------ Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB) The blocks module is running a separate query for each region, e.g.: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='left' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='right' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='header' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='footer' ORDER BY weight, module Ultimately, we're going to need all of these so let's save a few queries. I have changed the query to: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER BY region, weight, module And set all results to the static $blocks array on the first pass. ------------------------------------------------------------------------ Sun, 11 Sep 2005 23:50:53 +0000 : m3avrck Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB) Awesome patch! Noticed the version was set wrong so changed this to CVS. Also, rerolled the patch, there was an extraneous $region in there no longer needed. Reducing 4 queries is definetly a great start in optimizing Drupal :) Take this patch along with this one (might need to be updated now with this query rewrite I'm thinking): http://drupal.org/node/27157 ... and we'll have Drupal's loading of blocks pretty much maxed out with optimizations, then on to the slooow alias ones... ;) ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:24:56 +0000 : Souvent22 Gotta love when something is just logical. Makes sense, and the patch worked. I'd say this is ready. Get these blocks sped up a bit. This coupled with 'block primary key' patch should make the blocks solid. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:00 +0000 : Dries Has this been benchmarked? I would expect this to be faster, but it's good practice nonetheless. Feel free to throw that other block query patch into the mix. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:49:15 +0000 : m3avrck Dries, I ran some simple benchmarks with the devel.module. Queries with a default install were reduced from 125 to 121. Query times averaged around 50ms and this was reduced to roughly 47-48ms. It's not a *huge* savings, but on a a dev machine, with only one user, this is still good nonetheless. I imagine on a hard hit Drupal site this would start to scale up nicely with time/load savings. I'll reroll a patch that combines the two in a few. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:26:08 +0000 : Dries This patch reduces the SQL overhead but adds some CPU-overhead and memory-overhead. Hence, it's better to benchmark the number of requests/second (for example using ab or ab2) ... ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:35:54 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_14.patch (3.31 KB) Ok here is an updated patch, along with the blocks table modification of adding a PRIMARY KEY(module,delta). Spent sometime on #mysql discussing best practice for this table and adding a primary key is the best bet. Any indexes would not improve performance since this table will probably almost never have more than 30 rows in it. The primary key itself is slightly neglible as well, however, if any query were to perform advanced queries on this table, the primary key would benefit, so in best practice, we should have this as well. I'll try to get some benchmarks up soon backing these simple changes. ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:49:34 +0000 : Allie Micka This patch will not increase memory overhead unless a theme has one or more regions that are defined but not in use. Otherwise, the static $blocks array will be identical with or without the patch applied. The patch also does not increase CPU overhead because less code is processed. Without the patch, the entire query and parsing routine is executed 5 or more times (everything between if (!isset($blocks[$region])) { } ). With the patch, the same code executes only once. But the proof is in the pudding. I'm not sure what your standards are for ab, but I like to do 1 serial check and 1 check with concurrency so I can rule out performance gained by increased memory consumption, which bogs down concurrent requests. This patch appears to save an average of 9-12ms per request and increase performance by .05 requests per second. The relevant bits from my ab results are as follows: Before Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.32 [#/sec] (mean) Time per request: 430.20 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.16 [#/sec] (mean) Time per request: 4631.00 [ms] (mean) Time per request: 463.10 [ms] (mean, across all concurrent requests) After Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.37 [#/sec] (mean) Time per request: 421.08 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.22 [#/sec] (mean) Time per request: 4510.90 [ms] (mean) Time per request: 451.09 [ms] (mean, across all concurrent requests) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:04:27 +0000 : m3avrck Exactly running similar to what I was uncovering as well. So seems this patch is ready to go, time to commit :) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:27:32 +0000 : Dries Thanks for the measurements! Patch committed to HEAD. Thanks. ------------------------------------------------------------------------ Tue, 13 Sep 2005 07:37:26 +0000 : asimmonds With this patch committed, and updating my HEAD install with update.php, I get the error "Duplicate entry 'user-0' for key 1" from mysql. I have two themes configured and there is a primary key (module,delta) clash between the two themes. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:39:43 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_3.patch (1.96 KB) Eeek!!! Forgot to test with multiple themes, this patches fixes the key definition. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:41:46 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_4.patch (1.96 KB) Noticed a typo in the PGSQL. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:00:19 +0000 : mathias I can recreate this bug when running update.php but I question the necessity of this index in the first place. I couldn't find one SELECT query in block.module that benefit from it. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:18:07 +0000 : Allie Micka For what it's worth, the benchmarks I submitted did not include the key/index patch, they only included the query consolidation patch. The performance impacts of the patch that hijacked this issue are unknown. I'd suggest moving this issue back to http://drupal.org/node/27157 , where you can assess performance impacts and the back out or correct that patch based on the results. Alternatively, continue using this issue but supply a few benchmarks to justify adding and/or removing the block table indexes. Thanks! ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:56:32 +0000 : m3avrck Just chatted with mathias on IRC. I think there was some slight confusion and I apologize for this. The other issue is related to this issue indeed as it affects the block query and that is why Dries above mentioned to incorporate that fix into this one. Then both of these were rolled into the latest HEAD. However, a small oversight on my part, the key wasn't unique in all situations, and this above patch rectifies this situation. Now, in terms of performance, the key doesn't affect performance in any measurable way. Rather, it adds data integrity and structure to the table, and enhances other *unknown and future* queries. I chatted with a knowledgeable fellow on #mysql a while about this table and it was determined that a key *was* beneficial, even if any performance gains were not. In hindsight, maybe these patches shouldn't have been put together, but as such, they were, and this above patches fixes this issue and brings us back up to speed. So basically this patch "consolidates block queries *and* adds data integrity to the block table". :) ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:19:08 +0000 : I1uv4t4r Attachment: http://drupal.org/files/issues/block.module_6.patch (966 bytes) First I want to say that I am astonished that those patches have made it into CVS. Both the database and block.module patches don't work for me and cause significant errors. Firstly, a primary key of module, delta and theme makes MySQL throw up a 'key too long' error. Secondly, in block.module an array is fetched from the database and then it is used as an object, and the blocks are added to the region the function is called for. You'll understand when you see my patch. I must say that my patch is not tested thouroughly, but I believe it impossible that the patches above are even tested at all, but tell me if I'm wrong. The patch I provide is solely for block.module by the way.

Issue status update for http://drupal.org/node/30801 Post a follow up: http://drupal.org/project/comments/add/30801 Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: Allie Micka Updated by: Dries Status: patch (code needs work) This is somewhat hairy: + $blocks[$block['region']]["$block[module]_$block[delta]"] = (object) $block; Can't that be done in a cleaner fashion? Dries Previous comments: ------------------------------------------------------------------------ Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB) The blocks module is running a separate query for each region, e.g.: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='left' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='right' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='header' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='footer' ORDER BY weight, module Ultimately, we're going to need all of these so let's save a few queries. I have changed the query to: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER BY region, weight, module And set all results to the static $blocks array on the first pass. ------------------------------------------------------------------------ Sun, 11 Sep 2005 23:50:53 +0000 : m3avrck Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB) Awesome patch! Noticed the version was set wrong so changed this to CVS. Also, rerolled the patch, there was an extraneous $region in there no longer needed. Reducing 4 queries is definetly a great start in optimizing Drupal :) Take this patch along with this one (might need to be updated now with this query rewrite I'm thinking): http://drupal.org/node/27157 ... and we'll have Drupal's loading of blocks pretty much maxed out with optimizations, then on to the slooow alias ones... ;) ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:24:56 +0000 : Souvent22 Gotta love when something is just logical. Makes sense, and the patch worked. I'd say this is ready. Get these blocks sped up a bit. This coupled with 'block primary key' patch should make the blocks solid. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:00 +0000 : Dries Has this been benchmarked? I would expect this to be faster, but it's good practice nonetheless. Feel free to throw that other block query patch into the mix. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:49:15 +0000 : m3avrck Dries, I ran some simple benchmarks with the devel.module. Queries with a default install were reduced from 125 to 121. Query times averaged around 50ms and this was reduced to roughly 47-48ms. It's not a *huge* savings, but on a a dev machine, with only one user, this is still good nonetheless. I imagine on a hard hit Drupal site this would start to scale up nicely with time/load savings. I'll reroll a patch that combines the two in a few. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:26:08 +0000 : Dries This patch reduces the SQL overhead but adds some CPU-overhead and memory-overhead. Hence, it's better to benchmark the number of requests/second (for example using ab or ab2) ... ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:35:54 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_14.patch (3.31 KB) Ok here is an updated patch, along with the blocks table modification of adding a PRIMARY KEY(module,delta). Spent sometime on #mysql discussing best practice for this table and adding a primary key is the best bet. Any indexes would not improve performance since this table will probably almost never have more than 30 rows in it. The primary key itself is slightly neglible as well, however, if any query were to perform advanced queries on this table, the primary key would benefit, so in best practice, we should have this as well. I'll try to get some benchmarks up soon backing these simple changes. ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:49:34 +0000 : Allie Micka This patch will not increase memory overhead unless a theme has one or more regions that are defined but not in use. Otherwise, the static $blocks array will be identical with or without the patch applied. The patch also does not increase CPU overhead because less code is processed. Without the patch, the entire query and parsing routine is executed 5 or more times (everything between if (!isset($blocks[$region])) { } ). With the patch, the same code executes only once. But the proof is in the pudding. I'm not sure what your standards are for ab, but I like to do 1 serial check and 1 check with concurrency so I can rule out performance gained by increased memory consumption, which bogs down concurrent requests. This patch appears to save an average of 9-12ms per request and increase performance by .05 requests per second. The relevant bits from my ab results are as follows: Before Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.32 [#/sec] (mean) Time per request: 430.20 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.16 [#/sec] (mean) Time per request: 4631.00 [ms] (mean) Time per request: 463.10 [ms] (mean, across all concurrent requests) After Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.37 [#/sec] (mean) Time per request: 421.08 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.22 [#/sec] (mean) Time per request: 4510.90 [ms] (mean) Time per request: 451.09 [ms] (mean, across all concurrent requests) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:04:27 +0000 : m3avrck Exactly running similar to what I was uncovering as well. So seems this patch is ready to go, time to commit :) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:27:32 +0000 : Dries Thanks for the measurements! Patch committed to HEAD. Thanks. ------------------------------------------------------------------------ Tue, 13 Sep 2005 07:37:26 +0000 : asimmonds With this patch committed, and updating my HEAD install with update.php, I get the error "Duplicate entry 'user-0' for key 1" from mysql. I have two themes configured and there is a primary key (module,delta) clash between the two themes. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:39:43 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_3.patch (1.96 KB) Eeek!!! Forgot to test with multiple themes, this patches fixes the key definition. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:41:46 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_4.patch (1.96 KB) Noticed a typo in the PGSQL. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:00:19 +0000 : mathias I can recreate this bug when running update.php but I question the necessity of this index in the first place. I couldn't find one SELECT query in block.module that benefit from it. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:18:07 +0000 : Allie Micka For what it's worth, the benchmarks I submitted did not include the key/index patch, they only included the query consolidation patch. The performance impacts of the patch that hijacked this issue are unknown. I'd suggest moving this issue back to http://drupal.org/node/27157 , where you can assess performance impacts and the back out or correct that patch based on the results. Alternatively, continue using this issue but supply a few benchmarks to justify adding and/or removing the block table indexes. Thanks! ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:56:32 +0000 : m3avrck Just chatted with mathias on IRC. I think there was some slight confusion and I apologize for this. The other issue is related to this issue indeed as it affects the block query and that is why Dries above mentioned to incorporate that fix into this one. Then both of these were rolled into the latest HEAD. However, a small oversight on my part, the key wasn't unique in all situations, and this above patch rectifies this situation. Now, in terms of performance, the key doesn't affect performance in any measurable way. Rather, it adds data integrity and structure to the table, and enhances other *unknown and future* queries. I chatted with a knowledgeable fellow on #mysql a while about this table and it was determined that a key *was* beneficial, even if any performance gains were not. In hindsight, maybe these patches shouldn't have been put together, but as such, they were, and this above patches fixes this issue and brings us back up to speed. So basically this patch "consolidates block queries *and* adds data integrity to the block table". :) ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:19:08 +0000 : I1uv4t4r Attachment: http://drupal.org/files/issues/block.module_6.patch (966 bytes) First I want to say that I am astonished that those patches have made it into CVS. Both the database and block.module patches don't work for me and cause significant errors. Firstly, a primary key of module, delta and theme makes MySQL throw up a 'key too long' error. Secondly, in block.module an array is fetched from the database and then it is used as an object, and the blocks are added to the region the function is called for. You'll understand when you see my patch. I must say that my patch is not tested thouroughly, but I believe it impossible that the patches above are even tested at all, but tell me if I'm wrong. The patch I provide is solely for block.module by the way. ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:59:01 +0000 : m3avrck I1uv4t4r, I welcome your comments. However, this patch was thorougly tested on this end. PHP5.0.5 with MySQL 4.1.11 yields *no* errors at all. Everything works as expected. The reason for the first hiccup was that I only had 1 theme enabled, once I tried 2 themes, I ran into the duplicate key error, which was easily fixed by modifying the key. Running that fine on my machine here. Can you be more specific as to the problems you encountered and which version fo PHP / MySQL you are using? ------------------------------------------------------------------------ Tue, 13 Sep 2005 20:29:17 +0000 : Allie Micka No, I1uv4t4r is right. It would have been helpful if he provided more information about his platform (PHP, MySQL versions, etc.) and less editorializing, but what he says is true and the error is mine. I'm a bit confused by Drupal's inconsistent use of db_fetch_array() versus db_fetch_object(). It seems that the latter approach is more common, so I do it that way myself. But that's no excuse for my missing use of arrays in in block.module! My patch didn't break anything on my 4.3.11 system but it might be a problem for an earlier PHP version. I have tested I1uv4t4r's patch and it doesn't change any behavior on my local system, so I'd consider it "ready to commit". Again, there are two patch threads moving along in this issue. I believe that there are still one or two unresolved issues pertaining to the key/index patch. Please commit block.module_6.patch so we can narrow things down again. Meanwhile, I1uv4t4r - please tell us which version of MySQL (and PHP) you are working with. It may help us troublshoot your other problems. Thanks, and sorry for all the drama!

Issue status update for http://drupal.org/node/30801 Post a follow up: http://drupal.org/project/comments/add/30801 Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: Allie Micka Updated by: I1uv4t4r Status: patch (code needs work) Attachment: http://drupal.org/files/issues/block.module_7.patch (968 bytes) Here's an updated patch which decreases the 'hairyness'. I1uv4t4r Previous comments: ------------------------------------------------------------------------ Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB) The blocks module is running a separate query for each region, e.g.: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='left' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='right' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='header' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='footer' ORDER BY weight, module Ultimately, we're going to need all of these so let's save a few queries. I have changed the query to: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER BY region, weight, module And set all results to the static $blocks array on the first pass. ------------------------------------------------------------------------ Sun, 11 Sep 2005 23:50:53 +0000 : m3avrck Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB) Awesome patch! Noticed the version was set wrong so changed this to CVS. Also, rerolled the patch, there was an extraneous $region in there no longer needed. Reducing 4 queries is definetly a great start in optimizing Drupal :) Take this patch along with this one (might need to be updated now with this query rewrite I'm thinking): http://drupal.org/node/27157 ... and we'll have Drupal's loading of blocks pretty much maxed out with optimizations, then on to the slooow alias ones... ;) ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:24:56 +0000 : Souvent22 Gotta love when something is just logical. Makes sense, and the patch worked. I'd say this is ready. Get these blocks sped up a bit. This coupled with 'block primary key' patch should make the blocks solid. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:00 +0000 : Dries Has this been benchmarked? I would expect this to be faster, but it's good practice nonetheless. Feel free to throw that other block query patch into the mix. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:49:15 +0000 : m3avrck Dries, I ran some simple benchmarks with the devel.module. Queries with a default install were reduced from 125 to 121. Query times averaged around 50ms and this was reduced to roughly 47-48ms. It's not a *huge* savings, but on a a dev machine, with only one user, this is still good nonetheless. I imagine on a hard hit Drupal site this would start to scale up nicely with time/load savings. I'll reroll a patch that combines the two in a few. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:26:08 +0000 : Dries This patch reduces the SQL overhead but adds some CPU-overhead and memory-overhead. Hence, it's better to benchmark the number of requests/second (for example using ab or ab2) ... ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:35:54 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_14.patch (3.31 KB) Ok here is an updated patch, along with the blocks table modification of adding a PRIMARY KEY(module,delta). Spent sometime on #mysql discussing best practice for this table and adding a primary key is the best bet. Any indexes would not improve performance since this table will probably almost never have more than 30 rows in it. The primary key itself is slightly neglible as well, however, if any query were to perform advanced queries on this table, the primary key would benefit, so in best practice, we should have this as well. I'll try to get some benchmarks up soon backing these simple changes. ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:49:34 +0000 : Allie Micka This patch will not increase memory overhead unless a theme has one or more regions that are defined but not in use. Otherwise, the static $blocks array will be identical with or without the patch applied. The patch also does not increase CPU overhead because less code is processed. Without the patch, the entire query and parsing routine is executed 5 or more times (everything between if (!isset($blocks[$region])) { } ). With the patch, the same code executes only once. But the proof is in the pudding. I'm not sure what your standards are for ab, but I like to do 1 serial check and 1 check with concurrency so I can rule out performance gained by increased memory consumption, which bogs down concurrent requests. This patch appears to save an average of 9-12ms per request and increase performance by .05 requests per second. The relevant bits from my ab results are as follows: Before Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.32 [#/sec] (mean) Time per request: 430.20 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.16 [#/sec] (mean) Time per request: 4631.00 [ms] (mean) Time per request: 463.10 [ms] (mean, across all concurrent requests) After Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.37 [#/sec] (mean) Time per request: 421.08 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.22 [#/sec] (mean) Time per request: 4510.90 [ms] (mean) Time per request: 451.09 [ms] (mean, across all concurrent requests) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:04:27 +0000 : m3avrck Exactly running similar to what I was uncovering as well. So seems this patch is ready to go, time to commit :) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:27:32 +0000 : Dries Thanks for the measurements! Patch committed to HEAD. Thanks. ------------------------------------------------------------------------ Tue, 13 Sep 2005 07:37:26 +0000 : asimmonds With this patch committed, and updating my HEAD install with update.php, I get the error "Duplicate entry 'user-0' for key 1" from mysql. I have two themes configured and there is a primary key (module,delta) clash between the two themes. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:39:43 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_3.patch (1.96 KB) Eeek!!! Forgot to test with multiple themes, this patches fixes the key definition. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:41:46 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_4.patch (1.96 KB) Noticed a typo in the PGSQL. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:00:19 +0000 : mathias I can recreate this bug when running update.php but I question the necessity of this index in the first place. I couldn't find one SELECT query in block.module that benefit from it. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:18:07 +0000 : Allie Micka For what it's worth, the benchmarks I submitted did not include the key/index patch, they only included the query consolidation patch. The performance impacts of the patch that hijacked this issue are unknown. I'd suggest moving this issue back to http://drupal.org/node/27157 , where you can assess performance impacts and the back out or correct that patch based on the results. Alternatively, continue using this issue but supply a few benchmarks to justify adding and/or removing the block table indexes. Thanks! ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:56:32 +0000 : m3avrck Just chatted with mathias on IRC. I think there was some slight confusion and I apologize for this. The other issue is related to this issue indeed as it affects the block query and that is why Dries above mentioned to incorporate that fix into this one. Then both of these were rolled into the latest HEAD. However, a small oversight on my part, the key wasn't unique in all situations, and this above patch rectifies this situation. Now, in terms of performance, the key doesn't affect performance in any measurable way. Rather, it adds data integrity and structure to the table, and enhances other *unknown and future* queries. I chatted with a knowledgeable fellow on #mysql a while about this table and it was determined that a key *was* beneficial, even if any performance gains were not. In hindsight, maybe these patches shouldn't have been put together, but as such, they were, and this above patches fixes this issue and brings us back up to speed. So basically this patch "consolidates block queries *and* adds data integrity to the block table". :) ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:19:08 +0000 : I1uv4t4r Attachment: http://drupal.org/files/issues/block.module_6.patch (966 bytes) First I want to say that I am astonished that those patches have made it into CVS. Both the database and block.module patches don't work for me and cause significant errors. Firstly, a primary key of module, delta and theme makes MySQL throw up a 'key too long' error. Secondly, in block.module an array is fetched from the database and then it is used as an object, and the blocks are added to the region the function is called for. You'll understand when you see my patch. I must say that my patch is not tested thouroughly, but I believe it impossible that the patches above are even tested at all, but tell me if I'm wrong. The patch I provide is solely for block.module by the way. ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:59:01 +0000 : m3avrck I1uv4t4r, I welcome your comments. However, this patch was thorougly tested on this end. PHP5.0.5 with MySQL 4.1.11 yields *no* errors at all. Everything works as expected. The reason for the first hiccup was that I only had 1 theme enabled, once I tried 2 themes, I ran into the duplicate key error, which was easily fixed by modifying the key. Running that fine on my machine here. Can you be more specific as to the problems you encountered and which version fo PHP / MySQL you are using? ------------------------------------------------------------------------ Tue, 13 Sep 2005 20:29:17 +0000 : Allie Micka No, I1uv4t4r is right. It would have been helpful if he provided more information about his platform (PHP, MySQL versions, etc.) and less editorializing, but what he says is true and the error is mine. I'm a bit confused by Drupal's inconsistent use of db_fetch_array() versus db_fetch_object(). It seems that the latter approach is more common, so I do it that way myself. But that's no excuse for my missing use of arrays in in block.module! My patch didn't break anything on my 4.3.11 system but it might be a problem for an earlier PHP version. I have tested I1uv4t4r's patch and it doesn't change any behavior on my local system, so I'd consider it "ready to commit". Again, there are two patch threads moving along in this issue. I believe that there are still one or two unresolved issues pertaining to the key/index patch. Please commit block.module_6.patch so we can narrow things down again. Meanwhile, I1uv4t4r - please tell us which version of MySQL (and PHP) you are working with. It may help us troublshoot your other problems. Thanks, and sorry for all the drama! ------------------------------------------------------------------------ Wed, 14 Sep 2005 08:37:21 +0000 : Dries This is somewhat hairy: + $blocks[$block['region']]["$block[module]_$block[delta]"] = (object) $block; Can't that be done in a cleaner fashion? ------------------------------------------------------------------------ Wed, 14 Sep 2005 12:44:36 +0000 : I1uv4t4r I am using PHP 4.4.0 and MySQL 4.1.14. The main problem was that all blocks were put in the region for which the function is called first during a page request, in my case the left region. That has been fixed by my patch. The other problem was that the database server didn't really like a primary key of the three columns /module, delta and theme/: #1071 - Specified key was too long; max key length is 1000 bytes. I don't know how to fix this, the column lenghts do not add up to 1000, so I don't know why MySQL spits out this error. I agree that that line of code is kinda 'hairy' as you call it, you could consider assigning the value of $block['region'] to a variable because it is used more than once. I meant no offence to anybody, by the way, everybody can make mistakes. I just found it weird that it made it into CVS.

Issue status update for http://drupal.org/node/30801 Post a follow up: http://drupal.org/project/comments/add/30801 Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: Allie Micka Updated by: m3avrck Status: patch (code needs work) Attachment: http://drupal.org/files/issues/database_5.patch (1.99 KB) I looked into this issue and found this, http://bugs.mysql.com/bug.php?id=4541 . Turns out to be a bug in MySQL when dealing with MyISAM tables and UTF-8. Also: " If you must use 4.1 (and your PHP installation is properly configured for it!) then make sure the database is created with latin-1 as the default character set, not UTF-8. MySQL 4.1's character set support will otherwise do strange things, including complaining about keys being too long. " Right now because of this, it is in Drupal's best interest *not* put any keys in the block table. Don't want this issue creeping up elsewhere and the performance/data integrity benefits of this key aren't worth the headache for other people. The attached patch fixes this. Please roll this patch into the patch to fix the queries and we can do a clean fix to HEAD. m3avrck Previous comments: ------------------------------------------------------------------------ Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB) The blocks module is running a separate query for each region, e.g.: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='left' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='right' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='header' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='footer' ORDER BY weight, module Ultimately, we're going to need all of these so let's save a few queries. I have changed the query to: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER BY region, weight, module And set all results to the static $blocks array on the first pass. ------------------------------------------------------------------------ Sun, 11 Sep 2005 23:50:53 +0000 : m3avrck Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB) Awesome patch! Noticed the version was set wrong so changed this to CVS. Also, rerolled the patch, there was an extraneous $region in there no longer needed. Reducing 4 queries is definetly a great start in optimizing Drupal :) Take this patch along with this one (might need to be updated now with this query rewrite I'm thinking): http://drupal.org/node/27157 ... and we'll have Drupal's loading of blocks pretty much maxed out with optimizations, then on to the slooow alias ones... ;) ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:24:56 +0000 : Souvent22 Gotta love when something is just logical. Makes sense, and the patch worked. I'd say this is ready. Get these blocks sped up a bit. This coupled with 'block primary key' patch should make the blocks solid. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:00 +0000 : Dries Has this been benchmarked? I would expect this to be faster, but it's good practice nonetheless. Feel free to throw that other block query patch into the mix. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:49:15 +0000 : m3avrck Dries, I ran some simple benchmarks with the devel.module. Queries with a default install were reduced from 125 to 121. Query times averaged around 50ms and this was reduced to roughly 47-48ms. It's not a *huge* savings, but on a a dev machine, with only one user, this is still good nonetheless. I imagine on a hard hit Drupal site this would start to scale up nicely with time/load savings. I'll reroll a patch that combines the two in a few. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:26:08 +0000 : Dries This patch reduces the SQL overhead but adds some CPU-overhead and memory-overhead. Hence, it's better to benchmark the number of requests/second (for example using ab or ab2) ... ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:35:54 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_14.patch (3.31 KB) Ok here is an updated patch, along with the blocks table modification of adding a PRIMARY KEY(module,delta). Spent sometime on #mysql discussing best practice for this table and adding a primary key is the best bet. Any indexes would not improve performance since this table will probably almost never have more than 30 rows in it. The primary key itself is slightly neglible as well, however, if any query were to perform advanced queries on this table, the primary key would benefit, so in best practice, we should have this as well. I'll try to get some benchmarks up soon backing these simple changes. ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:49:34 +0000 : Allie Micka This patch will not increase memory overhead unless a theme has one or more regions that are defined but not in use. Otherwise, the static $blocks array will be identical with or without the patch applied. The patch also does not increase CPU overhead because less code is processed. Without the patch, the entire query and parsing routine is executed 5 or more times (everything between if (!isset($blocks[$region])) { } ). With the patch, the same code executes only once. But the proof is in the pudding. I'm not sure what your standards are for ab, but I like to do 1 serial check and 1 check with concurrency so I can rule out performance gained by increased memory consumption, which bogs down concurrent requests. This patch appears to save an average of 9-12ms per request and increase performance by .05 requests per second. The relevant bits from my ab results are as follows: Before Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.32 [#/sec] (mean) Time per request: 430.20 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.16 [#/sec] (mean) Time per request: 4631.00 [ms] (mean) Time per request: 463.10 [ms] (mean, across all concurrent requests) After Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.37 [#/sec] (mean) Time per request: 421.08 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.22 [#/sec] (mean) Time per request: 4510.90 [ms] (mean) Time per request: 451.09 [ms] (mean, across all concurrent requests) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:04:27 +0000 : m3avrck Exactly running similar to what I was uncovering as well. So seems this patch is ready to go, time to commit :) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:27:32 +0000 : Dries Thanks for the measurements! Patch committed to HEAD. Thanks. ------------------------------------------------------------------------ Tue, 13 Sep 2005 07:37:26 +0000 : asimmonds With this patch committed, and updating my HEAD install with update.php, I get the error "Duplicate entry 'user-0' for key 1" from mysql. I have two themes configured and there is a primary key (module,delta) clash between the two themes. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:39:43 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_3.patch (1.96 KB) Eeek!!! Forgot to test with multiple themes, this patches fixes the key definition. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:41:46 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_4.patch (1.96 KB) Noticed a typo in the PGSQL. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:00:19 +0000 : mathias I can recreate this bug when running update.php but I question the necessity of this index in the first place. I couldn't find one SELECT query in block.module that benefit from it. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:18:07 +0000 : Allie Micka For what it's worth, the benchmarks I submitted did not include the key/index patch, they only included the query consolidation patch. The performance impacts of the patch that hijacked this issue are unknown. I'd suggest moving this issue back to http://drupal.org/node/27157 , where you can assess performance impacts and the back out or correct that patch based on the results. Alternatively, continue using this issue but supply a few benchmarks to justify adding and/or removing the block table indexes. Thanks! ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:56:32 +0000 : m3avrck Just chatted with mathias on IRC. I think there was some slight confusion and I apologize for this. The other issue is related to this issue indeed as it affects the block query and that is why Dries above mentioned to incorporate that fix into this one. Then both of these were rolled into the latest HEAD. However, a small oversight on my part, the key wasn't unique in all situations, and this above patch rectifies this situation. Now, in terms of performance, the key doesn't affect performance in any measurable way. Rather, it adds data integrity and structure to the table, and enhances other *unknown and future* queries. I chatted with a knowledgeable fellow on #mysql a while about this table and it was determined that a key *was* beneficial, even if any performance gains were not. In hindsight, maybe these patches shouldn't have been put together, but as such, they were, and this above patches fixes this issue and brings us back up to speed. So basically this patch "consolidates block queries *and* adds data integrity to the block table". :) ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:19:08 +0000 : I1uv4t4r Attachment: http://drupal.org/files/issues/block.module_6.patch (966 bytes) First I want to say that I am astonished that those patches have made it into CVS. Both the database and block.module patches don't work for me and cause significant errors. Firstly, a primary key of module, delta and theme makes MySQL throw up a 'key too long' error. Secondly, in block.module an array is fetched from the database and then it is used as an object, and the blocks are added to the region the function is called for. You'll understand when you see my patch. I must say that my patch is not tested thouroughly, but I believe it impossible that the patches above are even tested at all, but tell me if I'm wrong. The patch I provide is solely for block.module by the way. ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:59:01 +0000 : m3avrck I1uv4t4r, I welcome your comments. However, this patch was thorougly tested on this end. PHP5.0.5 with MySQL 4.1.11 yields *no* errors at all. Everything works as expected. The reason for the first hiccup was that I only had 1 theme enabled, once I tried 2 themes, I ran into the duplicate key error, which was easily fixed by modifying the key. Running that fine on my machine here. Can you be more specific as to the problems you encountered and which version fo PHP / MySQL you are using? ------------------------------------------------------------------------ Tue, 13 Sep 2005 20:29:17 +0000 : Allie Micka No, I1uv4t4r is right. It would have been helpful if he provided more information about his platform (PHP, MySQL versions, etc.) and less editorializing, but what he says is true and the error is mine. I'm a bit confused by Drupal's inconsistent use of db_fetch_array() versus db_fetch_object(). It seems that the latter approach is more common, so I do it that way myself. But that's no excuse for my missing use of arrays in in block.module! My patch didn't break anything on my 4.3.11 system but it might be a problem for an earlier PHP version. I have tested I1uv4t4r's patch and it doesn't change any behavior on my local system, so I'd consider it "ready to commit". Again, there are two patch threads moving along in this issue. I believe that there are still one or two unresolved issues pertaining to the key/index patch. Please commit block.module_6.patch so we can narrow things down again. Meanwhile, I1uv4t4r - please tell us which version of MySQL (and PHP) you are working with. It may help us troublshoot your other problems. Thanks, and sorry for all the drama! ------------------------------------------------------------------------ Wed, 14 Sep 2005 08:37:21 +0000 : Dries This is somewhat hairy: + $blocks[$block['region']]["$block[module]_$block[delta]"] = (object) $block; Can't that be done in a cleaner fashion? ------------------------------------------------------------------------ Wed, 14 Sep 2005 12:44:36 +0000 : I1uv4t4r I am using PHP 4.4.0 and MySQL 4.1.14. The main problem was that all blocks were put in the region for which the function is called first during a page request, in my case the left region. That has been fixed by my patch. The other problem was that the database server didn't really like a primary key of the three columns /module, delta and theme/: #1071 - Specified key was too long; max key length is 1000 bytes. I don't know how to fix this, the column lenghts do not add up to 1000, so I don't know why MySQL spits out this error. I agree that that line of code is kinda 'hairy' as you call it, you could consider assigning the value of $block['region'] to a variable because it is used more than once. I meant no offence to anybody, by the way, everybody can make mistakes. I just found it weird that it made it into CVS. ------------------------------------------------------------------------ Wed, 14 Sep 2005 13:02:14 +0000 : I1uv4t4r Attachment: http://drupal.org/files/issues/block.module_7.patch (968 bytes) Here's an updated patch which decreases the 'hairyness'. ------------------------------------------------------------------------ Wed, 14 Sep 2005 14:22:34 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block.module_8.patch (3.29 KB) Actually, the root of all that hair is in the db_fetch_array() call. This function is expected to return an array of objects, so why not select it as such and keep the type consistent? I have even heard stories about this inconsistency causing new Drupal developers to introduce bugs with their patches ;) There are only two other references to db_fetch_array() in block.module - one for an $edit array internal to its function, and the other for boxes. So this patch replaces the call and uses attributes consistently with all block-returning code. I have tested this patch with page specific settings, user preference settings and by moving blocks to different regions and with different weights. I have NOT tested this with throttling. I have also not tested this on I1uv4t4r's machine, which seems necessary somehow. Following this issue, we still have - address m3avrck's multiple themes patch and I1uv4t4r's key length concerns - revise phpdoc for block_list() to update/remove todo item for the database key

Issue status update for http://drupal.org/node/30801 Post a follow up: http://drupal.org/project/comments/add/30801 Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: Allie Micka Updated by: m3avrck -Status: patch (code needs work) +Status: patch (ready to be committed) Dries, please patch comment #22 and #23. This will fix the block queries and will remove the key to prevent any other users from receiving the error 'key index too long'. That is a tricky bug and that primary key() benefits don't away the bug issues. m3avrck Previous comments: ------------------------------------------------------------------------ Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB) The blocks module is running a separate query for each region, e.g.: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='left' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='right' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='header' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='footer' ORDER BY weight, module Ultimately, we're going to need all of these so let's save a few queries. I have changed the query to: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER BY region, weight, module And set all results to the static $blocks array on the first pass. ------------------------------------------------------------------------ Sun, 11 Sep 2005 23:50:53 +0000 : m3avrck Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB) Awesome patch! Noticed the version was set wrong so changed this to CVS. Also, rerolled the patch, there was an extraneous $region in there no longer needed. Reducing 4 queries is definetly a great start in optimizing Drupal :) Take this patch along with this one (might need to be updated now with this query rewrite I'm thinking): http://drupal.org/node/27157 ... and we'll have Drupal's loading of blocks pretty much maxed out with optimizations, then on to the slooow alias ones... ;) ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:24:56 +0000 : Souvent22 Gotta love when something is just logical. Makes sense, and the patch worked. I'd say this is ready. Get these blocks sped up a bit. This coupled with 'block primary key' patch should make the blocks solid. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:00 +0000 : Dries Has this been benchmarked? I would expect this to be faster, but it's good practice nonetheless. Feel free to throw that other block query patch into the mix. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:49:15 +0000 : m3avrck Dries, I ran some simple benchmarks with the devel.module. Queries with a default install were reduced from 125 to 121. Query times averaged around 50ms and this was reduced to roughly 47-48ms. It's not a *huge* savings, but on a a dev machine, with only one user, this is still good nonetheless. I imagine on a hard hit Drupal site this would start to scale up nicely with time/load savings. I'll reroll a patch that combines the two in a few. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:26:08 +0000 : Dries This patch reduces the SQL overhead but adds some CPU-overhead and memory-overhead. Hence, it's better to benchmark the number of requests/second (for example using ab or ab2) ... ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:35:54 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_14.patch (3.31 KB) Ok here is an updated patch, along with the blocks table modification of adding a PRIMARY KEY(module,delta). Spent sometime on #mysql discussing best practice for this table and adding a primary key is the best bet. Any indexes would not improve performance since this table will probably almost never have more than 30 rows in it. The primary key itself is slightly neglible as well, however, if any query were to perform advanced queries on this table, the primary key would benefit, so in best practice, we should have this as well. I'll try to get some benchmarks up soon backing these simple changes. ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:49:34 +0000 : Allie Micka This patch will not increase memory overhead unless a theme has one or more regions that are defined but not in use. Otherwise, the static $blocks array will be identical with or without the patch applied. The patch also does not increase CPU overhead because less code is processed. Without the patch, the entire query and parsing routine is executed 5 or more times (everything between if (!isset($blocks[$region])) { } ). With the patch, the same code executes only once. But the proof is in the pudding. I'm not sure what your standards are for ab, but I like to do 1 serial check and 1 check with concurrency so I can rule out performance gained by increased memory consumption, which bogs down concurrent requests. This patch appears to save an average of 9-12ms per request and increase performance by .05 requests per second. The relevant bits from my ab results are as follows: Before Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.32 [#/sec] (mean) Time per request: 430.20 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.16 [#/sec] (mean) Time per request: 4631.00 [ms] (mean) Time per request: 463.10 [ms] (mean, across all concurrent requests) After Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.37 [#/sec] (mean) Time per request: 421.08 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.22 [#/sec] (mean) Time per request: 4510.90 [ms] (mean) Time per request: 451.09 [ms] (mean, across all concurrent requests) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:04:27 +0000 : m3avrck Exactly running similar to what I was uncovering as well. So seems this patch is ready to go, time to commit :) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:27:32 +0000 : Dries Thanks for the measurements! Patch committed to HEAD. Thanks. ------------------------------------------------------------------------ Tue, 13 Sep 2005 07:37:26 +0000 : asimmonds With this patch committed, and updating my HEAD install with update.php, I get the error "Duplicate entry 'user-0' for key 1" from mysql. I have two themes configured and there is a primary key (module,delta) clash between the two themes. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:39:43 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_3.patch (1.96 KB) Eeek!!! Forgot to test with multiple themes, this patches fixes the key definition. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:41:46 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_4.patch (1.96 KB) Noticed a typo in the PGSQL. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:00:19 +0000 : mathias I can recreate this bug when running update.php but I question the necessity of this index in the first place. I couldn't find one SELECT query in block.module that benefit from it. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:18:07 +0000 : Allie Micka For what it's worth, the benchmarks I submitted did not include the key/index patch, they only included the query consolidation patch. The performance impacts of the patch that hijacked this issue are unknown. I'd suggest moving this issue back to http://drupal.org/node/27157 , where you can assess performance impacts and the back out or correct that patch based on the results. Alternatively, continue using this issue but supply a few benchmarks to justify adding and/or removing the block table indexes. Thanks! ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:56:32 +0000 : m3avrck Just chatted with mathias on IRC. I think there was some slight confusion and I apologize for this. The other issue is related to this issue indeed as it affects the block query and that is why Dries above mentioned to incorporate that fix into this one. Then both of these were rolled into the latest HEAD. However, a small oversight on my part, the key wasn't unique in all situations, and this above patch rectifies this situation. Now, in terms of performance, the key doesn't affect performance in any measurable way. Rather, it adds data integrity and structure to the table, and enhances other *unknown and future* queries. I chatted with a knowledgeable fellow on #mysql a while about this table and it was determined that a key *was* beneficial, even if any performance gains were not. In hindsight, maybe these patches shouldn't have been put together, but as such, they were, and this above patches fixes this issue and brings us back up to speed. So basically this patch "consolidates block queries *and* adds data integrity to the block table". :) ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:19:08 +0000 : I1uv4t4r Attachment: http://drupal.org/files/issues/block.module_6.patch (966 bytes) First I want to say that I am astonished that those patches have made it into CVS. Both the database and block.module patches don't work for me and cause significant errors. Firstly, a primary key of module, delta and theme makes MySQL throw up a 'key too long' error. Secondly, in block.module an array is fetched from the database and then it is used as an object, and the blocks are added to the region the function is called for. You'll understand when you see my patch. I must say that my patch is not tested thouroughly, but I believe it impossible that the patches above are even tested at all, but tell me if I'm wrong. The patch I provide is solely for block.module by the way. ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:59:01 +0000 : m3avrck I1uv4t4r, I welcome your comments. However, this patch was thorougly tested on this end. PHP5.0.5 with MySQL 4.1.11 yields *no* errors at all. Everything works as expected. The reason for the first hiccup was that I only had 1 theme enabled, once I tried 2 themes, I ran into the duplicate key error, which was easily fixed by modifying the key. Running that fine on my machine here. Can you be more specific as to the problems you encountered and which version fo PHP / MySQL you are using? ------------------------------------------------------------------------ Tue, 13 Sep 2005 20:29:17 +0000 : Allie Micka No, I1uv4t4r is right. It would have been helpful if he provided more information about his platform (PHP, MySQL versions, etc.) and less editorializing, but what he says is true and the error is mine. I'm a bit confused by Drupal's inconsistent use of db_fetch_array() versus db_fetch_object(). It seems that the latter approach is more common, so I do it that way myself. But that's no excuse for my missing use of arrays in in block.module! My patch didn't break anything on my 4.3.11 system but it might be a problem for an earlier PHP version. I have tested I1uv4t4r's patch and it doesn't change any behavior on my local system, so I'd consider it "ready to commit". Again, there are two patch threads moving along in this issue. I believe that there are still one or two unresolved issues pertaining to the key/index patch. Please commit block.module_6.patch so we can narrow things down again. Meanwhile, I1uv4t4r - please tell us which version of MySQL (and PHP) you are working with. It may help us troublshoot your other problems. Thanks, and sorry for all the drama! ------------------------------------------------------------------------ Wed, 14 Sep 2005 08:37:21 +0000 : Dries This is somewhat hairy: + $blocks[$block['region']]["$block[module]_$block[delta]"] = (object) $block; Can't that be done in a cleaner fashion? ------------------------------------------------------------------------ Wed, 14 Sep 2005 12:44:36 +0000 : I1uv4t4r I am using PHP 4.4.0 and MySQL 4.1.14. The main problem was that all blocks were put in the region for which the function is called first during a page request, in my case the left region. That has been fixed by my patch. The other problem was that the database server didn't really like a primary key of the three columns /module, delta and theme/: #1071 - Specified key was too long; max key length is 1000 bytes. I don't know how to fix this, the column lenghts do not add up to 1000, so I don't know why MySQL spits out this error. I agree that that line of code is kinda 'hairy' as you call it, you could consider assigning the value of $block['region'] to a variable because it is used more than once. I meant no offence to anybody, by the way, everybody can make mistakes. I just found it weird that it made it into CVS. ------------------------------------------------------------------------ Wed, 14 Sep 2005 13:02:14 +0000 : I1uv4t4r Attachment: http://drupal.org/files/issues/block.module_7.patch (968 bytes) Here's an updated patch which decreases the 'hairyness'. ------------------------------------------------------------------------ Wed, 14 Sep 2005 14:22:34 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block.module_8.patch (3.29 KB) Actually, the root of all that hair is in the db_fetch_array() call. This function is expected to return an array of objects, so why not select it as such and keep the type consistent? I have even heard stories about this inconsistency causing new Drupal developers to introduce bugs with their patches ;) There are only two other references to db_fetch_array() in block.module - one for an $edit array internal to its function, and the other for boxes. So this patch replaces the call and uses attributes consistently with all block-returning code. I have tested this patch with page specific settings, user preference settings and by moving blocks to different regions and with different weights. I have NOT tested this with throttling. I have also not tested this on I1uv4t4r's machine, which seems necessary somehow. Following this issue, we still have - address m3avrck's multiple themes patch and I1uv4t4r's key length concerns - revise phpdoc for block_list() to update/remove todo item for the database key ------------------------------------------------------------------------ Wed, 14 Sep 2005 14:36:58 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_5.patch (1.99 KB) I looked into this issue and found this, http://bugs.mysql.com/bug.php?id=4541 . Turns out to be a bug in MySQL when dealing with MyISAM tables and UTF-8. Also: " If you must use 4.1 (and your PHP installation is properly configured for it!) then make sure the database is created with latin-1 as the default character set, not UTF-8. MySQL 4.1's character set support will otherwise do strange things, including complaining about keys being too long. " Right now because of this, it is in Drupal's best interest *not* put any keys in the block table. Don't want this issue creeping up elsewhere and the performance/data integrity benefits of this key aren't worth the headache for other people. The attached patch fixes this. Please roll this patch into the patch to fix the queries and we can do a clean fix to HEAD. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:23:19 +0000 : I1uv4t4r Well, the patch (block.module_8.patch) works for me! Ready to be committed I'd say.