Extremely Poor Performance with CCK fields passing data to database

Great idea. Very interested. Might have a conflict with another module I'm running? Or because it's 5.x.1.6-1 ?

Received this upon submitting a fairly generic cck node after applying this patch.

    * warning: implode() [function.implode]: Bad arguments. in modules/cck/content.module on line 565.
    * user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'FROM content_type_projectdiscussion WHERE vid = 272 LIMIT 0, 1' at line 1 query: SELECT FROM content_type_projectdiscussion WHERE vid = 272 LIMIT 0, 1 in includes/database.mysql.inc on line 172.

Relevant to this particular content type and instance, I'm using comment_upload and have a node_reference field and just before this patch, applied the cck_panels.patch (which is fantastic! and fantastically timed, too!)

Other cck related modules installed comment_cck and cck_field_perms used with other contetn types but not used with this slighter one. No errors prior to adding the new content.

Great work, Gordon - here are a few remarks from a cursory look at the patch :

- The patch replaces content_field()'s 'load' / 'insert' / ... ops, so it seems it should also remove the current ones ?
Besides, I'd rather have this structured like current CCK D6, with _content_field_invoke_default() left untouched, only calling a new content_storage() function which does the storage-related ops.
D5 version content_field() would then be left with no actual ops in it, but this would keep the structure consistent with D6 (and D4.7), and make maintaining branches easier on us.

- CCK D6 pre-computes and caches some of the needed information (which fields in which tables...) in _content_type_info(). Not sure how this maps to your patch, but this might be worth investigating.

- CCK allows field modules to opt-out of default db storage, and manage it themselves in their own implementations of hook_field('load' / 'insert' / ...) - I think Eaton's Amazon field is one of those. The convention for this is fields that declare no columns ($db_info['columns'] is empty) opt out of CCK's storage. Could you ensure the patch doesn't break this ? (maybe it's OK as is, just want to make sure)

I'll be (mainly) AFK for the next 10 days or so. I intend to make a 5.x-1.7 shortly after my return (1.6 is about 8 months old, we've waited long enough).
It would be great to include this patch, but since it impacts the most critical part of CCK, it needs to be 200% tested and approved (and the absence of field CRUD API in the D5 branch means we don't have simpletests...). So releasing 1.7 without it is also OK by me - this patch on itself would legitimate a 1.8 release as soon as it's ready. We need to release more often anyway :-)

subscribing

Noting here as I did in my email to you, Gordon:

