Re: SKIP LOCKED DATA (work in progress)

On Sun, Jul 27, 2014 at 4:49 AM, Thomas Munro <munro(at)ip9(dot)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.

Also, I'm just looking at some of the changes that you've done to function
signatures... I see quite a few of them are now beyond 80 chars wide (see
http://www.postgresql.org/docs/devel/static/source-format.html).

Regards

David Rowley