Re: SKIP LOCKED DATA (work in progress)

On Tue, Jul 29, 2014 at 9:48 AM, Thomas Munro <munro(at)ip9(dot)org> wrote:

> On 27 July 2014 23:19, Thomas Munro <munro(at)ip9(dot)org> wrote:
> > 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.
>
> Now with extra isolation tests so that the three different code
> branches that can skip rows are covered. I temporarily added some
> logging lines to double check that the expected branches are reached
> by each permutation while developing the specs. They change the
> output and are not part of the patch -- attaching separately.
>

I've had a look over the isolation tests now and I can't see too much
wrong, just a couple of typos...

* skip-locked-2.spec

# s2 againt skips because it can't acquired a multi-xact lock

"againt" should be "again"

also mixed use of multixact and multi-xact, probably would be better to
stick to just 1.

Also this file would probably be slightly easier to read with a new line
after each "permutation" line.

* skip_locked-3.spec

# s3 skips to second record due to tuple lock held by s2

There's a missing "the" after "skips to"

Also, won't the lock be held by s1 not s2?

There's just a couple of other tiny things.

* Some whitespace issues shown by git diff --check

src/backend/parser/gram.y:9221: trailing whitespace.
+opt_nowait_or_skip:
src/backend/rewrite/rewriteHandler.c:65: trailing whitespace.
+
LockClauseStrength strength, LockWaitPolicy waitPolicy,

* analyze.c

The StaticAssertStmt's I think these should be removed. The only place
where this behaviour can be changed
is in lockwaitpolicy.h, I think it would be better to just strengthen the
comment on the enum's definition.

Perhaps something more along the lines of:

Policy for what to do when a row lock cannot be obtained immediately.

The enum values defined here have critical importance to how the parser
treats multiple FOR UPDATE/SHARE statements in different nested levels of
the query. Effectively if multiple locking clauses are defined in the query
then the one with the highest enum value takes precedence over the others.

Apart from this I can't see any other problems with the patch and I'd be
very inclined, once the above are fixed up to mark the patch ready for
commiter.

Good work

Regards

David Rowley