dev/core#663 - Use InnoDB engine for extended log tables #14256
Conversation
|
(Standard links)
|
There was a problem hiding this comment.
I see in other parts of this PR that similar issues with potentially confusing naming exist (eg createLogTableFor() would be createDataLoggingTableFor() in my perfectly coloured bikeshed), so please consider my comments only in the scope of new additions I guess?
CRM/Logging/Schema.php
Outdated
| @@ -302,7 +302,7 @@ public function fixSchemaDifferences($enableLogging = FALSE) { | |||
| * and also implements the engine change defined by the hook (i.e. INNODB). | |||
| * | |||
| * Note changing engine & adding hook-defined indexes, but not changing back | |||
| * to ARCHIVE if engine has not been deliberately set (by hook) and not dropping | |||
| * to INNODB if engine has not been deliberately set (by hook) and not dropping | |||
| * indexes. Sysadmin will need to manually intervene to revert to defaults. | |||
| */ | |||
| public function updateLogTableSchema() { | |||
There was a problem hiding this comment.
Can I suggest that here and in the naming of the API call System.updatelogtables, we are explicit about what this actually does? Eg convertDataLoggingTablesFromArchiveToInnoDB() is explicit, where "updateLogTables" could mean many things.
- In future there will be other updates, and a function named like this will occupy a namingspace that also fits other functions.
- Who wouldn't run a function called "update"? Sounds reasonable, but as @Stoob points out, this process has an impact on a functioning site.
- CiviCRM has a pattern of recycling terminology in multiple spaces (I count six different meanings for "log"), I think it's helpful for both DX and UX if we refer to things using clear terminology.
There was a problem hiding this comment.
Comment above was written thinking you'd added the function in this PR - on second view this need not block your proposed changes (but please consider it where adding new methods etc).
There was a problem hiding this comment.
I agree that the current naming is confusing - I've spent time in this code in the past, but I still get System.updatelogtables mixed up with System.createmissinglogtables all the time.
What makes it a bit tricky is that, in addition to changing the engine schema, the function/API will potentially also do the following things:
- Change the column definition of
log_conn_id - Create any missing indexes (which may have been added via
hook_civicrm_alterLogTables)
I'm okay with renaming the existing API if we can come up with a less confusing name for the API that encapsulates all of that.
pfigel
force-pushed
the
innodb-log-tables
branch
3 times, most recently
from
6bdff20 to
adf428a
Compare
|
@pfigel I feel like we would normally chunk things like this over a couple of releases with one release just changing the default behaviour for new installs / new enable loggings & the next chunk addressing existing sites |
This updates the detailed logging documentation to reflect the changes being made in core in civicrm/civicrm-core#14256. Additionally, it adds some notes on log table migration and adds a reference to a new extension allowing for more flexible configuration of log tables.
This changes the default storage engine used for log tables to InnoDB in order to provide safer defaults. Existing log tables are not upgraded unless explicitly requested using the new forceEngineMigration System.updatelogtables API parameter. Hook overrides are respected.
pfigel
changed the title
|
@eileenmcnaughton I've extracted the system check/upgrade prompt for a future PR and changed This is now ready for review. I've opened a corresponding doc update in civicrm/civicrm-sysadmin-guide#167. |
|
@Stoob ping |
|
This is good from a code point of view - I made minor comments - @Stoob is going to try to r-run it & if that is good we can merge |
This adds API defaults for the updateChangedEngineConfig and forceEngineMigration parameters of System.updatelogtables and removes some fallback code that's unnecessary now.
|
@eileenmcnaughton @Stoob I added API defaults for |
|
@pfigel If we're already using https://github.com/eileenmcnaughton/nz.co.fuzion.innodbtriggers and have already converted our log tables to innodb then applying this patch and disabling that extension should be fine? |
|
@mattwire generally yes, although this change does not enable InnoDB compression ( The behaviour would be like this:
New tables are generally any tables in the |
|
I have tested this and confirmed that without this change log tables are created as Archive, with this change they are created with INNODB, i have also confirmed that this change by default doesn't change the log tables themselves without the |
|
Yay - Archive is on the way out! |
|
My test site successfully implemented nz.co.fuzion.innodbtriggers on about 150k records. I'm eager to give this patch a try out too. Thanks for this! I can rebuild my dev site if needed @eileenmcnaughton |
|
@Stoob so I think this uses INNODB by default - but I don't think it adds any indexes by default - which you'd likely want for 150k records - so nz.co.fuzion.innodbtriggers may still make sense for you |
|
Ok, thanks for the clarification @eileenmcnaughton. So the patch makes the table engine conversion, but does not add indexes. The extension adds indexes. For large installs, the extension adds value. |
|
@Stoob yes that is my understanding |
This updates the detailed logging documentation to reflect the changes made in civicrm/civicrm-core#14256. Additionally, it adds some notes on log table migration and adds a reference to a new extension allowing for more flexible configuration of log tables.
This updates the detailed logging documentation to reflect the changes made in civicrm/civicrm-core#14256. Additionally, it adds some notes on log table migration and adds a reference to a new extension allowing for more flexible configuration of log tables.
Overview
CiviCRM's optional detailed logging feature creates a log table to mirror every table in the
civicrm_%namespace. To preserve disk space, log tables use theARCHIVEengine by default.ARCHIVEhas a number of limitations, most importantly:UPDATEorDELETEThis has lead to a number of performance, reliability and durability issues in the past. It is possible to use different storage engines by installing extensions, but CiviCRM should offer sane and safe defaults.
Before
Detailed log tables use the
ARCHIVEengine if available.After
Detailed log tables on new installations use the
InnoDBengine by default. Log tables created at some point after this version was installed also use theInnoDBengine.Existing log tables will continue to use the
ARCHIVEtable.Site administrators may convert existing log tables using the
System.updatelogtablesAPI withforceEngineMigrationset.Technical Details
Some tests were added to cover the new behaviour. Migration code will not trigger if an engine was set by the hook, so
ARCHIVEcould still be used that way.Comments
An update for the sysadmin guide was prepared in civicrm/civicrm-sysadmin-guide#167, including notes on migration and a way to restore the old default (should anyone want to do that). It assumes we're targeting 5.16 for this.