rename node_(typename) tables to node_type_(typename)

Why not "node_data_type_page", "node_data_type_example"
This would play well IMO with the "node_data_field_myfield" pattern used by "per field" storage.

Or maybe (while we're at it) move from "node_data_" to "node_content_" for both field and type tables, if we want to make the name of the "content" module stand out more.
I think we would benefit harmonizing tablenames for "per fields" and "per content types" storage tables anyway.

I'm working on delegating the table names generation in a small helper function, that would stop these sort of things being scattered all around the code. Plus this gives us time to debate about naming conventions :-)

Deal :-)
patch coming soon...

Here is a first step :
This patch uses a _content_tablename helper function to generate table names for fields and node types, and moves the rest of code to use it.
The current naming conventions are not altered for now, I think we should keep that for a second step, once we're sure that everything works fine.

I tried to separate the cases where :
- the table name is actually generated (content type creation or update) : the new _content_tablename generates the name
- the table name is only "used" (to perform queries, etc...) : a new key 'table' in the content type description (content_types() function) is added, and holds the table name generated for this content type.

There remains one or two spots where I'm uncertain - I added comments for that in the patch

Thanks for the validation / correction.
I committed a version including your edits - there are two places where I kept my original code :
- content_crud.inc / content_type_create() :
no need for content_clear_type_cache(); being called twice
- content_views.inc / content_views_field_handler_group()
the 'node_data_' strings that remain are the only db alias for the table - I'm not quite sure what to do with them, but I don't think they _need_ to be changed (plus I think this might alter css id's and break themes.

Do you think this should go in 4.7 as well ? (not required, but would be easier for us to follow...)

Second step : rename :-) (+ update function... I think we'll have to regenarate all the stored queries for the views involving cck fields...)

Here is the patch that actually does the renaming (5.0 for now) :
- content_field_myfield for "per field" tables
- content_type_mytype for "per content type" tables
general tables are also renamed (+ plural form to avoid clashes with a possible content type named "instances"...)
- content_fields
- content_fields_instances
- content_groups
- content_groups_fields

Attached patch provides update functions for content.install and fieldgroup.install,
and (should) take care of regenerating affected views (was the hardest part, actually...)

This is brain surgery, so of course this needs testing - PLEASE backup your db before applying :-)

PS : a few "remove trailing white spaces" lines might have sneaked in the patch, making it (just a little) bigger than it should be.

Karen, IMO there is little risk of breaking other contribs - be it contrib cck field module or 'plain' contrib module. As Jeff mentioned, external code is not likely to directly query cck data tables as they are 'constantly' moving by nature (until recent commit in http://drupal.org/node/104259, you could not even be sure there was a 'node_mytype' table for each type field).

This argument is less true for 'fixed' tables (node_field, node_field_instance), but I can't think of any cck-related module that would have a reason to query those directly instead of getting the info from API function content_types().
Fieldgroup does, but being now in ckk-core it is handled by the patch, and I think we'd be aware of other fieldgroup-like modules out there.

This being said, my previous 'helper tablename function' commit _might_ have broken some (internal) stuff, there were two bugs posted today that could be related - I need to investigate those.

update : the two bugs I mentioned (http://drupal.org/node/106926 and http://drupal.org/node/106900) are not related to the 'helper tablename' refactoring.

New version of the patch rerolled after today's cleanup commits

Rerolled patch against latest 5.0
I think we should get this going pretty fast now.
As was recently pointed in http://drupal.org/node/110371 (marked a dupe of this), this is not really serious when you try to create a node with a conflicting machine name, but hell might open when you _change_ an existing type and set its name to a conflicting table. Having cck columns added to, say, node_access would be... annoying.

FWIW, my test cck install has been running this patch for 10 days now without any problem.
Plus, as I wrote earlier, the massive changes went in the previous 'helper function' patch that is already in.

Er, wait. Thinking about it twice, this might be much more complicated because of upgrades.
Upgrade paths for cck + field modules are currently a mess to predict, and I'm having a hard time trying to figure out all the possible cases.

And yet is seems we _have_ to move...

Here are a few ideas for a better future.
- Gather in content.install all db scheme updates for fields that defer their db storage to content.module (that is all currently known field modules).
That includes contrib fields as well, of course.
Currently we have number_update_4, for instance, that updates from ancient 'per field type' to 'per content type' storage

- Ensure that content updates get run before any update for field modules - things are too unpredictable without this.
Updates are ordered by module weight, then by file path.
Weight is used for a lot of other things so it's not really flexible...
Maybe we should force all field modules to be stored in a '/fields' fiolder inside the cck folder ?
Not really install-friendly.

By prefixing all these tables with 'content_' we have another set of possible conflicts
Actually, no : the patch uses 'content_type_*' and 'content_field_*' prefix, so there is no way this could conflict with hardcoded 'content_groups*' and 'content_fields*' tables.

only rename node_[content type] to content_[content type]
Yeaah, well. This leaves the db in a 'not-very-clean' state, with some tables in the 'content_' area, and others in the 'node_' area. Really inconvenient when you browse your db in phpMyAdmin, or try to debug a Views query (this concerns us maintainers, primarily :-) ). Later on we'll want to change that eventually and embark in another renaming update.
I really wish we could do that once and for all. But we have a sort of emergency here, so maybe you're right.
I propose allow ourselves 1 or 2 more days to think about it ?

OK - I'll submit a new -limited- patch later today for release 1.3.

I propose we "clean the slate" after that, meaning in the next release (1.4 ?) we do not handle upgrade paths for versions <= 1.2. (users from older versions have to update to 1.3 first). How does this sound ?

Here's a trimmed down version : only renames content type tables from 'node_foo' to 'content_type_foo', and should also update views accordingly.
I have to say that in spite of the more limited scope, and after another two hours on the question, I still can't see a way to ensure that the upgrade paths will go smoothly for older 4.7 branches.

Not sure we should do this in two steps and "draw an line" after an intermediate 1.3, or draw the line now, tell 4.7 users to update to 4.7--1.2 and go faster to a nice and consistent db state in 1.3
Well, I guess I'm too tired, my ideas on this right now are kind of blurred.

And I forgot the patch :-)

