Issue #752: Migrate all the location status to bits

[petersistrom]

Resolves #752

Schema: boolean columns instead of a bitmask column

Moodle has no cross-database support for bitwise indexes, so a single combined location integer column cannot be indexed efficiently. Replacing it with three boolean columns (in_filedir, in_mdl_files, in_remote) allows a standard composite index to work on all supported databases.

PHP/DB boundary

PHP code keeps the existing bitmask constants. location_helper translates at the boundary: splitting a bitmask into the three boolean values on write and reconstructing it from a DB row on read.

Candidate classes

Candidate queries previously filtered on exact location equality, which breaks when new bit flags are added. They now express two masks - bits an object must have, and bits it must not have. So they survive the addition of new flags without modification.

[alexdamsted]

Is the condition this comment asserts actually enforced?

E.g., If the function were ever called for a contenthash not present in mdl_files it would return LOCAL (3) instead of ORPHANED (1) right?

[petersistrom]

Yes correct, if it were called for a file not in mdl_files it would return LOCAL. The precondition isn't enforced, it's just a calling convention. However, orphan detection is handled via scheduled tasks.

[brendanheywood]

The recoverer task looks for objects which are in the objectfs table but not in mdl_files and then tries to recover them. I'd prefer it if this had an param which if set did proper checks to see if its in the mdl_files as well.

Or perhaps better, it would pass in a bit map of the places we know it is present and then we don't have to re-check those places. So most places start with knowing mdl_files is present, the recoverer on the other hand only knows that it is present in objectfs tables.

get_object_location_from_hash($contenthash, $knownlocations = OBJECT_LOCATION_IN_MDL_FILES) {

recoverer:
get_object_location_from_hash($contenthash, 0) {

[petersistrom]

Yes, I agree passing in a bit map of known locations is a better approach. Happy to implement that.

[petersistrom]

Updated to pass in known locations

[alexdamsted]

The default for $knownlocations should be 0 and callers of this method should pass OBJECT_LOCATION_IN_MDL_FILES (add this to the doc block of this method to make it known to callers).

We want to do this because if a caller calls this on a hash in the db table objectfs_objects but the associated file reference of the hash no longer exists in mdl_files then the bitmask returned will incorrectly include IN_MDL_FILES which causes the checker to class the object as EXTERNAL or DUPLICATED instead of MISSING or the like.

[alexdamsted]

@petersistrom Can the deleter, puller, pusher, and recoverer candidates classes now just be deleted? They aren't reachable via the factory anymore from your changes.

@brendanheywood mentioned about the need of candidates now after your changes. To me looking at the code raw it looks like a layer of abstraction that just complicates things more. You could move the sql from these into a single candidates static class to simplify the abstraction perhaps.

What problem was the candidates abstraction solving before your changes? To me the candidates had different sql logic and has some useful unique methods, e.g., relating to logging, but now with your changes its not as much the case and the unique methods aren't a lot of code.

[petersistrom]

@alexdamsted Yes they can be deleted now, they were providing different SQL queries for each task but that has now collapsed into one query with the bitmask changes. And I think Brendan's suggestion now makes sense to nuke the concept of candidate classes completely.