Re: ALTER TABLE lock strength reduction patch is unsafe

On 1 March 2014 21:25, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> wrote:
> On 03/01/2014 12:06 PM, Simon Riggs wrote:
>> On 27 February 2014 08:48, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> On 26 February 2014 15:25, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>>> On 2014-02-26 15:15:00 +0000, Simon Riggs wrote:
>>>>> On 26 February 2014 13:38, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2014-02-26 07:32:45 +0000, Simon Riggs wrote:
>>>>>>>> * This definitely should include isolationtester tests actually
>>>>>>>> performing concurrent ALTER TABLEs. All that's currently there is
>>>>>>>> tests that the locklevel isn't too high, but not that it actually works.
>>>>>>> There is no concurrent behaviour here, hence no code that would be
>>>>>>> exercised by concurrent tests.
>>>>>> Huh? There's most definitely new concurrent behaviour. Previously no
>>>>>> other backends could have a relation open (and locked) while it got
>>>>>> altered (which then sends out relcache invalidations). That's something
>>>>>> that should be tested.
>>>>> It has been. High volume concurrent testing has been performed, per
>>>>> Tom's original discussion upthread, but that's not part of the test
>>>>> suite.
>>>> Yea, that's not what I am looking for.
>>>>
>>>>> For other tests I have no guide as to how to write a set of automated
>>>>> regression tests. Anything could cause a failure, so I'd need to write
>>>>> an infinite set of tests to prove there is no bug *somewhere*. How
>>>>> many tests are required? 0, 1, 3, 30?
>>>> I think some isolationtester tests for the most important changes in
>>>> lock levels are appropriate. Say, create a PRIMARY KEY, DROP INHERIT,
>>>> ... while a query is in progress in a nother session.
>>> OK, I'll work on some tests.
>>>
>>> v18 attached, with v19 coming soon
>> v19 complete apart from requested comment additions
>
> I've started to look at this patch and re-read the thread. The first
> thing I noticed is what seems like an automated replace error. The docs
> say "This form requires only an SHARE x EXCLUSIVE lock." where the "an"
> was not changed to "a".
>
> Attached is a patch-on-patch to fix this. A more complete review will
> come later.

v20 includes slightly re-ordered checks in GetLockLevel, plus more
detailed comments on each group of subcommands.

Also corrects grammar as noted by Vik.

Plus adds an example of usage to the docs.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services