On Sun, Jul 27, 2014 at 4:49 AM, Thomas Munro <munro@ip9.org> wrote:
Here is a new version of the patch with a single enum LockWaitPolicy
defined in utils/lockwaitpolicy.h.
That seems much cleaner
A few more comments:
You seem to have lost the comment which indicates that the values of the enum are important due to the code in applyLockingClause(), but I see now instead that you've added some assert checks in applyLockingClause(), likely these should use Assert() rather than StaticAssertExpr().
I've also been looking at the isolation tests and I see that you've added a series of tests for NOWAIT. I was wondering why you did that as that's really existing code, probably if you thought the tests were a bit thin around NOWAIT then maybe that should be a separate patch?
In ExecLockRows(), is there a need to define the wait_policy variable now? It's just used once so you could probably just pass erm->waitPolicy directly to heap_lock_tuple().
I'm a bit confused at some of the code in heap_lock_tuple(). If I'm not wrong then after the line that does have_tuple_lock = true; it's never possible for have_tuple_lock to be false, but I see you've added checks to ensure we only unlock if have_tuple_lock is true. I'm thinking you probably did this because in the goto failed situation the check is done, but I was thinking that was perhaps put there in case a goto jump was added before have_tuple_lock is set to true. I'm wondering if it would be ok just to replace the test with an Assert() instead, or maybe just no check.
