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

Stop using refresh_date in civicrm_group table #19287

Merged
merged 1 commit into from
Jan 7, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Dec 30, 2020

Overview

Stop using refresh_date in civicrm_group table

Before

We use a mix of cache_date & refresh_date but refresh date doesn't seem to confer any extra value

After

We just use cache_date

Technical Details

I was looking to add indexes to civicrm_group.cache_date & civicrm_group.refresh_date - but I couldn't figure out why
the latter exists / is used. I went through the places it's used and it is simply a calculated version of
cache_date + smartGroupCacheTime and I can't see any value.

I can follow up on this with a removal if no-one else can see the point of it

Comments

@civibot
Copy link

civibot bot commented Dec 30, 2020

(Standard links)

@civibot civibot bot added the master label Dec 30, 2020
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Dec 30, 2020
Per civicrm#19287 I also considered an index on refresh_date but
I couldn't see any evidence the field is useful
I was looking to add indexes to civicrm_group.cache_date & civicrm_group.refresh_date - but I couldn't figure out why
the latter exists / is used. I went through the places it's used and it is simply a calculated version of
cache_date + smartGroupCacheTime and I can't see any value.

I can follow up on this with a removal if no-one else can see the point of it
@demeritcowboy
Copy link
Contributor

Was there some problem about groups and the log table getting really big and I sort of remember it being date-related in some way. Sorry don't have anything specific, but maybe that's what it's for? Does that sound familiar?

@mattwire
Copy link
Contributor

@eileenmcnaughton I think you are probably right and we don't need both

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy I can only guess that it was because people originally were doing an unindexed query against cache_date & so people thought that adding refresh_date would avoid that - of course doing the date calc in php does that too....

@eileenmcnaughton
Copy link
Contributor Author

thanks @mattwire - I suspect if you & @demeritcowboy can't find a reason for the field it doesn't exist

@demeritcowboy
Copy link
Contributor

This seems like what I was thinking of: https://lab.civicrm.org/dev/core/-/issues/449. It's not related there were just several mentions of cache_date and refresh_date.

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy I can see why it would have traumatised you :-)

@mattwire mattwire merged commit ed89dcb into civicrm:master Jan 7, 2021
@eileenmcnaughton eileenmcnaughton deleted the group2 branch January 7, 2021 20:24
colemanw added a commit to colemanw/civicrm-core that referenced this pull request May 26, 2023
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.

3 participants