Remove Magento/Customer/Setup/RecurringData.php #21235

slackerzz · 2019-02-14T17:00:05Z

Description (*)

Magento/Customer/Setup/RecurringData.php performs a Customer grid reindex at every setup:upgrade. In real shops, you usually have hundreds of thousands of customer and this requires 10+ minutes in my case. This is not acceptable.

Fixed Issues (if relevant)

2.3 Customer module Recurring setup script performance problems. #19469: 2.3 Customer module Recurring setup script performance problems
...

Manual testing scenarios (*)

...
...

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

magento-engcom-team · 2019-02-14T17:00:10Z

Hi @slackerzz. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

orlangur

@slackerzz I believe this file was added for a reason. Please find out why it was added in the first place and then we can decide whether it is still needed or not.

slackerzz · 2019-02-15T08:43:52Z

@orlangur i cannot know the reason why this was added, at first it seems a bad copy-paste.
Have you ever run setup:upgrade with more than 500k customer?

orlangur · 2019-02-15T11:07:03Z

@slackerzz from dc4bb0e#diff-1868f52993762635208d45246e032b70 it looks like it was intentionally moved. Please analyze commit history to assure removal of this code won't cause any regression.

slackerzz · 2019-02-15T12:03:20Z

Intentionally or unintentionally this means that a deploy will have about 10 minutes of maintenance.

This is not acceptable for a real shop.

orlangur · 2019-02-15T12:26:02Z

@slackerzz we cannot remove code "just because". For a particular shop, where it causes troubles, it can be removed.

However, if there is a real problem fixed by this recurring update, instead of just a removal a proper fix should be elaborated.

slackerzz · 2019-02-15T12:47:03Z

Ok, don't worry. I'll prepare a composer patch.
Have a good day.

ghost · 2019-02-15T12:47:04Z

Hi @slackerzz, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

hostep · 2019-02-15T19:48:00Z

Let's not close this just because of not agreeing with each other. @slackerzz has a valid point here. @orlangur also has a point.

Maybe @sereban can shed some light on why this RecurringData script was added in the first place?
Was this maybe a leftover from testing something perhaps? (I can imagine using a RecurringData script as an easy way to test a db migration script while developing)

Nazar65 · 2019-02-15T19:52:45Z

Hi @hostep seem like i'm found an open issue -> #19469

Nazar65 · 2019-02-15T20:21:18Z

@slackerzz Perhaps we need to add functionality where it will be necessary to do reindex only when it is needed?

hostep · 2019-02-15T20:22:19Z

This probably won't help much with discovering why this was added, but here are all the commits which contain the phrase MAGETWO-85326:

