Merge lp://staging/~edoardo-serra/storm/select-for-update-support into lp://staging/storm

Edoardo, thanks for this branch! Before we spend time reviewing the
changes, have you signed the Canonical copyright assignment forms?
This process will need to be completed before we can accept changes.

Hi Jamu, sorry I didn't find any mention of this on the Wiki.

I just accepdet the form via email.

Regards

On Sep 10, 2010, at 4:52 PM, Jamu Kakar wrote:

> Edoardo, thanks for this branch! Before we spend time reviewing the
> changes, have you signed the Canonical copyright assignment forms?
> This process will need to be completed before we can accept changes.
>
> --
> https://code.launchpad.net/~edoardo-serra/storm/select-for-update-support/+merge/35112
> You are the owner of lp:~edoardo-serra/storm/select-for-update-support.

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().

One other point: it is probably worth using an API that can support "FOR SHARE" read locks in addition to "FOR UPDATE" write locks.