https://bugs.winehq.org/show_bug.cgi?id=45023
Bug ID: 45023 Summary: TestBot Object Relational Mapper issues Product: Wine-Testbot Version: unspecified Hardware: x86 OS: Linux Status: NEW Severity: normal Priority: P2 Component: unknown Assignee: wine-bugs@winehq.org Reporter: fgouget@codeweavers.com Distribution: ---
The TestBot implements its own perl Object Relational Mapper (ORM) to access the database. It is implemented by the modules in lib/ObjectModel. The are two main concepts:
* Items An Item is an object representing a table row. The classes such as Job, Step and Task all inherit from Item.
* Collections A collection represents a table and is thus a collection of Items. To allow for fast access the items are stored in a perl hashtable indexed by their primary key. A Collection may not contain all of the table's rows. For instance a Step Collection only contains the Steps of the parent Job object used to create it. So $Job->Steps is a collection that contains only the $Job steps.
The items in a Collection can be further filtered out by specifying that one of their property must be in a set of values. For instance $Jobs->AddFilter("Status", ["queued", "running"]) will cause the $Jobs collection to only return the queued and running jobs.
Unfortunately this implementation has a number of limitations and design issues.
1. Care must be taken when creating multiple objects.
my $Job1 = $Jobs->Add(); ... Set mandatory fields ... my $Job2 = $Jobs->Add(); ... Set mandatory fields ... # Here $Job1 != $Job2 $Jobs->Save();
Only $Job2 got saved to the database! The reason is that a Job's primary key is an auto-increment field. So the Job Id is set when it is saved to the database. Thus when created in memory that field is unset and an empty string is used when adding it to the $Jobs items hashtable. Thus $Job2 overwrites $Jobs->{Items}->{""} and is the only one that gets saved.
Note that there is no such issue for Steps for instance because their key is not an auto-increment field and is set as soon as the object is created.
Fortunately the workaround is simple: systematically save an object before creating the next one but the difference in behavior between $Jobs->Add() and $Steps->Add() is error prone.
2. Auto-increment values on new Items are not ignored
If an auto-increment field is modified on an object, that value is used when saving the object to the database.
This means fixing the previous issue is not as simple as setting $Job->Id(--$Unique): handling of auto-increment fields also needs to be changed when saving new objects.
3. Items retrieved from Detailref collections are missing fields
'Detailref' properties are used to represent 'one to many' relationships. They are used for instance to access the Steps that belong to a Job:
my $Step = $Job->Steps->GetItem(1);
Here Steps is a collection that gets created based on the information of the job's DetailRef property.
However objects gotten in this way don't provide access to all of the corresponding database fields. In the case of Steps, although the Steps table has a JobId column, the $Step->JobId property does not exist. This presents difficulties in a number of cases such as when trying to figure out the name of the directory containing the Step's files since it is called "jobs/$JobId/$StepNo" (see Step::GetDir()).
It also means there is no $Step->Job property to get back to the parent Job object (this would also cause a perl reference loop which would be troublesome).
The reason for this issue is that when a Collection or Item has a parent object (called a 'master' object in the TestBot ORM) the parent's foreign key fields (Step.JobId here) are stored separately in a 'master cols' structure and are thus not accessible through the normal field methods.
4. "Child" objects can only be accessed from their parent
Any collection referenced through a Detailref property can only be accessed through the corresponding 'parent' object.
This is the case for the Steps collections. The only way to get a collection of steps is through $Job->Steps and this only returns the steps that belong to $Job. So it's not possible to iterate over all the rows in the Steps table.
There is a (somewhat incorrect) workaround for this for the Records table. This involves having the CreateRecords() method picking one of two PropertyDescriptors list depending on the presence of a parent object.
However the corresponding Record objects have an extra field compared to the ones obtained through $RecordGroup->Records: this time the foreign key identifying the parent object is accessible since it's not not tucked away in the 'master cols' structure. This is not an issue unless you start mixing both types of objects, for instance through scope inheritance.
5. The 'master cols' terminology is ambiguous.
It's never clear if a 'master cols' method treats $self as a child of its parent object, or as the parent to other child objects.
* $Step->GetMasterCols() treats $Step as a child object and returns the key columns of its $Job master object, that is (JobId).
* Similarly $Step->MasterKeyChanged() means the parent object's key changed, so here that the value of $JobId changed (which happens on new objects due to the auto-increment field btw).
* Despite the unchanged 'Master' moniker, $Step->GetMasterKey() considers $Step to be the master object of the Tasks it contains and thus includes the step's key in the returned columns. This means $Step->GetMasterKey() returns (JobId, StepNo).
* Despite not having the 'Master' moniker, $Step->GetFullKey() works the same as $Step->GetMasterKey() but returns the column values as a single string '$JobId#@#$StepNo'.
6. Filters on Detailref collections
It's possible to put a filter on a collection to only get a subset of the corresponding rows. For instance $Job->Steps->AddFilter('Status', ['queued']) ensures that one will only get the queued steps of $Job.
But Detailref collections are stored as a field of the Item they belong to. This means $Job->Steps always returns the same collection object. So once a piece of code has set a filter on it, to only return 'queued' steps for instance, all other parts of the code will only get the 'queued' steps, no matter what they need. Also there is no method for cloning a collection (so one cannot do $Job->Steps->Clone()->AddFilter()), or for removing a filter.
For instance: $Job->Steps->AddFilter('Status', ['queued']); ... # Here UpdateStatus() will only take into account the queued steps! $Job->UpdateStatus();
Note also that it's not entirely trivial to work around. It's tempting to do something like: $Job->Steps->AddFilter('Status', ['queued']); ... # Make sure the database contains an up-to-date view of all the Steps and # Tasks in $Job. $Job->Save(); # Then create a new job object from the database my $TmpJob = CreateJobs()->GetItem($Job->Id); # Now we can update the status without fearing bad filters. $TmpJob->UpdateStatus(); # But here $Job->Status could still be wrong so any code that still uses # $Job will be lead astray. So update $Job. $Job->Status($TmpJob->Status) # But that still leaves out every Step and Task object referenced by $Job # Also saving $Job will save $Job->Status, again. What could go wrong?
7. Filters vs. loading from the database
Once a collection has been loaded from the database, adding a filter to it has no effect. That's because filtering happens by tweaking the SQL query and changing the filter does not mark the collection for reloading.
Again this issue is made more severe because of the way Detailref collections are handled. If a piece of code has caused the $Job->Steps collection to be loaded, then adding a filter elsewhere will have no effect.
For instance: $Job->UpdateStatus(); # calls $Job->GetItems(), loading it from DB ... $Job->Steps->AddFilter('Status', ['queued']); foreach my $Step ($Job->Steps->GetItem()) # Here we will get all steps, not just the queued ones!
8. No "or" operator in filters
Originally the filters only allowed one to request that a field be in a set of admissible values. This was extended to allow requesting for a field to be less than, greater than or like some value.
However it is still not possible to use an "or" operator in filters. So for instance one cannot retrieve all Steps that completed recently or correspond to a WineTest run (WHERE Ended > 5 minutes ago OR User == 'batch').
9. No Itemref support for multiple key fields.
A Task has a VMName field which is a foreign key referencing a VM. Through the use of an Itemref property one can directly access the VM with $Task->VM. But we cannot do the same thing for steps.
The primary key of a step has two fields, (JobId, No). A step also has a PreviousNo field which identifies the step it depends on. So we would want to be able to access the previous step through $Step->Previous by using an Itemref property:
CreateItemrefPropertyDescriptor("Previous", "Previous step", !1, !1, &CreateSteps, ["JobId", "PreviousNo"]),
However this fails with a "Multiple key components not supported" error. Note that for this to work we would also need a way to tell the Itemref that the Step.PreviousNo field should be mapped to Step.No. But there is no way to do so.
10. Collection::GetItem() blindly adds the item to the collection
Collections have a GetItem() method that returns an item when given a string containing that object's key.
So using the Step example above, if we still have the right $Job->Steps collection accessible, we can easily get the Previous step through:
$Job->Steps->GetItem($Step->PreviousNo)
However this also adds $Step->PreviousNo to the $Job->Steps collection, regardless of what the filter on that collection is. So when the scheduler analyzes the queued and running steps to figure out whether the one they depend on, $Step->PreviousNo, has completed, it also unwittingly adds the previous step to the $Job->Step collection. So the next iteration on the collection may return completed steps despite expectations.
11. Save order issues
The TestBot ORM automatically takes care of saving parent objects before the child objects they contain. In practice, if you create a Job > Step > Task hierarchy you can count on the TestBot ORM to save the Job before saving the Steps that use the JobId as part of their key and so on.
But this breaks down for $Step->PreviousNo. There you have to make sure to save the steps in the right order otherwise you get an SQL error.
Similarly deletions must be performed in the right order. Fortunately this is mostly transparent since we only delete whole Jobs and Job::OnDelete() takes care of blanking the Step::PreviousNo fields before recursing.
12. No Order-by support
There are a many cases where we retrieve a number of rows from the database and then do a simple alphabetical or numerical sort on them. That's the kind of thing that the database would do much faster than Perl. This is mostly an issue for the activity and statistics web pages because of the number of RecordGroups they handle.
So it would be nice to be able to specify an Order-By SQL directive when retrieving the objects. However this would run into the same issues as the filters for Detailref collections with regards to already loaded collections, and GetItem() not knowing where to insert freshly loaded objects.
13. No support for joins
There are a few cases where doing a join could be useful.
For instance when reporting the activity what we really want is all the Records rows corresponding to a RecordGroup that is less than 12 hours old. For now we have to proceed indirectly: we query all the RecordGroup objects that are less than 12 hours old, take the lowest GroupId we find and then retrieve all the Records that have a Group Id greater than that.
A different type of join could also be useful in many other places: currently we first retrieve the jobs and then we do one more SQL request per job to retrieve its steps and then one per step to retrieve the tasks. A join could let us load it all in one request. Fortunately we don't have many jobs so unlike in the activity case this does not have a significant performance impact.
14. Performance
Most parts of the TestBot don't have much processing to do so that performance is not an issue (though there is not really any hard data).
Things are different for the activity and statistics page. The statistics page is the worst and generating it takes over a dozen seconds. That's annoying because of the load it puts on the TestBot web server. It's also really long for a mere couple dozen thousand Records. Not all of it comes from the ORM but over 50% of it does.
There are some relatively obvious paths for improvement. For instance accessing a field like $Job->Ended causes the ORM to loop over all of a job's property descriptors until it finds one called 'Ended', and then it returns the value from a hash table. It should be possible to make it so that most of the time the value is returned directly from the hash table. Detailref and Itemref properties have their own hash tables and so must be handled separately. But hat could likely also be changed.