Code review comment for lp:~edoardo-serra/storm/select-for-update-support

Revision history for this message
James Henstridge (jamesh) wrote :

This isn't a full review, but here are a few initial comments:

1. If "SELECT FOR UPDATE" is incompatible with "GROUP BY", perhaps it would make sense to check for that in Select's compile function and raise ExprError there rather than trying to protect all the ResultSet entry points that could cause this problem.

2. You've added a comment asking whether it makes sense to remove the "FOR UPDATE" clause for ResultSet.is_empty(). If is_empty() returns True, then no rows would be locked if "FOR UPDATE" was left in. If it returns False, the program is likely to request those rows if it actually wants them locked. So I don't think the comment is necessary.

3. This doesn't need to go in your branch, but I wonder if it would be worth storing the face that a row has been locked in its obj_info, so that Store.get(..., for_update=True) can avoid a query if the row has already been locked? This could be set in Store._load_object() and unset by Store.invalidate().

« Back to merge proposal