Re: SKIP LOCKED DATA (work in progress) - Mailing list pgsql-hackers

<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jul 29, 2014 at 9:48 AM, Thomas Munro <span
dir="ltr"><<ahref="mailto:munro@ip9.org" target="_blank">munro@ip9.org</a>></span> wrote:<br /><blockquote
class="gmail_quote"style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div
class="">On27 July 2014 23:19, Thomas Munro <<a href="mailto:munro@ip9.org">munro@ip9.org</a>> wrote:<br /> >
Onthe subject of isolation tests, I think skip-locked.spec is only<br /> > producing schedules that reach third of
thethree 'return<br /> > HeapTupleWouldBlock' statements in heap_lock_tuple.  I will follow up<br /> > with some
morethorough isolation tests in the next week or so to<br /> > cover the other two, and some other scenarios and
interactionswith<br /> > other feature.<br /><br /></div>Now with extra isolation tests so that the three different
code<br/> branches that can skip rows are covered.  I temporarily added some<br /> logging lines to double check that
theexpected branches are reached<br /> by each permutation while developing the specs.  They change the<br /> output
andare not part of the patch -- attaching separately.<br /></blockquote></div><br /></div><div class="gmail_extra">I've
hada look over the isolation tests now and I can't see too much wrong, just a couple of typos... </div><div
class="gmail_extra"><br/></div><div class="gmail_extra"><div class="gmail_extra">* skip-locked-2.spec</div><div
class="gmail_extra"><br/></div><div class="gmail_extra"># s2 againt skips because it can't acquired a multi-xact
lock</div><divclass="gmail_extra"><br /></div><div class="gmail_extra"> "againt" should be "again"</div><div
class="gmail_extra"><br/></div><div class="gmail_extra">also mixed use of multixact and multi-xact, probably would be
betterto stick to just 1.</div><div class="gmail_extra"><br /></div><div class="gmail_extra">Also this file would
probablybe slightly easier to read with a new line after each "permutation" line.</div><div class="gmail_extra"><br
/></div><divclass="gmail_extra">* skip_locked-3.spec</div><div class="gmail_extra"><br /></div><div
class="gmail_extra">#s3 skips to second record due to tuple lock held by s2</div><div class="gmail_extra"><br
/></div><divclass="gmail_extra">There's a missing "the" after "skips to"</div><div class="gmail_extra"><br /></div><div
class="gmail_extra">Also,won't the lock be held by s1 not s2?</div><div class="gmail_extra"><br /></div><div
class="gmail_extra">There'sjust a couple of other tiny things.</div><div class="gmail_extra"><br /></div><div
class="gmail_extra"><divclass="gmail_extra">* Some whitespace issues shown by git diff --check</div><div
class="gmail_extra"><br/></div><div class="gmail_extra">src/backend/parser/gram.y:9221: trailing whitespace.</div><div
class="gmail_extra">+opt_nowait_or_skip:</div><divclass="gmail_extra">src/backend/rewrite/rewriteHandler.c:65: trailing
whitespace.</div><divclass="gmail_extra">+                                                              
LockClauseStrengthstrength, LockWaitPolicy waitPolicy,</div><div class="gmail_extra"><br /></div><div
class="gmail_extra">*analyze.c</div><div class="gmail_extra"><br /></div><div class="gmail_extra">The
StaticAssertStmt'sI think these should be removed. The only place where this behaviour can be changed</div><div
class="gmail_extra">isin lockwaitpolicy.h, I think it would be better to just strengthen the comment on the enum's
definition.</div><divclass="gmail_extra"><br /></div><div class="gmail_extra">Perhaps something more along the lines
of:</div><divclass="gmail_extra"><br /></div><div class="gmail_extra">Policy for what to do when a row lock cannot be
obtainedimmediately.</div><div class="gmail_extra"><br /></div><div class="gmail_extra">The enum values defined here
havecritical importance to how the parser</div><div class="gmail_extra">treats multiple FOR UPDATE/SHARE statements in
differentnested levels of</div><div class="gmail_extra">the query. Effectively if multiple locking clauses are defined
inthe query</div><div class="gmail_extra"> then the one with the highest enum value takes precedence over the
others.</div><divclass="gmail_extra"><br /></div><div class="gmail_extra"><br /></div><div class="gmail_extra">Apart
fromthis I can't see any other problems with the patch and I'd be very inclined, once the above are fixed up to mark
thepatch ready for commiter.</div><div class="gmail_extra"><br /></div><div class="gmail_extra">Good work</div><div
class="gmail_extra"><br/></div><div class="gmail_extra">Regards</div><div class="gmail_extra"><br /></div><div
class="gmail_extra">DavidRowley </div></div></div></div>