Re: SKIP LOCKED DATA (work in progress)

On 27 July 2014 14:31, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> 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().

Here's a new version with explicit numerical values and a comment in
lockwaitpolicy.h to explain that the order is important and point to
the relevant code. The assertions are about the relationship between
constant values known at compile time, so why would we want a runtime
assertion? I have changed it from StaticAssertExpr to
StaticAssertStmt though.

> 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?

Since I was meddling with code that controls both NOWAIT and SKIP
LOCKED, I wanted to convince myself that I had not broken NOWAIT using
at least a basic smoke test . I suppose by the same logic I should
also wite isolation tests for default blocking FOR UPDATE... Ok, I've
taken nowait.spec out for now.

On the subject of isolation tests, I think skip-locked.spec is only
producing schedules that reach third of the three 'return
HeapTupleWouldBlock' statements in heap_lock_tuple. I will follow up
with some more thorough isolation tests in the next week or so to
cover the other two, and some other scenarios and interactions with
other feature.

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

Fixed.

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

Right, I have removed the redundant conditionals.

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

Fixed.

Best regards,
Thomas Munro