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.

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

Dedupe performance - hard-remove financial_item from list of dymnamic refs to contact table #17567

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jun 10, 2020

Overview

Rc fix for performance regression in deduping - note I want to see if we can do a better fix onn master

Before

SQL runs:

select id FROM civicrm_financial_item WHERE entity_table = 'civicrm_contact'; to see if the table is a dedupe candidate.

However, the index on this table does not cover the where clause & as this is a large table it gets slow

After

Query hacked out of the list

Technical Details

For the 'real' fix on master I think we want a schema for this - ie something like

--- a/xml/schema/Financial/FinancialItem.xml
+++ b/xml/schema/Financial/FinancialItem.xml
@@ -130,9 +130,11 @@
     <name>entity_table</name>
     <type>varchar</type>
     <title>Entity Table</title>
+    <required>true</required>
     <length>64</length>
     <comment>The table providing the source of this item such as civicrm_line_item</comment>
     <add>4.3</add>
+    <validTables>civicrm_line_item, civicrm_financial_trxn</validTables>
   </field>
   <field>
     <name>entity_id</name>

Note there is a previous pointer to this approach

* TODO: filter dynamic entity references on the $tableName, based on

Comments

@totten @seamuslee001 @colemanw a couple of questions

  1. How do we want the xml to look for specifying valid tables for civicrm_entity
  2. Does this need to be in regen / go into the DAO ?
  3. Should it be hookabale?

@civibot
Copy link

civibot bot commented Jun 10, 2020

(Standard links)

@civibot civibot bot added the 5.27 label Jun 10, 2020
@@ -2401,7 +2401,6 @@ public static function getReferencesToTable($tableName) {
$refsFound = [];
foreach (CRM_Core_DAO_AllCoreTables::getClasses() as $daoClassName) {
$links = $daoClassName::getReferenceColumns();
$daoTableName = $daoClassName::getTableName();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused

@eileenmcnaughton
Copy link
Contributor Author

Just thinking on the 'right fix' - perhaps DAO generate should put an array rather than NULL into the table, if in the xml

Screen Shot 2020-06-11 at 11 53 22 AM

@eileenmcnaughton
Copy link
Contributor Author

SO I'm thinking that we

  1. agree an xml syntax for allowed tables
  2. update the DAO generator to put an array of tables rather than NULL
  3. fix Core_Reference_Dymnamic to cope with an array if present

@totten are you OK with ^^

@seamuslee001
Copy link
Contributor

We discussed this in core team meeting and Tim was ok with this as an interim solution merging

@seamuslee001 seamuslee001 merged commit c2cc5b8 into civicrm:5.27 Jun 11, 2020
@seamuslee001 seamuslee001 deleted the dupe_speed branch June 11, 2020 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants