Plugin model - #1045
Conversation
noise64
left a comment
noise64
left a comment
There was a problem hiding this comment.
Added a few DB related comments and questions, let's disccuss them in more detail IRL
| component_id uuid REFERENCES components (component_id), | ||
| component_version bigint, | ||
|
|
||
| FOREIGN KEY (plugin_name, plugin_version) REFERENCES plugins (name, version) |
There was a problem hiding this comment.
Foreign keys are not enforced or enabled by default, it is required to be configured on each open (and missing to do so can create a db with integrity problems). I think we do not have such option in our sqlite config currenlty and it not set by default.
Also there are a few things that worth to be checked (see https://www.sqlite.org/foreignkeys.html), like the foreign key must reference a fields in a way that matches a primary or unique key in the referenced table. This seems to be the case here, just wanted to highlight that indexes are not auto created for such uses, unlike other DBs.
There was a problem hiding this comment.
As discussed, will be done in an followup PR
| transform_url text, | ||
| component_id uuid REFERENCES components (component_id), | ||
| component_version bigint, | ||
| deleted boolean NOT NULL DEFAULT FALSE, |
There was a problem hiding this comment.
it seems that we do need to use this in almost every query, so i would add a covering index for the primary key columns and this together
| let namespace: String = result.get("namespace"); | ||
| let name: String = result.get("name"); | ||
| if namespace != component.namespace || name != component.name { | ||
| transaction.rollback().await?; |
There was a problem hiding this comment.
is there a higher level transaction construct in sqlx? manually doing the rollbacks on errors seems fragile
There was a problem hiding this comment.
I would change this select to be part of the insert (or rather, an upsert, and check if there were any affected rows, otherwise i think there are some race conditions here.
There was a problem hiding this comment.
It should be rolled back on drop, my tests indicated that it is was not working properly, and adding an explicit rollback fixed the test. I don't like it either.
| ) -> Result<ComponentRecord, RepoError> { | ||
| let mut transaction = self.db_pool.begin().await?; | ||
|
|
||
| let result = sqlx::query("SELECT namespace FROM components WHERE component_id = $1") |
There was a problem hiding this comment.
i would probably make this part of the update through a join, and make them a predicate.
As for the "WITH prev selects", i think those are not reliable at all without golding explicit locks for the affected rows (or use a pattern where before any component version changes we do a single row lock on the components table)
* Initial plugin model and REST API * Plugins CLI first steps * More CLI work * CLI implementation and REST API fixes * Fixes and refactorings regarding component service's repo tests * Further test refactorings * test_default_plugin_repo * Tests and fixes for the plugin repos * gRPC API, changes * Plugin installation repo actions moved to component repo transactions WIP * Plugin installation repo actions moved to component repo transactions * Rewired plugin service * Intermediate state of redesignin the component repo * Switched to ComponentOwner from Namespace in component-service * Introduced component version available flag * Moved namespace filtering to SQL * Decoupled object store entity from component version, WIP * Decoupled object store entity from component version * gRPC implementation * Plugin scope request context * Separate plugin owner vs component owner abstraction * Changed accessible scope interface * Extra error case * Moved UpdatePayload to base * pub * Fixes * Refactoring to make some plugin types more reusable * Further refactoring to share plugin installation queries * PluginOwner moved to common * Connection between component owner and plugin owner * Fix * Fix * Update serde_json_path * Made some gRPC helper functions public * Namespace to the worker request executor * Removed unused auth context parameters * Removed auth context completely from the worker service trait * Exposed function in cli * Exposed some more functions in cli * Reusing InitialComponentFile from model in golem-client * Fix * Fix
2 participants
Resolves #1034