Text column type doesn't reliably hold serialized variables

This is unfortunate. If we have to do this, let's change all the TEXT columns that are used to store serialized data into BLOB, and promote that to a rule.

Promoting to the database layer, we need to fix that for every database engine.

BLOB fields make any sort of searching or indexing or treatment of that field impossible. It also complicates field handling on some databases. I really don't want to go down that route if we can avoid it.

How many "serialized" fields do we have? I know of cache, session, and bits of the Field system.

Oh yeah, and PHP, you suck. PostgreSQL, you suck, too.

@Ognyan Kulev: we need to promote clear and simple rules.

"All serialized data should be stored in BLOB fields" is clear and simple.

"All serialized data that might have zero values in them should be stored in BLOB fields, otherwise TEXT is ok" is unclear and confusing.

I agree. If it can contain NULL or is just an array of bytes (i.e. raw serialization), then it is binary and should be treated as such by everything. If it is text (and has a specific character encoding), then it should never contain NULL (at least, without escaping). If we need to use text functions against these fields, then we should not be using serialization and should be normalizing the data.

Blob fields for serialized data makes sense, its serialized therefore there is no need to index or search on such fields. Also for schema updates, dropping cache column to re-create them won't matter since we can rebuild those easily enough.

@Ognyan Kulev because this is a change to Drupal 7, you need to write an update function called system_update_70XX in system.install that alters the columns your changing to blob.

Nice work BTW :)

There are other serialized fields besides cache that we cannot drop, like Field API configuration and sometimes even the variable table.

serialized columns will have a key from which they index on, we can build an array in PHP from which we can store the data until it can be inserted back into the new blob field. Unless we can do a manual CAST conversion in the db?

That could work, possibly, but I'm concerned about the memory usage of that approach. It would have to be in 3 separate update hooks: Add new column, port data over one record at a time (batched), and delete old column.

I'd prefer not to if we can avoid it, but maybe.

For MySQL we can probably just do a straight read since BLOBs are almost TEXT and require no special processing. I've no idea about SQLite, though, other than making schema changes to SQLite tables is a PITA.

Although... No one is running Drupal 6 on SQLite, so do we even need that update hook?

Well, technically - no. We currently not supporting any version of Drupal that runs on SQLite until Drupal 7.0 is released. Considering the nature, if need be - a per database solution will be fine.

The queue table doesn't exist in D6, so it wouldn't need to be changed anyway. :-)

In core, there's the cache tables (which don't need data retained as we flush them anyway), sessions (same), and variable. There's also, and this one is ugly, most of the configuration for fields. That has to migrate from CCK's tables in D6 to core's tables in D7. I am not sure what else is happening there, though, so those may be covered elsewhere already by other upgrade processes.

The tables themselves would need to be updated in the schema either way, even if we don't preserve data.

As far as I know, we have everything in place for:

  $data_column_schema = array('description' => 'A collection of data to cache.', 'type' => 'blob', 'not null' => FALSE, 'size' => 'big');
  db_change_field('queue', 'data', 'data', $data_column_schema);

To work on all databases.

Which problem have you bumped into with that, Ognyan?

Subscribe.

I'm wondering if its a reserved word issue? http://www.postgresql.org/docs/7.3/static/sql-keywords-appendix.html the table says its not but "data" is a reserved word in the SQL standard. That PHP code doesn't make sense to me. Shouldn't it be more like:

$this->addField($table, "$field_new", $spec);
$this->connection->query("UPDATE {" . $table . "} SET $field_new = CAST(" . $field . "_old as $type_cast)");
// set not null here if the case.
$this->dropField($table, $field . '_old');

or maybe I'm just seeing it out of context

It's a casting trouble. A working solution I found http://bytes.com/topic/postgresql/answers/174532-cast-text-bytea

drupal=# alter table queue add column data2 text;
ALTER TABLE
drupal=# update queue set data2=data
UPDATE 1
drupal=# alter table queue alter data2 type bytea using decode(replace(data2, '\\', '\\\\'), 'escape');
ALTER TABLE

