Re: SKIP LOCKED DATA (work in progress)

On 1 August 2014 10:37, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> * skip-locked-2.spec
>
> # s2 againt skips because it can't acquired a multi-xact lock
>
> "againt" should be "again"

Fixed.

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

Changed to multixact as seen in other places.

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

Done.

> * skip_locked-3.spec
>
> # s3 skips to second record due to tuple lock held by s2
>
> There's a missing "the" after "skips to"

Fixed.

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

s1 holds the row lock, and s2 holds the tuple lock (because it is at
the head of the queue waiting for the row lock). s3 skips because it
couldn't acquire the tuple lock held by s2. The other two specs are
about not being able to acquire a row lock and only need two sessions.
This one tests the code path when there is already a queue forming and
you can't even get the tuple lock, which requires an extra session. I
have updated the comment to make that clearer.

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

Fixed.

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

Removed.

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

Added something along those lines.

> 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

Thanks for all the guidance, I appreciate it! My review karma account
is now well overdrawn.

Best regards,
Thomas Munro