(maybe someone with access to Magento's Jira system can find more info in that particular ticket around this?)

commit 15d1a96b4fc38a30e9b145c14601fdae26e30ae4
Merge: a2b6f53fb63 65667f6a54c
Date:   Sun Feb 4 14:34:00 2018 +0200

    Merge pull request #2041 from magento-trigger/MAGETWO-84100
    
    * MAGETWO-83201: Creating Diff artifacts between arbitrary Declarative Schemas
    * MAGETWO-84100: Unify Magento SQL adapters
    * MAGETWO-85326: Tool for generating schema
    * MAGETWO-81032: Create whitelist of tables
    * MAGETWO-86872: Stabilize L3 and L4 builds
    * MAGETWO-86895: Inspect all old scripts and remove them
    * MAGETWO-87208: Stabilize schema setup mechanism

commit 35c872e9786ffa77e50ccbba9443680f866abeff
Date:   Fri Jan 12 15:57:16 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on CE stuff

commit 42d46e6e8342d1488c471fe80bc9cc783d91c192
Date:   Fri Jan 12 15:47:10 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on CE stuff

commit 4430c44dc1565e251161cf29765f59682333b711
Date:   Fri Jan 12 15:12:37 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on CE stuff

commit 1d98530387353c64e930cc2eb0be959b89a26d9d
Date:   Fri Jan 12 14:31:17 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on EE stuff

commit 9e61d021480844960ae87037ff9293753d21d732
Date:   Fri Jan 12 14:09:06 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on EE stuff

commit 1b481910a0d068cf97dedf76734676fe54529920
Date:   Fri Jan 12 14:04:15 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on EE stuff

commit af201a1474f8ef6ea61b9016bab934ce7b631465
Date:   Fri Jan 12 14:04:15 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on EE stuff

commit ebb59fc28a6040fedb76da18e214c9ffc025087e
Date:   Fri Jan 12 14:04:15 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on EE stuff

commit 150afea1386ec85cc56aecb50d492cbe0cdd00d2
Date:   Fri Jan 12 14:04:15 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on EE stuff

commit e14beeb8d01fb12319b92f496ebeec5a728e70f0
Date:   Fri Jan 12 14:04:15 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on EE stuff

commit af214e83eebda8db7d6ded179382dd751941aaf5
Date:   Fri Jan 12 13:18:26 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on CE stuff

commit dc0fa0dca622a27a5e6fc8de027cd299cf3e767b
Date:   Fri Jan 12 13:09:23 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on CE stuff

commit 70fe8f55bee18fd3617b6dd55142265fd610733c
Date:   Fri Jan 12 12:47:09 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on CE stuff

commit 5946584770ffbeca1b20435bd5856bc850297865
Date:   Thu Jan 11 19:04:11 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on CE stuff

commit 66af1121c5e3530a897ac389fa98d233f81e3e6e
Date:   Thu Jan 11 16:48:44 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on CE stuff

commit 20f285b50fc8f75ec3961c78b5e5d0215c0014e3
Date:   Thu Jan 11 11:56:08 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on EE stuff

commit 684a6de540dadab0ef35358882b6dfbd18c66a2e
Date:   Wed Jan 10 14:48:06 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on EE stuff

commit 388ae8a76fbbcb9a5649463dd844545789ba05f6
Date:   Tue Jan 9 18:04:32 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --working on EE stuff

commit 3dee823f028edf0798ee50c55ec7a56c5c35068b
Date:   Fri Jan 5 19:08:44 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --make possible upgrade from CE to EE

commit dc4bb0eae824588459f1e82b64dae96ab6026092
Date:   Thu Jan 4 19:57:16 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --stabilize CE

commit 4a8e1921dea933bc426e0d011ba748f05b1b45bf
Date:   Thu Jan 4 17:58:45 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --add b2b schema

commit 9c4aa724c1cddf687bb05f2672a2464717368401
Date:   Thu Jan 4 16:07:10 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --add staging schema

commit a385dc80644f214b7b8422d4e4ea40a22664f33b
Date:   Thu Jan 4 16:07:09 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --add staging schema

commit 6da8148ee48ea3313f5d62648ded0e65a69dd9fe
Date:   Thu Jan 4 16:07:09 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --add staging schema

commit 7b0c8dea5e6ef56c9b20551172ba24678522377e
Date:   Thu Jan 4 16:07:09 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --add staging schema

commit b13af5d59880ae2d39f0bc7464e9e0cfa3e5a292
Date:   Thu Jan 4 16:07:09 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --add staging schema

commit 86fb388e59c0fed19ce7f20cc5a0f80ecca5f6e3
Date:   Wed Jan 3 13:28:36 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --make magento works with new schema

commit 732bb68d6d6a4cc9fc6730457fb6a2c908f10c27
Date:   Tue Jan 2 14:37:27 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --make magento works with new schema

commit 2fb1956dc0e7544bdd49aac6fd4d793ee76457b5
Date:   Tue Jan 2 14:11:47 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --add schemas

commit 00e29b2f5e51ff187e0817652acc4c0b1ee38bea
Date:   Tue Jan 2 14:03:42 2018 +0200

    MAGETWO-85326: Tool for generating schema
    
    --add schemas

commit 113f4a5f9ff1924b8f35838ec089b1ea61ed72e7
Date:   Fri Dec 29 13:23:55 2017 +0200

    MAGETWO-85326: Tool for generating schema

commit 4c123f1e82ef7d33255709bd726410dad35ab9de
Date:   Fri Dec 29 13:23:54 2017 +0200

    MAGETWO-85326: Tool for generating schema

commit 88ab84dcc336335ce331fb5f9ad8f30bef013991
Date:   Fri Dec 29 13:20:32 2017 +0200

    MAGETWO-85326: Tool for generating schema

orlangur · 2019-02-17T23:20:26Z

@hostep I believe MAGETWO-85326 just converted logic existed before.

hostep · 2019-02-18T08:01:53Z

So if we track down where the reindex in the upgrade script was introduced, it looks like it was done over here: dc19322

Over here it was just called at the end of the upgrade method.
Does somebody know if this method is always being triggered during setup:upgrade, even when no version is changed?
If it is only triggered when the version is changed, then it would probably be ok, because it will only reindex when the version is increased (even though it might not always be necessary).
The recurring data script however, will always execute during setup:upgrade, no matter if the version gets changed or not.
If the upgrade method does get executed even if the version hasn't changed, then the current situation is the same as before I'm guessing, but since nobody complained about it earlier, I'm not suspecting this to be the case.

dambrogia · 2019-02-18T22:11:15Z

Hi @hostep, chiming in a little bit here. I'm a lurker from #19469 and both an advocate of removing the recurring data script and finding out why it was put there in the first place to ensure it can safely be removed.

I am slowly but surely working on migrating from M1 -> M2 with a site that has around 500k users and experiencing > 1hr times consistently from setup:upgrade. That just isn't going to cut it for my team's automated deployments.

In regards to your question -- as stated in the Magento2 docs:

Magento executes your module’s recurring schema event class after every schema installation or upgrade stage. This class makes final modifications to the database schema after it has been installed or updated.

Would love to hear from @sereban as to why this is needed. And thank you @giacmir to inquiring within the commit.

For anyone who might be affected by this, a possible workaround might be to declare a preference for a custom RecurringData script through a di.xml file. Simply leave the install method empty in your custom class. This is untested, I'm not sure if you can overwrite Setup scripts, but it's worth a shot for someone in need.

app/code/YourVendor/YourPackage/etc/di.xml

<preference for="Magento\Customer\Setup\RecurringData" type="YourVendor\YourPackage\Setup\RecurringData" />

app/code/YourVendor/YourPackage/Setup/RecurringData.php

public function install(ModuleDataSetupInterface $setup, ModuleContextInterface $context)
{
    // empty
}

giacmir · 2019-02-19T08:23:36Z

While waiting for an "official" answer from the author I'll try to guess on why that code is in there.

In my opinion that reindex was added to ensure that if some setup, even from a module other than Magento_Customer changes something to the customer attributes in a way that can affect the grid, such grid is rebuilded and the change is immediately visible.

In general I agree with removal. The effect of this issue may not be much trouble with a low number of customers, but when you have hundred of thousand this can be a major flaw since it forces the whole website to stay offline for several minutes (and usually websites with this amount of customers have also lots of online visitors at any time) when the same operation could be performed in background with a manual or scheduled reindex.

Also, the customer grid is an admin only feature, and having customers to "pay" for an admin-only operation seems to be a wrong approach.

As a side note i would like to stress the importance of having meaningful commit messages. All those commits with the same message (also vague) aren't useful at all when there is the need to discover what happened to the code.

hostep · 2019-02-19T20:02:02Z

Ok, thanks for the feedback all.

I just now did some more testing, to see how UpgradeData scripts behave in Magento 2.2, and it turns out the upgrade method of such scripts only is called by bin/magento setup:upgrade when the version of the particular module is increased. So the upgrade method isn't called every single time you execute bin/magento setup:upgrade.

In contrast now, in Magento 2.3, where we have this declarative database schema and no longer seem to have UpgradeData scripts and there probably no longer is a similar logic where some code only gets executed when a version of a module is increased, because the purpose of this declarative schema is to get rid of these versions in the first place if I understand how they are supposed to work. Therefore it was probably chosen to put the script in a RecurringData script probably with the mindset not to break anything, because the author probably also wasn't able to figure out why it was there in the first place.

So now becomes the question, why was the reindexing added to upgrade method of the UpgradeData script in dc19322 (MAGETWO-40057)
Maybe @slavvka can help out here, hopfully he'll remember something from this as it was a pretty long time ago? :)
(As far as the history goes from that MAGETWO-40057 ticket, it looks - reading from the git log - that this was a part of MAGETWO-40058 which in itself was part of MAGETWO-38563 and all of those came together in one big merge commit: 9a960ca)

And like @giacmir says, it's probably completely unnecessary to execute this every single time when executing bin/magento setup:upgrade, reindexing should be handled in an asynchronous way.

In my opinion that reindex was added to ensure that if some setup, even from a module other than Magento_Customer changes something to the customer attributes in a way that can affect the grid, such grid is rebuilded and the change is immediately visible.

@giacmir: this probably isn't correct as far as I understand it, because the UpgradeData script only got executed when the Magento_Customer module's version was increased.

Conclusion from my part: I agree with removing this RecurringData script unless some strong arguments come up which explain why it is really necessary to keep it.

orlangur · 2019-02-20T16:30:05Z

@hostep,

no longer seem to have UpgradeData scripts and there probably no longer is a similar logic where some code only gets executed when a version of a module is increased

Then we can maybe add such call in the end of every separate patch existed before RecurringData introduction and remove RecurringData then?

giacmir · 2019-02-20T16:40:38Z

@orlangur I think that understanding why it was added in the first place is the key. Adding that to multiple patches means that in case of upgrades that span several patches the reindex will be triggered not one time but several times, worsening the effect.

orlangur · 2019-02-20T18:10:34Z

@giacmir but we will have flexibility to run schema updates before we got some 500k of customers. With recurring approach it is running each time.

slavvka · 2019-12-09T21:57:25Z

@magento run all tests

slavvka · 2019-12-10T22:29:42Z

Hey @slackerzz Could you please sign the CLA?

slavvka · 2019-12-10T23:28:13Z

Also please look at failing tests

slackerzz · 2019-12-11T07:39:48Z

@slavvka i've just signed the CLA
Static tests are failing due a @deprecated annotation not related to this PR

slavvka · 2019-12-11T15:36:22Z

Hey @slackerzz I know that Static test failure doesn't relate to your changes but since they touches the file with issues we are asking you to fix that as well to make the code better. This is how our static tests works, they check whole affected files

slackerzz · 2019-12-11T16:02:24Z

The test is failing because of a php code sniffer warning:

PHP Code Sniffer detected 1 violation(s): 

FILE: /var/www/html/app/code/Magento/Customer/Model/Customer.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 68 | WARNING | Motivation behind the added @deprecated tag MUST be
    |         | explained. @see tag MUST be used with reference to
    |         | new implementation when code is deprecated and there
    |         | is a new alternative.
----------------------------------------------------------------------

and refers to:

magento2/app/code/Magento/Customer/Model/Customer.php

Lines 64 to 68 in 5b829a8

    
               /** 
        
                * @deprecated 
        
                * @see AccountConfirmation::XML_PATH_IS_CONFIRM 
        
                */ 
        
               const XML_PATH_IS_CONFIRM = 'customer/create_account/confirm';

It is complaining about a @deprecated tag without a @see tag, but it's wrong.

How will you fix it?

ihor-sviziev · 2019-12-11T17:05:38Z

The test is failing because of a php code sniffer warning:
PHP Code Sniffer detected 1 violation(s): 

FILE: /var/www/html/app/code/Magento/Customer/Model/Customer.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 68 | WARNING | Motivation behind the added @deprecated tag MUST be
    |         | explained. @see tag MUST be used with reference to
    |         | new implementation when code is deprecated and there
    |         | is a new alternative.
----------------------------------------------------------------------
and refers to:

magento2/app/code/Magento/Customer/Model/Customer.php

Lines 64 to 68 in 5b829a8

/**

* @deprecated

* @see AccountConfirmation::XML_PATH_IS_CONFIRM

*/

const XML_PATH_IS_CONFIRM = 'customer/create_account/confirm';

It is complaining about a @deprecated tag without a @see tag, but it's wrong.

How will you fix it?

@slackerzz
It looks like false positive. I reported issue for that: magento/magento-coding-standard#162

Could you just add to ignore it?

slavvka · 2019-12-11T19:53:02Z

@slackerzz I fixed that

slavvka · 2019-12-13T17:35:41Z

@slackerzz There's a degradation in Performance tests (Tideways EE Measurements):

Customer Save (Admin Customer Management): 100% (mainline = 0 | team = 1) -> DEGRADATION

   SQLs were removed from mainline: 
      
   SQLs were added to team: 
       DELETE FROM `customer_grid_flat` WHERE (entity_id IN('1'))
Create customer (Catalog GraphQL): 100% (mainline = 0 | team = 2) -> DEGRADATION

   SQLs were removed from mainline: 
      
   SQLs were added to team: 
       DELETE FROM `customer_grid_flat` WHERE (entity_id IN('1202'))
       DELETE FROM `customer_grid_flat` WHERE (entity_id IN('1202'))

tideways-mysql-insert-requests-count
Customer Save (Admin Customer Management): 33.3% (mainline = 3 | team = 4) -> DEGRADATION

   SQLs were removed from mainline: 
      
   SQLs were added to team: 
       INSERT  INTO `customer_grid_flat` (`entity_id`,`name`,`email`,`group_id`,`created_at`,`website_id`,`confirmation`,`created_in`,`dob`,`gender`,`taxvat`,`lock_expires`,`shipping_full`,`billing_full`,`billing_firstname`,`billing_lastname`,`billing_telephone`,`billing_postcode`,`billing_country_id`,`billing_region`,`billing_street`,`billing_city`,`billing_fax`,`billing_vat_id`,`billing_company`) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON DUPLICATE KEY UPDATE `name` = VALUES(`name`), `email` = VALUES(`email`), `group_id` = VALUES(`group_id`), `created_at` = VALUES(`created_at`), `website_id` = VALUES(`website_id`), `confirmation` = VALUES(`confirmation`), `created_in` = VALUES(`created_in`), `dob` = VALUES(`dob`), `gender` = VALUES(`gender`), `taxvat` = VALUES(`taxvat`), `lock_expires` = VALUES(`lock_expires`), `shipping_full` = VALUES(`shipping_full`), `billing_full` = VALUES(`billing_full`), `billing_firstname` = VALUES(`billing_firstname`), `billing_lastname` = VALUES(`billing_lastname`), `billing_telephone` = VALUES(`billing_telephone`), `billing_postcode` = VALUES(`billing_postcode`), `billing_country_id` = VALUES(`billing_country_id`), `billing_region` = VALUES(`billing_region`), `billing_street` = VALUES(`billing_street`), `billing_city` = VALUES(`billing_city`), `billing_fax` = VALUES(`billing_fax`), `billing_vat_id` = VALUES(`billing_vat_id`), `billing_company` = VALUES(`billing_company`)
Create customer (Catalog GraphQL): 200% (mainline = 1 | team = 3) -> DEGRADATION

   SQLs were removed from mainline: 
      
   SQLs were added to team: 
       INSERT  INTO `customer_grid_flat` (`entity_id`,`name`,`email`,`group_id`,`created_at`,`website_id`,`confirmation`,`created_in`,`dob`,`gender`,`taxvat`,`lock_expires`,`shipping_full`,`billing_full`,`billing_firstname`,`billing_lastname`,`billing_telephone`,`billing_postcode`,`billing_country_id`,`billing_region`,`billing_street`,`billing_city`,`billing_fax`,`billing_vat_id`,`billing_company`) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON DUPLICATE KEY UPDATE `name` = VALUES(`name`), `email` = VALUES(`email`), `group_id` = VALUES(`group_id`), `created_at` = VALUES(`created_at`), `website_id` = VALUES(`website_id`), `confirmation` = VALUES(`confirmation`), `created_in` = VALUES(`created_in`), `dob` = VALUES(`dob`), `gender` = VALUES(`gender`), `taxvat` = VALUES(`taxvat`), `lock_expires` = VALUES(`lock_expires`), `shipping_full` = VALUES(`shipping_full`), `billing_full` = VALUES(`billing_full`), `billing_firstname` = VALUES(`billing_firstname`), `billing_lastname` = VALUES(`billing_lastname`), `billing_telephone` = VALUES(`billing_telephone`), `billing_postcode` = VALUES(`billing_postcode`), `billing_country_id` = VALUES(`billing_country_id`), `billing_region` = VALUES(`billing_region`), `billing_street` = VALUES(`billing_street`), `billing_city` = VALUES(`billing_city`), `billing_fax` = VALUES(`billing_fax`), `billing_vat_id` = VALUES(`billing_vat_id`), `billing_company` = VALUES(`billing_company`)
       INSERT  INTO `customer_grid_flat` (`entity_id`,`name`,`email`,`group_id`,`created_at`,`website_id`,`confirmation`,`created_in`,`dob`,`gender`,`taxvat`,`lock_expires`,`shipping_full`,`billing_full`,`billing_firstname`,`billing_lastname`,`billing_telephone`,`billing_postcode`,`billing_country_id`,`billing_region`,`billing_street`,`billing_city`,`billing_fax`,`billing_vat_id`,`billing_company`) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON DUPLICATE KEY UPDATE `name` = VALUES(`name`), `email` = VALUES(`email`), `group_id` = VALUES(`group_id`), `created_at` = VALUES(`created_at`), `website_id` = VALUES(`website_id`), `confirmation` = VALUES(`confirmation`), `created_in` = VALUES(`created_in`), `dob` = VALUES(`dob`), `gender` = VALUES(`gender`), `taxvat` = VALUES(`taxvat`), `lock_expires` = VALUES(`lock_expires`), `shipping_full` = VALUES(`shipping_full`), `billing_full` = VALUES(`billing_full`), `billing_firstname` = VALUES(`billing_firstname`), `billing_lastname` = VALUES(`billing_lastname`), `billing_telephone` = VALUES(`billing_telephone`), `billing_postcode` = VALUES(`billing_postcode`), `billing_country_id` = VALUES(`billing_country_id`), `billing_region` = VALUES(`billing_region`), `billing_street` = VALUES(`billing_street`), `billing_city` = VALUES(`billing_city`), `billing_fax` = VALUES(`billing_fax`), `billing_vat_id` = VALUES(`billing_vat_id`), `billing_company` = VALUES(`billing_company`)

tideways-mysql-select-requests-count
Create customer (Catalog GraphQL): 16.7% (mainline = 42 | team = 49) -> DEGRADATION

   SQLs were removed from mainline: 
      
   SQLs were added to team: 
       SELECT `main_table`.`attribute_id`, `main_table`.`entity_type_id`, `main_table`.`attribute_code`, `main_table`.`attribute_model`, `main_table`.`backend_model`, `main_table`.`backend_type`, `main_table`.`backend_table`, `main_table`.`frontend_model`, `main_table`.`frontend_input`, `main_table`.`frontend_label`, `main_table`.`frontend_class`, `main_table`.`source_model`, `main_table`.`is_required`, `main_table`.`is_user_defined`, `main_table`.`default_value`, `main_table`.`is_unique`, `main_table`.`note`, `additional_table`.`is_visible`, `additional_table`.`input_filter`, `additional_table`.`multiline_count`, `additional_table`.`validate_rules`, `additional_table`.`is_system`, `additional_table`.`sort_order`, `additional_table`.`data_model`, `additional_table`.`is_used_in_grid`, `additional_table`.`is_visible_in_grid`, `additional_table`.`is_filterable_in_grid`, `additional_table`.`is_searchable_in_grid`, `additional_table`.`is_used_for_customer_segment`, `scope_table`.`website_id` AS `scope_website_id`, `scope_table`.`is_visible` AS `scope_is_visible`, `scope_table`.`is_required` AS `scope_is_required`, `scope_table`.`default_value` AS `scope_default_value`, `scope_table`.`multiline_count` AS `scope_multiline_count` FROM `eav_attribute` AS `main_table` INNER JOIN `customer_eav_attribute` AS `additional_table` ON additional_table.attribute_id = main_table.attribute_id LEFT JOIN `customer_eav_attribute_website` AS `scope_table` ON scope_table.attribute_id = main_table.attribute_id AND scope_table.website_id = :scope_website_id WHERE (main_table.entity_type_id = :mt_entity_type_id)
       SELECT COUNT(*) FROM `customer_entity` AS `e` WHERE (((`e`.`entity_id` = '1202')))
       SELECT COUNT(*) FROM `customer_entity` AS `e` WHERE (((`e`.`entity_id` = '1202')))
       SELECT `e`.`entity_id`, TRIM(CONCAT_WS(' ', IF(`e`.`prefix` <> '', `e`.`prefix`, NULL), IF(`e`.`firstname` <> '', `e`.`firstname`, NULL), IF(`e`.`middlename` <> '', `e`.`middlename`, NULL), IF(`e`.`lastname` <> '', `e`.`lastname`, NULL), IF(`e`.`suffix` <> '', `e`.`suffix`, NULL))) AS `name`, `e`.`email`, `e`.`group_id`, `e`.`created_at`, `e`.`website_id`, `e`.`confirmation`, `e`.`created_in`, `e`.`dob`, `e`.`gender`, `e`.`taxvat`, `e`.`lock_expires`, TRIM(CONCAT_WS(' ', IF(`shipping`.`street` <> '', `shipping`.`street`, NULL), IF(`shipping`.`city` <> '', `shipping`.`city`, NULL), IF(`shipping`.`region` <> '', `shipping`.`region`, NULL), IF(`shipping`.`postcode` <> '', `shipping`.`postcode`, NULL))) AS `shipping_full`, TRIM(CONCAT_WS(' ', IF(`billing`.`street` <> '', `billing`.`street`, NULL), IF(`billing`.`city` <> '', `billing`.`city`, NULL), IF(`billing`.`region` <> '', `billing`.`region`, NULL), IF(`billing`.`postcode` <> '', `billing`.`postcode`, NULL))) AS `billing_full`, `billing`.`firstname` AS `billing_firstname`, `billing`.`lastname` AS `billing_lastname`, `billing`.`telephone` AS `billing_telephone`, `billing`.`postcode` AS `billing_postcode`, `billing`.`country_id` AS `billing_country_id`, `billing`.`region` AS `billing_region`, `billing`.`street` AS `billing_street`, `billing`.`city` AS `billing_city`, `billing`.`fax` AS `billing_fax`, `billing`.`vat_id` AS `billing_vat_id`, `billing`.`company` AS `billing_company` FROM `customer_entity` AS `e` LEFT JOIN `customer_address_entity` AS `shipping` ON shipping.entity_id=e.default_shipping LEFT JOIN `customer_address_entity` AS `billing` ON billing.entity_id=e.default_billing WHERE (((`e`.`entity_id` = '1202'))) LIMIT 10000
       SELECT COUNT(*) FROM `customer_entity` AS `e` WHERE (((`e`.`entity_id` = '1202')))
       SELECT COUNT(*) FROM `customer_entity` AS `e` WHERE (((`e`.`entity_id` = '1202')))
       SELECT `e`.`entity_id`, TRIM(CONCAT_WS(' ', IF(`e`.`prefix` <> '', `e`.`prefix`, NULL), IF(`e`.`firstname` <> '', `e`.`firstname`, NULL), IF(`e`.`middlename` <> '', `e`.`middlename`, NULL), IF(`e`.`lastname` <> '', `e`.`lastname`, NULL), IF(`e`.`suffix` <> '', `e`.`suffix`, NULL))) AS `name`, `e`.`email`, `e`.`group_id`, `e`.`created_at`, `e`.`website_id`, `e`.`confirmation`, `e`.`created_in`, `e`.`dob`, `e`.`gender`, `e`.`taxvat`, `e`.`lock_expires`, TRIM(CONCAT_WS(' ', IF(`shipping`.`street` <> '', `shipping`.`street`, NULL), IF(`shipping`.`city` <> '', `shipping`.`city`, NULL), IF(`shipping`.`region` <> '', `shipping`.`region`, NULL), IF(`shipping`.`postcode` <> '', `shipping`.`postcode`, NULL))) AS `shipping_full`, TRIM(CONCAT_WS(' ', IF(`billing`.`street` <> '', `billing`.`street`, NULL), IF(`billing`.`city` <> '', `billing`.`city`, NULL), IF(`billing`.`region` <> '', `billing`.`region`, NULL), IF(`billing`.`postcode` <> '', `billing`.`postcode`, NULL))) AS `billing_full`, `billing`.`firstname` AS `billing_firstname`, `billing`.`lastname` AS `billing_lastname`, `billing`.`telephone` AS `billing_telephone`, `billing`.`postcode` AS `billing_postcode`, `billing`.`country_id` AS `billing_country_id`, `billing`.`region` AS `billing_region`, `billing`.`street` AS `billing_street`, `billing`.`city` AS `billing_city`, `billing`.`fax` AS `billing_fax`, `billing`.`vat_id` AS `billing_vat_id`, `billing`.`company` AS `billing_company` FROM `customer_entity` AS `e` LEFT JOIN `customer_address_entity` AS `shipping` ON shipping.entity_id=e.default_shipping LEFT JOIN `customer_address_entity` AS `billing` ON billing.entity_id=e.default_billing WHERE (((`e`.`entity_id` = '1202'))) LIMIT 10000

giacmir · 2019-12-16T08:54:43Z

Hello @slavvka,
This pull request was originally meant to avoid downtime during a website deploy by removing a potentially long running and nonsensical operation during setup:upgrade.

BUT, digging deeper into the code was discovered that:

Customer Grid Indexer can't work in 'Update on Schedule' mode. All data for indexer must be updated in realtime

during save/update customer data(including address data)."

(see dev/tests/integration/testsuite/Magento/Customer/Model/ResourceModel/Grid/CollectionTest.php)

and this requirements were actually not met in the current implementation because the indexer invalidation is made only if the indexer was valid, but in a place where the indexer status is not trustable. (in an after save situation, so I have actually updated a customer, but the indexer in still valid because no-one has invalidated it yet). So the choice was to ask for a reindex without checking the initial indexer condition.

Said that, the detected queries were to be expected as they actually were missing before, and due now.

ihor-sviziev · 2019-12-17T09:21:21Z

Hi @slackerzz @giacmir,

magento2/dev/tests/integration/testsuite/Magento/Customer/Model/ResourceModel/Grid/CollectionTest.php

Lines 31 to 36 in dc7f483

    
               /** 
        
                * Test updated data for customer grid indexer during save/update customer data(including address data) 
        
                * in 'Update on Schedule' mode. 
        
                * 
        
                * Customer Grid Indexer can't work in 'Update on Schedule' mode. All data for indexer must be updated in realtime 
        
                * during save/update customer data(including address data).

Really good finding, but could you add more details how reindex was executed earlier, as grid was up to date?

slackerzz · 2019-12-20T11:38:27Z

We have a big problem as described in #19469. Each deploy you end up with 10 minutes or more of downtime. It was hard to explain to the merchants.
I'm sure that this problem doesn't appear in dev box with sample data, but you know, we use this software in production.
This issue was opened more than a year ago.

When i first faced this issue i resolved with a composer patch that you can find here and i thought that it was a good idea to share it with others by submitting a pull request.

Now, after 10 months with 15 participants, 5 reviewers and 77 comments we are still arguing about 30 lines of code.

m2-assistant · 2019-12-20T11:38:29Z

Hi @slackerzz, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Remove Magento/Customer/Setup/RecurringData.php

7e3448e

magento-engcom-team added Component: Customer Release Line: 2.3 Partner: MageSpecialist partners-contribution Pull Request is created by Magento Partner labels Feb 14, 2019

orlangur self-assigned this Feb 14, 2019

orlangur requested changes Feb 14, 2019

View reviewed changes

slackerzz closed this Feb 15, 2019

slackerzz deleted the remove_magento_customer_recurring_data branch February 15, 2019 12:49

slackerzz restored the remove_magento_customer_recurring_data branch February 15, 2019 16:42

hostep reopened this Feb 15, 2019

sidolov added Release Line: 2.4 and removed Release Line: 2.3 labels Dec 5, 2019

magento-engcom-team assigned slavvka Dec 9, 2019

Merge branch '2.4-develop' into remove_magento_customer_recurring_data

2b8599a

slavvka requested a review from paliarush as a code owner December 10, 2019 21:00

Merge branch '2.4-develop' into remove_magento_customer_recurring_data

5b829a8

slavvka added Progress: needs update and removed Requires status update labels Dec 11, 2019

ihor-sviziev mentioned this pull request Dec 11, 2019

@deprecated without @see tag false positive magento/magento-coding-standard#162

Closed

Update Customer.php

d6a017a

slavvka removed the Progress: needs update label Dec 11, 2019

slavvka approved these changes Dec 11, 2019

View reviewed changes

slavvka added Progress: needs update and removed Progress: accept labels Dec 13, 2019

VladimirZaets added the help wanted label Dec 16, 2019

slackerzz closed this Dec 20, 2019

ihor-sviziev mentioned this pull request Dec 24, 2019

Invalidate indexers in RecurringData #26163

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Magento/Customer/Setup/RecurringData.php #21235

Remove Magento/Customer/Setup/RecurringData.php #21235

slackerzz commented Feb 14, 2019 •

edited by sdzhepa

magento-engcom-team commented Feb 14, 2019

orlangur left a comment

slackerzz commented Feb 15, 2019 •

edited

orlangur commented Feb 15, 2019

slackerzz commented Feb 15, 2019

orlangur commented Feb 15, 2019

slackerzz commented Feb 15, 2019

ghost commented Feb 15, 2019

hostep commented Feb 15, 2019 •

edited

Nazar65 commented Feb 15, 2019

Nazar65 commented Feb 15, 2019

hostep commented Feb 15, 2019 •

edited

orlangur commented Feb 17, 2019

hostep commented Feb 18, 2019

dambrogia commented Feb 18, 2019

giacmir commented Feb 19, 2019

hostep commented Feb 19, 2019

orlangur commented Feb 20, 2019

giacmir commented Feb 20, 2019

orlangur commented Feb 20, 2019

slavvka commented Dec 9, 2019

slavvka commented Dec 10, 2019

slavvka commented Dec 10, 2019

slackerzz commented Dec 11, 2019

slavvka commented Dec 11, 2019

slackerzz commented Dec 11, 2019

ihor-sviziev commented Dec 11, 2019 •

edited

slavvka commented Dec 11, 2019

slavvka commented Dec 13, 2019

giacmir commented Dec 16, 2019

ihor-sviziev commented Dec 17, 2019

slackerzz commented Dec 20, 2019

m2-assistant bot commented Dec 20, 2019

Remove Magento/Customer/Setup/RecurringData.php #21235

Remove Magento/Customer/Setup/RecurringData.php #21235

Conversation

slackerzz commented Feb 14, 2019 • edited by sdzhepa

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

magento-engcom-team commented Feb 14, 2019

orlangur left a comment

Choose a reason for hiding this comment

slackerzz commented Feb 15, 2019 • edited

orlangur commented Feb 15, 2019

slackerzz commented Feb 15, 2019

orlangur commented Feb 15, 2019

slackerzz commented Feb 15, 2019

ghost commented Feb 15, 2019

hostep commented Feb 15, 2019 • edited

Nazar65 commented Feb 15, 2019

Nazar65 commented Feb 15, 2019

hostep commented Feb 15, 2019 • edited

orlangur commented Feb 17, 2019

hostep commented Feb 18, 2019

dambrogia commented Feb 18, 2019

giacmir commented Feb 19, 2019

hostep commented Feb 19, 2019

orlangur commented Feb 20, 2019

giacmir commented Feb 20, 2019

orlangur commented Feb 20, 2019

slavvka commented Dec 9, 2019

slavvka commented Dec 10, 2019

slavvka commented Dec 10, 2019

slackerzz commented Dec 11, 2019

slavvka commented Dec 11, 2019

slackerzz commented Dec 11, 2019

ihor-sviziev commented Dec 11, 2019 • edited

slavvka commented Dec 11, 2019

slavvka commented Dec 13, 2019

giacmir commented Dec 16, 2019

ihor-sviziev commented Dec 17, 2019

slackerzz commented Dec 20, 2019

m2-assistant bot commented Dec 20, 2019

slackerzz commented Feb 14, 2019 •

edited by sdzhepa

slackerzz commented Feb 15, 2019 •

edited

hostep commented Feb 15, 2019 •

edited

hostep commented Feb 15, 2019 •

edited

ihor-sviziev commented Dec 11, 2019 •

edited