OK, here's my proposition on this :

Attached patch adds some versioning 'logic' into the _content_tablename function (the helper function that gives the name of dynamic 'per field' and 'per type' tables).
This provides more safety for cck API functions that are called in update functions in content.install and field modules .install : whenever an api function does operations on the tables, it operates on the tables using the correct naming convention (pre or post renaming)

This should eliminate some of the 'irrationalities' and (hopefully) provide for smoother upgrade paths - so this patch goes a little further than my previous 'minimal renaming' one, and moves both 'per type' *and* 'per fields' tables to the 'content_' namespace. It definitely feels too weird having 'per type' on 'content_type_' while 'per field' is still on 'node_field_'.

The patch leaves 'static' tables (node_field, node_field_instance, node_group, node_group_instance) where they are. If we want later on (when we draw that line...), we can move them with the others.

Waiting for some approval before committing :-)

PS : thanks for the reminder on hook_enable et al - I can't say I was really aware they were here.

It did not seem as they could be of immediate help on our current issue, but it made me think that maybe we could use hook_enable to perform content_types_rebuild, instead of currently on each module overview submit ?
I opened http://drupal.org/node/112225 about that.

I have made a test install with a pre-'per field' version of cck (that's march 26, 2006), and I am currently testing the upgrade process...
Should be useful whichever way we go.

This raised another problem : the 'display settings' column added in node_field_instance by the display settings patch triggers errors in older content.install updates (content_update_5 calls content_fields et al) and breaks the upgrade path.

Well, I'd like to move faster on this too and move on to more interesting (and less tedious...) stuff, but it seems we have to fix this as well :-/

When I talked about moving ASAP I wasn't suggesting you aren't moving fast enough (...)
Of course, I know - sorry about that. I guess this stuff starts to get on me :-)

unset the display field in the result that comes back from content_fields()
Unfortunately no, because the error is raised in _content_type_info, that is called in content_fields().
$field_result = db_query("SELECT nfi.field_name, nfi.weight, nfi.label, nfi.widget_type, nfi.widget_settings, nfi.display_settings, nfi.description FROM {node_field_instance} nfi WHERE nfi.type_name = '%s' ORDER BY nfi.weight ASC, nfi.label ASC", $type['type']);

I guess either we move the display settings back in the 'widget_settings' column as it was mentioned at one point (although I agree that these ideally deserve a column of their own), or we manage to conditionally load the column somehow ?

What can I do to help at this point?
Well, I'm currently trying to run the update process through a PHP debugger to understand what's going on. Tricky for now, and I will have to stop for today shortly.
I guess I could send you a copy of my install and db dump (it's a 4.7 + 'old cck' db upgraded to 5.0 core, so from there I just have to roll cck 5.0 updates, see errors, drop and re-import the db, roll cck 5.0 updates, etc...). But it's not nice stuff... No obligation, really :-)

