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

get log date from tables available in query with data instead of last table #18868

Merged
merged 1 commit into from
Nov 6, 2020

Conversation

sunilpawar
Copy link
Contributor

Overview

When Logging feature is enabled and CiviCRM have Millions of records for contact, membership, contribution , entity tag, custom fields etc..., so in log tables, each contact have thousand of change log entries.

When we visit contact Change Log tab and mouse over the update icon, civicrm will send log connection id to report to find out the difference.

CiviCRM collect all tables having reference with civicrm_contact tables. In this list, last tables is civicrm_entity_tag

To to fetch the log date value, civicrm uses last table civicrm_entity_tag, but there are possibility that, change log may not exist for that contact using specific connection id, but to find out value, civicrm iterate entire table with out result.

Then ultimately , either we get no log change or database error or server timeout.

Instead of checking last table values, we should get table from Query we prepared with all table using union

$tables = Array
(
    [0] => civicrm_contact
    [2] => civicrm_batch
    [4] => civicrm_financial_account
    [5] => civicrm_financial_item
    [6] => civicrm_campaign
    [7] => civicrm_survey
    [8] => civicrm_event_carts
    [9] => civicrm_dedupe_exception
    [10] => civicrm_grant
    [11] => civicrm_pcp
    [12] => civicrm_custom_group
    [13] => civicrm_domain
    [14] => civicrm_email
    [15] => civicrm_file
    [16] => civicrm_im
    [18] => civicrm_note
    [19] => civicrm_phone
    [20] => civicrm_tag
    [21] => civicrm_uf_match
    [22] => civicrm_openid
    [23] => civicrm_website
    [24] => civicrm_setting
    [25] => civicrm_print_label
    [27] => civicrm_group
    [28] => civicrm_subscription_history
    [30] => civicrm_group_organization
    [32] => civicrm_contribution_page
    [57] => civicrm_account_contact
    [58] => civicrm_stripe_paymentintent
    [59] => civicrm_value_summary_field_7
    [66] => civicrm_entity_tag
)

Before

log date were get from civicrm_entity_tag table only

After

Now log date get from civicrm_contact first, if not found , then next table get used.

@civibot
Copy link

civibot bot commented Oct 28, 2020

(Standard links)

@civibot civibot bot added the master label Oct 28, 2020
@sunilpawar sunilpawar changed the title correct query to log_date for log_conn_id get log date from tables available in query with data instead of last table Oct 28, 2020
@eileenmcnaughton
Copy link
Contributor

@sunilpawar @VangelisP perhaps you could trade review with this & #18851

@VangelisP
Copy link
Contributor

Hi @sunilpawar , what you mention makes total sense!

In fact it will make even more sense if we combine it with something that I am working on at the moment, to be able to drop some unnecessary triggers from some tables that someone might not want to monitor with advanced logging, for example: civicrm_mailing, civicrm_activity and even perhaps civicrm_entity_tag !

In regards to your issue, specifically:

Then ultimately, either we get no log change or database error or server timeout.

for your specific issue, I have made this PR #18851 that should render a preview without those dreaded timeouts (we had the same as well!)

However, as I completely missed the mouseover action, your issue brings me an idea to remove the pager from the mouseover if the layout is 'overlay' (but that should go to PR 18851)

@demeritcowboy
Copy link
Contributor

demeritcowboy commented Nov 6, 2020

This makes sense to me and runs fine. It's possible it was even originally a copy/paste typo to put table instead of dao->table_name. $table is arguably out of scope at that point in the code, but php lets you do that.

I didn't test the performance improvement, just ran it and code review.

@eileenmcnaughton
Copy link
Contributor

Merging based on @demeritcowboy's review

One thing to be careful of is the Logging.revert api is not impacted - however that does not appear to call this function

@eileenmcnaughton eileenmcnaughton merged commit 1930897 into civicrm:master Nov 6, 2020
@demeritcowboy
Copy link
Contributor

Good point - I don't usually think about that.

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.

4 participants