Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase size of media.field_file_size #829

Merged
merged 6 commits into from
Mar 30, 2021
Merged

Increase size of media.field_file_size #829

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2bed2bc
Increase size of media.field_file_size.
seth-shaw-unlv Mar 17, 2021
781d0c4
coding style
seth-shaw-unlv Mar 17, 2021
f0d0d90
Use temporary files for update and schema::changeField()
seth-shaw-unlv Mar 17, 2021
a52617e
Rename, recreate, and reload tables instead of using temporary files.
seth-shaw-unlv Mar 26, 2021
a8f8e40
Batch the update
seth-shaw-unlv Mar 26, 2021
4a0a47c
drop renamed tables *after* we are done copying
seth-shaw-unlv Mar 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions modules/islandora_core_feature/config/install/field.storage.media.field_file_size.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ field_name: field_file_size
entity_type: media
type: integer
settings:
unsigned: false
size: normal
unsigned: true
size: big
module: core
locked: false
cardinality: 1
Expand Down
67 changes: 67 additions & 0 deletions modules/islandora_core_feature/islandora_core_feature.install
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
<?php

/**
* @file
* Update Hooks.
*/

use Drupal\field\Entity\FieldStorageConfig;

/**
* Updates Media file size field storage for larger files.
*/
function islandora_core_feature_update_8001() {

$entity_type = 'media';
$field_name = 'field_file_size';

$database = \Drupal::database();
$data = [];
$storage_key = $entity_type . '.field_schema_data.' . $field_name;
$storage_schema = \Drupal::keyValue('entity.storage_schema.sql');
$field_schema = $storage_schema->get($storage_key);

foreach (["{$entity_type}__{$field_name}", "{$entity_type}_revision__{$field_name}"] as $table) {
// Squirrel away the data.
$data[$table] = $database->select($table, 'n')
->fields('n')
->execute()
->fetchAll();

// Clean it out for resizing.
$database->truncate($table)->execute();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems potentially catastrophic, if the process later fails for some reason... different DB with different syntax or whatever?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I attempted to resize without cleaning it out and Drupal simply would not let me. Cleaning it out is the only way to save it. I suppose we could create a temporary table instead of relying on an in-memory store... 🤔

According to the Drupal notes this is how you truncate a table.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat ambiguous due to nomenclature, but I'm assuming you don't mean a "temporary table" as supported by various DB engines which are automatically deleted at the end of the DB session, as they would exhibit the same issue of losing the stored data if something failed before getting it back in... but yeah, a short-lived table we create/check-for and populate, and delete when we're done... assuming success with in-place alteration with changeField() doesn't make it irrelevant.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like Drupal won't let changeField update the table without cleaning it out first either: "[error] The SQL storage cannot change the schema for an existing field (field_file_size in media entity) with data."

I'll see about creating a table to keep this all in, should the update fail and need to be attempted again.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a new table I created a temporary file to stash a JSON copy of each table before truncating. I then re-read the file to insert the data back in after the schema is updated.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for using public, I didn't like the alternatives. Not all sites have a private filesystem set (a non-starter for an update hook) and temporary is at the whims of the OS' clean-up, so I'd rather not trust it for keeping our data dump in the case of a failure. At least a site admin knows where public is if they have to go grab it should the site fail. Also, it is just a list of file sizes.... That said, if y'all prefer we use the temporary file-space, it is a simple change; just confirm either way.

Ah, update hook batches (are a pain to use properly). I recently fought with this on a separate internal migration. Batch works best when the first and third steps are performed once and the middle step is repeated. (E.g. moving data from one field to another where the first phase sets up the new field, the middle step gradually copies, and the final step cleans up. Or a single repeatable step like modifying values in place.) Our three phases—dump all the data, make the change, re-import everything—are the inverse where the first and third can be done in chunks but the middle phase can't. Batching isn't suitable for this scenario.

I've attempted to use the strategy suggested by that recipe for in-place editing, but Drupal still balks at it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should also note that the pain of creating new tables, copying the data in then out, then dropping them didn't seem worth it compared to doing the same with temporary files briefly in the public filesystem. The attack window (knowing it exists and when) and reward of this is really small.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stuck in my head last night; I might be able to use batch on this after all by nesting the batches within stages...

Copy link

@adam-vessey adam-vessey Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of dumping to files and what not, might it be simpler to:

  • rename the target tables (assuming that renaming doesn't break similarly to the ::changeField() business.
  • create the new tables using the up-to-date schema
  • copy table content using insert/from, from the renamed to the newly created tables
  • drop the renamed copies

... if not, couldn't whatever batch structure be accomplished with multiple update hook implementations? A batch update hook followed by a non-batch, followed by a batch, kind of thing?

... any which way... still have the potential issue with the type change not being strictly widening with the move to exclude negative values... are we just ignoring this, or should we check for anything negative before trying to process, or... are we happy just letting it error how however it might?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, nice. I had seen references to Schema::copyTable which was no longer available. I hadn't see the rename table function.

As I eluded to in my previous comment, I think we can do the batching in a single hook. Also, with the table rename tactic, we only really need to batch the insert/from, which is manageable.

I do think we need to error, or at least warn, on negative file sizes. I mean, semantically speaking, those values are already broken.

$database->schema()->dropPrimaryKey($table);

// Resize the main field data table.
$database->query("ALTER TABLE $table MODIFY {$field_name}_value BIGINT(11) unsigned NOT NULL");
seth-shaw-unlv marked this conversation as resolved.
Show resolved Hide resolved

// Update storage schema.
$field_schema[$table]['fields'][$field_name . '_value']['type'] = "bigint";
$field_schema[$table]['fields'][$field_name . '_value']['unsigned'] = "true";
}
$storage_schema->set($storage_key, $field_schema);

// Update field storage configuration.
$config = \Drupal::configFactory()
->getEditable("field.storage.{$entity_type}.{$field_name}");
$config->set('settings.size', 'big');
$config->set('settings.unsigned', TRUE);
$config->save(TRUE);

// Make sure the new config persists.
FieldStorageConfig::loadByName($entity_type, $field_name)->save();

// Reload the data.
foreach ($data as $table => $rows) {
foreach ($rows as $row) {
$database->insert($table)
->fields((array) $row)
->execute();
}
}

return t('Length of @entity-type.@field-name updated to an unsigned big int', [
'@entity-type' => $entity_type,
'@field-name' => $field_name,
]);
}