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.

Sign up for GitHub

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

SearchKit - Refresh DB entities via table-swap #31767

Merged
merged 1 commit into from
Jan 15, 2025
Merged

SearchKit - Refresh DB entities via table-swap #31767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions ext/search_kit/Civi/Api4/Action/SKEntity/Refresh.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,40 @@ public function _run(Result $result) {
return;
}

// Build a new table with full data. Swap-in the new table and drop the old one.
//
// NOTE: This protocol destroys inbound FKs. But the prior protocol (TRUNCATE + INSERT SELECT)
// also destroyed inbound FKs. To keep inbound FKs, you would probably wind up working on
// something more incremental. (Maybe put new data into TEMPORARY table - and use INSERT/DELETE/UPDATE
// to sync to the real table. But that requires guaranteeing the presence of a stable PK column(s),
// and it would change the default ordering over time.)

// Prepare a sketch of the process. Ensure metadata is well-formed.
$query = (new SKEntityGenerator())->createQuery($display['saved_search_id.api_entity'], $display['saved_search_id.api_params'], $display['settings']);
$sql = $query->getSql();
$tableName = _getSearchKitDisplayTableName($displayName);
$finalTable = _getSearchKitDisplayTableName($displayName);
$columnSpecs = array_column($display['settings']['columns'], 'spec');
$columns = implode(', ', array_column($columnSpecs, 'name'));
\CRM_Core_DAO::executeQuery("TRUNCATE TABLE `$tableName`");
\CRM_Core_DAO::executeQuery("INSERT INTO `$tableName` ($columns) $sql");
$newTable = \CRM_Utils_SQL_TempTable::build()->setDurable()->setAutodrop(FALSE)->getName();
$junkTable = \CRM_Utils_SQL_TempTable::build()->setDurable()->setAutodrop(FALSE)->getName();

// Only one process should actually refresh this entity (at a given time).
$lock = \Civi::lockManager()->acquire("data.skentity." . $display['id'], 1);
if (!$lock->isAcquired()) {
throw new \Civi\Search\Exception\RefreshInProgressException(sprintf('Refresh (%s) is already in progress', $this->getEntityName()));
Copy link
Member Author

Choose a reason for hiding this comment

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

@colemanw You probably know the auto-refresh flag and existing callers better than me. Will emitting this kind of error cause random trouble?

IMHO, in a greenfield, this is good style. But it a brownfield, it might require sprinkling some try/catch expressions?

Copy link
Member

Choose a reason for hiding this comment

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

Well, the SearchKit UI calls this function every time you hit the "Save" button. It does so asynchronously so if this action fails it doesn't prevent the rest of the save from happening...
but I'd imagine if you repeatedly hit the "Save" button while rapidly making changes to the search, you'd manage to trigger the failure, which means your latest changes to the savedsearch wouldn't be reflected in the skentity table...
until the next time you hit the Save button or it gets refreshed via cron.
That's not a disastrous outcome by any means, but a nicer behavior would be like a tail-end debounce where e.g. if you click the button 5 times in quick succession, then refreshes 1-4 get cancelled and the 5th one gets to complete.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the SearchKit UI calls this function every time you hit the "Save" button. It does so asynchronously so if this action fails it doesn't prevent the rest of the save from happening...

OK, I tried it out. It's not bad. The "Save" button goes through several colored phases to indicate status:

  • Grey ("Save" -- available/inactive), then...
  • Orange ("Saving" -- primary save), then...
  • Green ("Saved" -- other stuff, unclear), then go back to "Grey"

If you exaggerate the refresh time (sleep(15)), the UI lingers during the green phase. The button remains blocked/non-responsive -- and it doesn't release until after the refresh finishes.

The green label is maybe slightly fictitious for this scenario, but not in any way that'll hurt a user, and the behavior seems good.

As far as the ScheduledJob goes, it sets a frequency of "Hourly" -- as long as the rebuild takes <1 hour, it should be fine. (And if it takes >1 hour, then that is cause for concern... seems entirely reasonable for the job-log to show a failure there...)

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks for testing that out @totten

}
$releaseLock = \CRM_Utils_AutoClean::with([$lock, 'release']);

// Go!
\CRM_Core_DAO::executeQuery("CREATE TABLE `$newTable` LIKE `$finalTable`");
\CRM_Core_DAO::executeQuery("INSERT INTO `$newTable` ($columns) $sql");
\CRM_Core_DAO::executeQuery(sprintf('RENAME TABLE `%s` TO `%s`, `%s` TO `%s`',
$finalTable, $junkTable,
$newTable, $finalTable
));
\CRM_Core_DAO::executeQuery(sprintf('DROP TABLE `%s`', $junkTable));

// All done
$result[] = [
'refresh_date' => \CRM_Core_DAO::singleValueQuery("SELECT NOW()"),
];
Expand Down
5 changes: 5 additions & 0 deletions ext/search_kit/Civi/Search/Exception/RefreshInProgressException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?php
namespace Civi\Search\Exception;

class RefreshInProgressException extends \CRM_Core_Exception {
}