Smart :-) That should do the trick indeed...

Er, well apart from the 'display setting' issue, I found something... annoying..., which probably explains much of the upgrade issues we've seen here and there.
_content_type_info, which builds the cached 'content_type_info' array that gathers all the info types and fields, is broken in updates :
it builds types info using something like

 $type_result = db_query('SELECT * FROM {node_type} ORDER BY type ASC');
      while ($type = db_fetch_array($type_result)) {
        ... build $type array...
      }

So when this is called during contribs update process before our friend content_update_1001 has copied 'old' cck types info into the new core {node_type} table, {node_type} only contains default 'page' and 'story', and thus _content_type_info only has no type info for 'old cck' types, and content_update_5, for instance, is screwed.

I possibly have made things worse during my 'helper tablename function' patch, since I made the choice to generate the tablename once and store it in $type['table'], instead of calling _content_tablename every time we need the table name...
I can try to revert that behaviour (while keeping _content_tablename, i think it's precious right now) and see if it makes things better.
But we may also think again about our upgrade policy... wouldn't we be better off telling 4.7 users 'update to latest 4.7 cck first' ?

And even once all that is worked out, what happens if they do the 5.0 update before we have a chance to throw up an error message?
Yes, that's the meanest thing IMO - people will be upgrading core to 5.0, and then find out that we ask them to update their 4.7 cck...

Well, I've made some progress on debugging - the only thing that still have issues (with data loss) are ... per content type fields (the ones we do want to move) ;-)

OK, after quite some debugging and db restoring, I think this patch manages to provide a smooth upgrade path from older 4.7 version (starting from antedeluvian 'per field *type*' storage) to new naming convention with 'per field' and 'per content type' tables moved to 'content_'. No warnings, no data loss, correct db scheme at the end.

That includes :
- various fixes in content_update_4/5 and (field module)_update_4 functions,
- an additional check in the part of content_alter_db_field that manages data migration from one storage type to the other
- reverting the 'per type' tablename being stored (and cached) in $type array (I added that in http://drupal.org/cvs?commit=49450)

AAMOF, the 'per field' tables precisely were the ones that did not raise errors or warnings to begin with, so I really think it makes no sense to leave them where they are.
Moving node_field and node_field_instance tables, like I primarily intended to, is, er, absolutely out of question...

Additional tests with 'less antique' 4.7 versions might be done, but I think the current state of the patch is quite close to filling the amount of time I'm willing to spend on this :-), so I think this can be committed.

PS : considering what I found in the process, i'm not sure the pure 4.7 (antique 4.7 to newest 4.7) upgrade is flawless either...
Not willing go deeper tonight...

I had just been worried that it was the per field tables that were going to create update problems.
I do understand that. It would really have bugged me, but I would have resigned myself to this as the most reasonable thing to do if I they proved to be troublesome. I'm glad they don't :-)

I'm not sure we have to rename stuff in 4..7. There is no real need as there is for 5.0, so if we folks can deal with different names while doing debugging / support managing in the issue queue, we can leave it as is.
Besides, renaming 4.7 tables also means dealing with the fact that during an upgrade in 5.0, the tables might or might not have been renamed in the 4.7 era (exact same kind of issue than the one 'display_settings' column you're pointing) - I wish we could avoid that.
What I'm wondering is whether the internal stuff (helper table naming function) should be backported (without actually doing any renaming). As I wrote earlier, the upgrade process from old 4.7 to new 4.7 seems buggy as well, and this update function provides some 'security'.

About the display_settings column : that's quite true. I'm not sure how to have an equivalent of db_table_exists for a column (MySQL + PgSQL)... Maybe we could jut add a @ in front of the line that adds the column ?

update 1003 : right.

Great idea about the post on g.d.o - writing a clean and well phrased public announcement in english always takes me some time, so I'd rather leave it to more adequate people :-)
I'll commit the renaming stuff for 5.0 ASAP. Maybe we can add a request for testing in the g.d.o thread before we release, or do you want to release straight ahead ?

committed to 5.0 and HEAD :-)

I agree that we cannot expect too much validation on upgrade process.
But the patch also connects to cck 'neuro system', so I'd be more confident if we could get people to use it, even on plain 5.0 day to day use.

even if that's the right classification, 'to be ported' hides the issue in default issue queue, so i'm setting back to 'needs work'