@Ognyan Kulev anyway we should choose a method for altering

1) add new column, copy data, drop old column
2) alter table queue alter data type bytea using data::bytea;

Both this changes require some decoding table to generate type-transformation

Tagging, tests for serialization also needed at #718636: Cache does not unserialize serialized data in certain conditions which already fixed

This problem can also be triggered in MySQL like this:

drupal_set_message(hash_file('md5', 'CHANGELOG.txt', TRUE));
drupal_exit();

resulting in

PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect string value: '\x90O\xA2\x12\xD8(...' for column 'session' at row 1: INSERT INTO {sessions} (uid, cache, hostname, session, timestamp, sid) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5) ON DUPLICATE KEY UPDATE uid=VALUES(uid), cache=VALUES(cache), hostname=VALUES(hostname), session=VALUES(session), timestamp=VALUES(timestamp); Array ( [:db_insert_placeholder_0] => 1 [:db_insert_placeholder_1] => 0 [:db_insert_placeholder_2] => 127.0.0.1 [:db_insert_placeholder_3] => batches|a:1:{i:1;b:1;}test|b:1;messages|a:1:{s:6:"status";a:1:{i:0;s:16:"�O��(�@���I�\";}} [:db_insert_placeholder_4] => 1269578171 [:db_insert_placeholder_5] => 0ae09c3f5feb76b49cfe9b5d133f172e ) in _drupal_session_write() (line 169 of /home/jonny/sites/drupal-7.x-dev/includes/session.inc).

This definitely needs to be fixed before D7 is released so we are not making major schema changes after.

I would be very interested to see the performance implications of a drupal_serialize() that wrapped gzdeflate() around serialize().

Writing test today.

Additional crash cases:

// Crash cases:
// 1)
// drupal_set_message("This freaky data is not a crash: " . hash('md5', 'lorem ipsum', TRUE));
// drupal_exit();

// 2)
// variable_set("dontcrash", "This freaky data is not a crash: " . hash('md5', 'lorem ipsum', TRUE));

// 3)
$name = "dontcrash";
$value = "This freaky data is not a crash: " . hash('md5', 'lorem ipsum', TRUE);
db_merge('variable')->key(array('name' => $name))->fields(array('value' => serialize($value)))->execute();

(New patch. Patch in #21 only covered queue table.)

What's the problem?

serialize($binarydata) will fail when inserted into a 'text' field. For example:

  //   variable_set("dontcrash", "Please dont crash" . hash('md5', 'too late', TRUE));
  $name = "dontcrash";
  $value = "Please dont crash" . hash('md5', 'too late', TRUE);
  db_merge('variable')->key(array('name' => $name))->fields(array('value' => serialize($value)))->execute();

What's the solution?

Convert:
'type' => 'text',
to:
'type' => 'blob',
'size' => 'big', // if data is bigger than 16k (mysql), make big

What fields were changed (current size)?

variable.value (big)
sessions.session (big)
actions.parameters (big)
batch.description (big)
menu_router.load_functions (16k) - comments say serialize
.to_arg_functions (16k) - comments say serialize
.access_arguments (big)
.page_arguments (big)
menu_links.options(big)
queue.data (big)
system.info (big)

What fields were skipped?

cache.headers (assumed cache knew what it was doing)
menu_router.description (description, not a serial)
menu_router.include_file (filename, not a serial)

Test plan:

Tell yer testbot to put that in it's pipe and smoke it!

I reviewed the test at the code sprint and it looks good. The patch does apply successfully and the test does not fail locally.

Note, this is a special test case. The patch is to the system.install, so the patch must be applied BEFORE installing Drupal--ie. while the database is still empty.

Testing manually gets the desired result -- test fails if the code patch is not applied. The test succeeds if installing using the patched system.install code.

Not sure how to get this so that testbot will like it.

"The test did not complete due to a fatal error. Completion check text.test 73 TextFieldTestCase->testTextfieldWidgets()"

Hmmm... What's up with that testbot?

Tests pass on my local dev machine.
- updated CVS
- ran just textfield, then all field tests

Reviewed test for impact, and didn't see anything that should have caused the test to fail to run.

Line 73 is a single "test" function that runs two large tests (text and text_long). Resubmitting patch with fix to break these two tests into two test functions.

We'll get more info on next run, or see if it's an intermittent test harness problem.

#35: serialize.patch queued for re-testing.

This new patch is the SAME CODE, but with a slight modification to the failed test to help with debugging. Test passes on my local computer, but did not complete due to "fatal error" on testbot. This patch will help isolate that error if testbot continues to fail.

Previously reviewed and approved: serialize.patch

serialize.patch failed on testbot problem. Resubmitted to testbot and passed. See #35

serialize2.patch is the SAME CODE, but added some debug statements to help diagnose testbot failure. This patch can be IGNORED.

Michael, I think the last successfully tested patch will be the one applied.

Might want to re-upload the patch from #35 to be sure.

Forgot that it changes the status... changing back, this patch is good.

Can someone confirm please that we don't need a complete definition array for the update function? I think we do actually need a complete definition with null and description keys.

Also, queue is new in Drupal 7 so we don't need to update that one.

Tagging.

#35 There are serialized fields in other core modules.

dblog
-----
watchdog.variables

field
-----
field_config.data
field_config_instance.data

filter
------
filter.settings

image
-----
image_effects.data

rdf
---
rdf.mapping

system
------
variable.value
actions.parameters
batch.batch
cache.headers
menu_router.load_functions
menu_router.to_arg_functions
menu_router.access_arguments
menu_router.page_arguments
menu_links.options
queue.data
sessions.session
system.info

user
----
user.data

Try this.

It converts all the fields in #46 to 'blob'.

Fields that store serialized data now have 'serialize' => TRUE even if they are not written with drupal_write_record() .

drupal_write_record() will only serialize data for 'blob' fields.

Confirm that db_change_field() should use full field definition. For example http://api.drupal.org/api/function/block_update_7003/7

Also all field constraints should be dropped (if any) http://api.drupal.org/api/function/db_change_field/7

I implemented the full field definition for the update functions. This is required because the mysql driver uses ALTER TABLE {' . $table . '} CHANGE which needs the same full column definition as CREATE TABLE: http://dev.mysql.com/doc/refman/5.1/en/alter-table.html

column_definition clauses use the same syntax for ADD and CHANGE as for CREATE TABLE. See Section 12.1.17, “CREATE TABLE Syntax”.

serialized fields don't have any constraints

Really needs a bit of work...

+++ modules/filter/filter.install	26 Apr 2010 20:21:46 -0000
@@ -422,6 +422,23 @@
+  ¶

trailing whitespace

+++ modules/system/system.install	26 Apr 2010 20:21:47 -0000
@@ -2398,6 +2409,100 @@
+  ¶

same 4 times

+1 to RTBC after re-roll

Upgrading from D6 needs to be tested with mysql and postgres.

@andypost: I don't know what trailing whitespace you are referring to.

Fix testDrupalWriteRecord() .

@jbrown please try to review your patch with dreditor and you'll see what I'm talking about, there should not be spaces on blank lines. Dreditor will show them as red colored

+++ modules/filter/filter.install	26 Apr 2010 20:57:30 -0000
@@ -422,6 +422,23 @@
+  db_change_field('filter', 'settings', 'settings', $spec);
+}
+  ¶ // two spaces here
+/**
  * @} End of "defgroup updates-6.x-to-7.x"

this is a trailing whitespace

Powered by Dreditor.

Thanks @andypost !

Now it's good to go

Not quite yet!

Upgrading from D6 needs to be tested with mysql and postgres.

This patch fixes creation of queue table during upgrade.

Todo: alter cache tables during upgrade.

We can drop and recreate cache tables, should be quicker than altering them if they have stuff in them. Modules which maintain their own cache table are going to have to do this too, so this will have to be documented in the 6-7 upgrade guide.

Are cache tables currently dropped during D6->D7 upgrade? Is there an issue for this?

Updated patch with cache tables dropped and recreated.

I'm confused. Why are we converting to use drupal_write_record() more? Blob fields are handled transparently by the DB drivers.

I am also not wild about the serialized flag in schema. That's not information about the table; it's information about how Drupal may or may not use those fields. I am not convinced those belong together; to date the schema is almost Drupal-agnostic in its definitions.

The main purpose of this patch is for serialized variables to be reliably stored in the db.

The two places where I have used drupal_write_record() turn 45 lines into 2 lines and make the code much cleaner.

This patch does not introduce the 'serialize' schema option - whether or not that is a good idea is a separate discussion. Although, perhaps this patch should not have 'serialize' for fields that are not written by drupal_write_record().

@Crell This patch about storing serialized data and not about handling blob fields.

Postgres could not be tested until #761976: DatabaseSchema_pgsql::changeField() is broken

Patch seems good

#62: serialize.patch queued for re-testing.

Hrm. OK, that's what I get for reading patches too quickly. Yeah, serialize is already there. It sucks, but we can't change that now.

It looks like the patch just broke on a change to the schema of cache tables, so here's a simple reroll. Bot, you cool?

Grumble grumble duplicate function names grumble grumble. I think I got them all.

I guess I didn't. *sigh*

You've got a few comments past the 80 character limit. Plus your comments for functions need to begin with a 3rd person verb as far as I understand (ie: s/convert/converts).

Don't hate me tho. I thought I'd point that out before aspilicious and sun jump you and steal your candy.

     // Build array of fields to update or insert.
-    if (empty($info['serialize'])) {
+    if ($info['type'] != 'blob' || empty($info['serialize'])) {
       $fields[$field] = $object->$field;
     }
     elseif (isset($object->$field)) {

Em. Let's not do that. If I set serialize, I want the data serialized. End of story. The fact that some databases cannot handle it in some cases is totally irrelevant.

Other then that, I see no compelling reasons to move to drupal_write_record() in those places. Let's only fix one bug at a time, please remove those. This issue is *only* about converting text columns to blob column, and adding a test.

Speaking of which:

+  /**
+   * Test that we can insert serialized data into important fields.
+   */
+  function testInsertSerializedData() {
+    // Alternative crash cases:
+    // 1)
+    //   drupal_set_message("Please dont crash" . hash('md5', 'too late', TRUE));
+    //   drupal_exit();
+    // 2)
+    //   variable_set("dontcrash", "Please dont crash" . hash('md5', 'too late', TRUE));
+    $name = "dontcrash";
+    $value = "Please dont crash" . hash('md5', 'too late', TRUE);
+    try {
+      db_merge('variable')->key(array('name' => $name))->fields(array('value' => serialize($value)))->execute();
+      $this->pass(t('Success! No PDO exception thrown.'));
+    } catch (Exception $e) {
+      $this->fail(t('Fail! Your serialized blobs are quite distraught.'));
+    }
+
+  }

This:

1. Doesn't really do what it says it does. It more tests for inserting binary data then anything.
2. Doesn't have a lot of added value (we already know for a fact that storing binary data in a TEXT column cannot reliably work, and we already have tests for inserting binary data in BLOB columns).

I vote for just removing it.

Implemented recommendations in #74.

For {cache_block} and {cache_update} I just modified the update functions that drop the headers field. Those functions now just drop and recreate the tables.

Is it okay to use drupal_get_schema_unprocessed('system', 'cache') in an update function?

Thanks for the new patch, we are getting there. Some additional remarks:

• Please remove the new 'serialize' => TRUE entries in the schemas.
• Ok to simply drop/create the cache tables, that makes a lot of sense.
• We already have an update function somewhere in the system module to crop the headers column of the cache_* tables, could you just modify that instead of the code at the bottom of system_update_7055()?

Done.

I don't think we can use drupal_get_schema_unprocessed() in an update function for the reasons discussed here: http://drupal.org/node/150220

Agreed, I don't think you can use drupal_get_schema_unprocessed()

The main blocker is #761976: DatabaseSchema_pgsql::changeField() is broken

You can't ever use existing schema in the code in the update because the the schema in hte code might change and then your update lands you in a different place and various users will have a different schema with the same schema version which is a terrible mess. Write out the definition.

Yeah - I'll reroll this with the definition once the blocker is resolved.

Why is the postgres issue a blocker?

See #65

db_change_field() needs to work on postgres

I agree, nothing blocks this issue. The issue with PostgreSQL schema implementation doesn't block testing of this on MySQL and SQLite.

I wasn't aware that we were breaking HEAD for postgres.

HEAD is already broken for postgres, we don't support head to head updates (see http://drupal.org/project/head2head), and we already have db_change_field() calls in core - so this patch doesn't cause any harm that's not already there.

@catch As reported at #32 mysql affected too!

Please move ahead with this patch. There is nothing blocking it. PostgreSQL doesn't implement the API correctly, and this problem is dealt with in the other issue. Nothing prevent us to move forward here.

I haven't actually tested this yet on a D6=>D7 upgrade.

Also see #831332: drupal_get_schema_unprocessed() called in update functions.

Hang on - the each cache table has its own description.

+  // 'text' column type doesn't reliably hold serialized variables. Convert to 'blob'. See http://drupal.org/node/690746
+  // 'headers' column is no longer used

This is longer than 80 chars and we rarely reference issues in code comments.

Just "Convert text to blob since this stores serialized data, remove headers column." should be fine I think.

Leaving CNR for the bot and apart from that it looks fine.

I'll run it through a D6=>D7 upgrade tomorrow and check it with the schema module.

Looks good to me too.

Fixes foreach typo in system.install

Updated comments.

Carefully reviewed the patch. Everything we need seems to be there.

Opened head2head issue ahead of time #832124: #690746 - Text column type doesn't reliably hold serialized variables.

subscribe

Fixed schema upgrade code in the image module.

This changes are correlate with #831332: drupal_get_schema_unprocessed() called in update functions

Maybe better to sync changes in cache tables by producing one update function.

#831332: drupal_get_schema_unprocessed() called in update functions got committed, meaning this patch doesn't apply any more.

Why are the cache tables droppend and recreated instead of doing a db_change_field like on the other the converted fields?

@andypost: I've posted an idea for achieving that (and a reason) in #837342: Simultaneous updating required for modules with a cache bin when the cache schema changes.

@Stevel The reason to drop/create cache tables is because cache tables could be big and once this change happens in _update_N so cache should be cleared anyway. So actually no reason to keep data in cache tables. Contrib modules that have cache tables should care about cleaning and tracking changes in cache tables.

Full reroll.

I kept the drop / recreate pattern for cache tables, which simplify everything.

This looks ready to go in for me.

Looks ready to me too, would mark RTBC but I haven't tested it at all.

#105: 690746.patch queued for re-testing.

RTBC based on #106 and my previous attempt on #98.

Committed to CVS HEAD. Thanks.

Sorry about that.

no, all cache tables have their own descriptions, so the array definition which holded these definitions is gone here, I've seen it somewhere recently, looking up where.
edit: found it in #101:

+  $cache_tables = array(
+    'cache' => 'Generic cache table for caching things not separated out into their own tables. Contributed modules may also use this to store cached items.',
+    'cache_form' => 'Cache table for the form system to store recently built forms and their storage data, to be used in subsequent page requests.',
+    'cache_page' => 'Cache table used to store compressed pages for anonymous users, if page caching is enabled.',
+    'cache_menu' => 'Cache table for the menu system to store router information as well as generated link trees for various menu/page/user combinations.',
+  );

Then we need $schema['description'] = $description in each run of the for loop.

New patch that sets the correct description for each cache table.

Committed to CVS HEAD. Thanks.