Re: ALTER TABLE lock strength reduction patch is unsafe

On Fri, Jun 24, 2011 at 10:08 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jun 24, 2011 at 4:27 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On Fri, Jun 24, 2011 at 9:00 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Fri, Jun 24, 2011 at 3:46 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>>> Test case please. I don't understand the problem you're describing.
>>>
>>> S1: select * from foo;
>>> S2: begin;
>>> S2: alter table foo alter column a set storage plain;
>>> S1: select * from foo;
>>> <blocks>
>>
>> Er,,.yes, that what locks do. Where is the bug?
>
> I didn't say it was a bug.  I said that the additional locking you
> added acted to remove the much of the benefit of reducing the lock
> strength in the first place.  If a brief exclusive lock (such as would
> be taken by ALTER TABLE SET STORAGE in 9.0 and prior release) wasn't
> acceptable, a brief exclusive lock on a different lock tag that blocks
> most of the same operations isn't going to be any better.  You might
> still see some improvement in the cases where some complicated
> operation like scanning or rewriting the table can be performed
> without holding the relation definition lock, and then only get the
> relation definition lock afterward.  But for something like the above
> example (or the ALTER TABLE SET (n_distinct) feature you mentioned in
> your previous email) the benefit is going to be darned close to zero.
>
>> This patch has locking, but its the most reduced form of locking that
>> is available for a non invasive patch for 9.1
>
> I don't like the extra lock manager traffic this adds to operations
> that aren't even performing DDL, I'm not convinced that it is safe,
> the additional locking negates a significant portion of the benefit of
> the original patch, and Tom's made a fairly convincing argument that
> even with this pg_dump will still become significantly less reliable
> than heretofore even if we did apply it.  In my view, what we need to
> do is revert to using AccessExclusiveLock until some of these other
> details are worked out.  I wasn't entirely convinced that that was
> necessary when Tom first posted this email, but having thought through
> the issues and looked over your patch, it's now my position that that
> is the best way forward.  The fact that your proposed patch appears
> not to be thinking very clearly about when locks need to be taken and
> released only adds to my feeling that we are in for a bad time if we
> go down this route.
>
> In short: -1 from me.

I've explained all of the above points to you already and you're wrong.

I don't think you understand this patch at all, nor even the original feature.

Locking the table is completely different from locking the definition
of a table.

Do you advocate that all ALTER TABLE operations use
AccessExclusiveLock, or just the operations I added? If you see
problems here, then you must also be willing to increase the lock
strength of things such as INHERITS, and back patch them to where the
same problems exist. You'll wriggle out of that, I'm sure. There are
regrettably, many bugs here and they can't be fixed in the simple
manner you propose.

It's not me you block Robert, I'm not actually a user and I will sleep
well whatever happens, happy that I tried to resolve this. Users watch
and remember.

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