use 'cache_geonames' table by default
bangpound - April 15, 2009 - 05:41
| Project: | GeoNames |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | bangpound |
| Status: | closed |
Description
The module now respects cache really well and Geonames data is well-suited for long-term caching, but Drupal (especially during development) often clears the whole cache. Are there hidden costs to creating a new cache table? This can remain configurable, but I think it's very easy to add a cache_geonames table to the existing schema.
$schema['cache_geonames'] = drupal_get_schema_unprocessed('system', 'cache');
#1
this is definitely a good idea.
#2
Here's a patch. For users who have already set up and changed their cache table from the default, that cache table will still be used. For everyone else, the new cache_geonames table is used by default.
Users need to re-install the module for this cache table to be created, unless you think I should make a hook_update_N that does this.
#3
#4
Yes, I think the module should be able to create the table on the fly, too.
#5
What do you mean by on the fly? The patch creates the cache table when the module is installed. The system settings form no longer exposes the cache table variable to the user. If the user wants to use another cache table, he's going to have to create the cache table himself like he always had to.
#6
I mean during module upgrade (update.php - hook_update_N etc). Requiring the user to uninstall and reinstall is unnecessary, for the couple of lines the update would take.
#7
Thanks for clarifying. I'll reroll the patch today with a hook_update_N in it.
#8
New patch! only 12 days late.
#9
Nooooo...
<?php+ drupal_uninstall_schema('geonames');
+ drupal_install_schema('geonames');
?>
Please just create the new table explicitly, without uninstalling and reinstalling the schema (which will loose all the data loaded at upgrade time). AFAIK, you can-not/should-not call these methods from a hook_update.
http://api.drupal.org/api/function/hook_update_N/6
#10
The schema is already different due to the patch #334726: Format of countryInfo.txt has changed, breaking geonames cache which contained no hook_update_N for those schema changes.
All the tables in the schema for Geonames are essentially cache tables. They're regularly rebuilt. The advice not to use hook_schema (and by association drupal_install_schema) in hook_update_N comes from the assumption that the schema's tables contain unique data you need to keep for your site to function. That assumption doesn't apply to Geonames.module.
I can write a hook_update_N to bring the tables up to date, but it's a lot of tedious work to preserve data that gets replaced on a monthly basis anyway. Are you sure it's justified in this case?
#11
1) #334726 should not have been committed without hook_update_n(). That's my fault, and I should have caught it at the time. I'll write that when I get a chance.
2) I don't think we should be taking liberties with deleting users' data (be it cache or otherwise), and it's kindof a bad precedent to be setting. Consider the performance impact on production site with tens thousands of cached entries.. In this particular case (changing cache table) we bust the cache anyway, but I'm talking about generally.
Uuugh, CVS is a mess right now - two diverging branches, and no real releases in quite some time.
Eg: #338771: Add support for loading Geonames Datafiles into local DB - not just a cache.
#12
Already done in #576112: Create and use a separate cache table by default (though I forgot about this ticket, and reinvented the code)
#13
Automatically closed -- issue fixed for 2 weeks with no activity.