Issue #317156 by navneet0693: Moved logic from activity_creator_update_8802 to hook_post_update_name and refactored.

[navneet0693]

Problem

Originating PR: #1965

When upgrading OpenSocial from 8.4 to 8.5, drush updb fails due to https://github.com/goalgorilla/open_social/blob/10.0.x/modules/custom/activity_creator/activity_creator.install#L146

The better place for any such entity updates is hook_post_update_name, also as documentation of hook_update_N states:

Be careful about API functions and especially CRUD operations that you use in your update function. If they invoke hooks or use services, they may not behave as expected, and it may actually not be appropriate to use the normal API functions that invoke all the hooks, use the database schema, and/or use services in an update function -- you may need to switch to using a more direct method (database query, etc.).
In particular, loading, saving, or performing any other CRUD operation on an entity is never safe to do (they always involve hooks and services).
Never rebuild the router during an update function.

Also, https://github.com/goalgorilla/open_social/blob/10.0.x/modules/custom/activity_creator/activity_creator.install#L163 is increasing the range of query exponentially.

Definition of range function:

/**
   * Restricts a query to a given range in the result set.
   *
   * If this method is called with no parameters, will remove any range
   * directives that have been set.
   *
   * @param $start
   *   The first record from the result set to return. If NULL, removes any
   *   range directives that are set.
   * @param $length
   *   The number of records to return from the result set.
   * @return $this
   *   The called object.
   */
  public function range($start = NULL, $length = NULL);

1st round: range(0, 100) // 100 entitites (Total = 100)
2nd round: range(100, 100+100) // 200 entities (Total = 300)
3rd round: range(200, 200+100) // 300 entities (Total = 600)
and so on..

Also, as @Kingdutch noted that this update could skip certain ids to be processed as it is reading https://github.com/goalgorilla/open_social/blob/10.0.x/modules/custom/activity_creator/activity_creator.install#L160 and deleting ids https://github.com/goalgorilla/open_social/blob/10.0.x/modules/custom/activity_creator/activity_creator.install#L187 from the same table.

Assume, we have notifications [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20]
The first query for $sandbox['total'] will count(*) 20 items. Say the $activities_per_batch is 5.
The first cycle will load five with offset 0:
$aids = [1, 2, 3, 4, 5];
// delete
$sandbox['current'] will now be set to 5.
The second cycle will load (assuming you fix the $range argument) another 5, offset 5. However, the dataset in the database is now:
[6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20]
So you’ll load 5 offsets 5:
[11, 12, 13, 14, 15]
This means 6 through 10 aren’t processed.

Solution

1. Move function activity_creator_update_8802(&$sandbox) to activity_creator.post_update.php
2. Refactor the function to delete in bulk.
3. Correct the logic of getting and deleting the data.

Issue tracker

https://www.drupal.org/project/social/issues/3171563

How to test

• Using version <8.5 of Open Social.
• Upgrade to 8.5.
• Check for no memory outage.

Screenshots

N.A

Release notes

Logic from activity_creator_update_8802() moved to activity_creator_post_update_8802_remove_activities_with_no_related_entities()

Change Record

Resolved a problem of memory outage on upgrade by moving logic from activity_creator_update_8802() moved to activity_creator_post_update_8802_remove_activities_with_no_related_entities()

Translations

N.A

[tbkot]

Looks good to me. Also, I've checked it on local and all ok.

[ribel]

I have tested the latest OS 9.2 with this patch and all updates went well.

[ribel]

@ronaldtebrake I think it would be good to merge this, and maybe even do a new release, what do you think?

[ronaldtebrake]

Thanks for notifying me, think it's a good idea indeed.

[navneet0693]

I request to hold on for merging. I am facing memory outage issues when dataset is above 1000K.

[navneet0693]

Looks I found the issue I was facing. It's related to drush:

1. batch process fails to start new thread after reaching 50% of available memory drush-ops/drush#3117
2. Revert "Remove respawn after 50% memory exhaustion during batch proce… drush-ops/drush#3952

As Berdir says in drush-ops/drush#3117 (comment). resetCache() was also not sufficient enough to solve our problem.

[navneet0693]

I have create a new PR #1988 with different approach.

[robertragas]

PR #1988 is being chosen in favor of this one, so closed.