The error I received, while referencing changes from this patch, may also be due to conflict with other modules I forgot to mention before (og_user_roles, comment_cck, or even that one I'm not really using - og_content_type_admin). When I get the chance, I'll try it out again w/o these and see what happens.

Oh, and the error didn't prevent any operations, it was just reported upon submission of a new or edited form.

.. and did I mention my great appreciation for that cck_panels.patch? I think I did, but it's worth mentioning again.

If one attempts to insert a node that does not contain values for all fields, it will fail to insert the data—I noticed this when my nodes were being created, but my actual data wasn't inserted.

The code in the original patch adds columns to the query regardless of whether the node has values for said columns, but only adds data for those columns if the corresponding fields exist in the node. Hence, you'll end up with twelve columns and twelve format directives after VALUES, but only six values given as arguments to the query.

I have fixed this by only adding the column to the query if the node has that field; if I am wrong, however, in doing this, then of course feel free to correct me.

subscribing

Subscribing.

subscribing

subscribing

@gordon: FYI: Project issue numbers (ex. #12345: How do I log in as admin?) turn into links automatically.

Subscribing.

subscribe

@gordon, I'm trying to sort out which patches I need for this. The patch on this page seems to duplicate some of the ones you linked to. Do I still need this patch, or just the ones in #10?

Also, the patches are huge complicated beasts, hard to evaluate :( but I agree it would be great to have more consistency between CCK 5 and CCK 6 to make it easier to maintain :) and it would be really great to get tests into CCK 5 :), so a +1 from me on looking at this.

My thought right now, if I can sort out the answer to my previous question, is to go ahead and get this all applied in the dev release so others can help test it, and the simple tests should also help ensure we're not breaking things. One of the reasons for releasing a new official release of CCK last week was so we have a stable version with all the previous fixes before we start messing with this one.

subscribing

subscribe

I applied patch 9 an got the following errors after submitting (or updating) content:

user warning: Column 'field_website_url' cannot be null query: INSERT INTO flagey_content_field_website (field_website_url,field_website_title,field_website_attributes,nid,vid,delta) VALUES (NULL, '', 'N;', 124, 124, 0) in /Users/stijn/Sites/_testserver/flagey/includes/database.mysql.inc on line 172.

warning: Invalid argument supplied for foreach() in /Users/stijn/Sites/_testserver/flagey/sites/all/modules/cck/content.module on line 653.

user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1 query: INSERT INTO flagey_content_field_images (field_images_fid,field_images_title,field_images_alt,nid,vid,delta) VALUES in /Users/stijn/Sites/_testserver/flagey/includes/database.mysql.inc on line 172.

warning: Invalid argument supplied for foreach() in /Users/stijn/Sites/_testserver/flagey/sites/all/modules/cck/content.module on line 653.

user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1 query: INSERT INTO flagey_content_field_audio (field_audio_fid,field_audio_sample_rate,field_audio_bitrate,field_audio_channel_mode,field_audio_playtime,nid,vid,delta) VALUES in /Users/stijn/Sites/_testserver/flagey/includes/database.mysql.inc on line 172.

The first warning concerns a multiple link field with optional title.
The second and third concern an optional image field allowing multiple values.
The last two concern an audio field allowing multiple values.

These three fields are all optional (and allow multiple values) and were left empty on submitting. If I fill out any one with at least one value the error is gone.

No problem for testing this patch. We can all benfit from it.
I tried out both of your last patches and got a new error when submitting or editing content.

Fatal error: [] operator not supported for strings in /Users/stijn/Sites/_testserver/flagey/sites/all/modules/cck/content.module on line 662

On line 662 is

$insert['placeholders'][$delta][] = '%d';

If I roll back line 661

foreach ($insert['placeholders'] as $delta => $value) {

to that of path #9

foreach ($insert['values'] as $delta => $value) {

the code executes, but I get the same errors for my image and audio fields as in #20

subscribe

Hi gordon. Thanks for the latest patch. That fixed the php error I was getting, but I was still getting Invalid argument errors (on submitting content) from my comment 20.

The modules I am using that cause these problems are imagefield.module and mediafield.module
http://drupal.org/project/mediafield
http://drupal.org/project/imagefield

It started to dawn on me that you put all data in an array and then you output the queries in a loop. But because these fields are empty no array is created. Wich was causing the invalid argument in foreach ($insert['placeholders'] as $delta => $value)

So I added an if statement to check if $insert['placeholders'] is an array before creating the queries. This seems to work fine.

Now nothing happens with these fields, as it should, when they are left empty. If one or more value(s) is added everything sits in aan array en the querie is added.

There might be a better way to do this? An earlier check? I hope this doesn't break support on other modules. I am going to try it out on different types of testing sites later today and see what happens.

foreach ($inserts as $table => $insert) {
        if (is_array($insert['placeholders'])) {
        
          $args = array();
          $insert['columns'][] = 'nid';
          $insert['columns'][] = 'vid';
          if ($insert['multiple']) {
            $insert['columns'][] = 'delta';
          }
       
          $sql = 'INSERT INTO {'. $table .'} ('. implode(',', $insert['columns']) .') VALUES ';
          $sql_values = array();
        
          foreach ($insert['placeholders'] as $delta => $value) {
            $insert['placeholders'][$delta][] = '%d';
            $insert['placeholders'][$delta][] = '%d';
            if ($insert['multiple']) {
              $insert['placeholders'][$delta][] = '%d';
            }
            $sql_values[] = '('. implode(', ', $insert['placeholders'][$delta]) .')';
            if (!empty($insert['values'][$delta])) {
              $args = array_merge($args, $insert['values'][$delta]);
            }
            $args[] = $node->nid;
            $args[] = $node->vid;
            if ($insert['multiple']) {
              $args[] = $delta;
            }
          }
        
          $sql.= implode(',', $sql_values);
        
          db_query($sql, $args);
        }
      }

I tried out your last patch and this does solve the problem of the invalid argument. But a new problem (minor) has shown up.

After updating an item and removing the added images and audio files the record in the database is replaced with an empty one, while the old record containing the information of the image/audiofile should be deleted.

If I use my patch the image records are deleted correctly but not the audio ones. So this might be more mediafiled.module related?

Same when creating a new item and leaving these fields blank. Empty records are added to the database. This could clutter the database and make it larger than necessary.

this wasn't happening before. I have some specific code to execute and write in my template when there is a value for audio of images. When submitting using the last patch the html is always printed even though there are no images/audio files because of the blank row. This does not happen when using the normal cck file.

I just checked the database of the live site wich uses the unpatched version of cck. Apparently some field do add a blank row to the database, but these are only the fields that are part of the cck package. The imagefield and mediafield don't insert empty rows on creation.

However, when updating an item that contains images or audio, de records for images are deleted and those for audio are replaced by an empty row.

So the standard behaviour of
cck + imagefield: when empty no rows are added to the database (or records are deleted on update)

cck+ audiofield (mediafield): when empty on creation no rows are added to the database, after updating ande deleteing a file the rows in the database are replaced by an empty row.

Something in your patch is overriding this behaviour by always inserting empty rows. Wich seems to be standard cck behaviour. (I'd prefer no rows were added but this is another issue all together).

Considering the above:
You latest patch (comment 33) seems to work fine and enforces standard cck behaviour
My patch (comment 32) also seems to work fine and respects the standard behaviour of imagefield.module and mediafield.module

I guess your patch is the safer bet when it comes to themeing and compatibility with cck.

Ref. #36:

"Yes I know that this does this, but this is actually how CCK stores data. If there are no rows in a multiple field it will add 1 blank row as a place holder so that when the node is loaded it will populate the node correctly."

Can't this be dealt with by some logic that identifies empty fields and then takes care of leaving them empty at the moment of populating the form? This seems to have the potential for performance improvement too.

I think I found another bug in this one. I will do my best to describe the problem.

I have a multilingual site running using internationalization (i18n). I have a custom contenttype with imagefield. It is a single value required field. Display is done with imagecache, although after the tests I have done I al pretty sure this has nothing to do with it.

The problem is with updating nodes that share the same image.

The workflow:
I create a new node with the required image in say English. Then I translate the nodes vie the translation menu, so for each language the content of the fields is copied. I am now using the same image in all my translated nodes.

After doing all this I realize I want to use a different image. I now have to change the image for each translated node. (I could activate the synchronize module that comes with i18n, which would automatically change the value for all the connected nodes. But it is experimental and broke some other more importent features we need.)

When uploading a new image in one of the nodes the original image is removed, and I assume some rows in the database are removed/replaced/edited. Sadly the image is also removed from the server, although it is still being used by other nodes (but this is default imagefield behaviour apparently).

The problem is when I go to the next node, one of the translated nodes, and upload the new image everything breaks after submission. The image is entered in the database files table but somehow isn't connected in the right way to the node. The returns and empty. Yet something is still in the database because when you go and edit the node again the image is not loaded correctly giving the impression that there is no image attached. Yet the field says that if I upload a new image the old one will be replaced and I can save the node with the required field apparently empty, getting no error.

If I role back the patch and use the default cck content.module everything works fine. New images are added correctly. Something is going wrong in this patch during update.

I have the latest version of Drupal (6.6) and CCK, imagefield, filefield and imagecache. Using php 5.2.1, site5 host.

I get this error when I create a node and do not add an image (image is not required in my CCK content entry form):

user warning: Column 'field_images_fid' cannot be null query: INSERT INTO content_field_images (vid, nid, delta, field_images_fid, field_images_list, field_images_data) VALUES (589, 589, 0, NULL, NULL, NULL) in /home/MYSITE/public_html/sites/all/modules/cck/content.module on line 1199.

Any ideas why this is? Any solution? I can't let our editors see this error every time they upload content. Before upgrade to D6 this all worked like a charm.
thanks.

cgjohnson - How is this even remotely related to the current thread ?
Please search imagefield issue queue about that, this has been reported before - maybe even fixed since then.

The D5 version is no longer being supported. Sorry.