Обсуждение: tablecmds.c and lock hierarchy

Hi all,

As mentioned in the thread related to lowering locks of autovacuum
reloptions in ALTER TABLE SET
(http://www.postgresql.org/message-id/CAFcNs+oX7jVENC_3i54fDQ3ibmOGmknc2tMevdSmvojbSXGbGg@mail.gmail.com),
I have noticed the following code in
AlterTableGetLockLevel@tablecmds.c:
                /*
                 * Take the greatest lockmode from any subcommand
                 */
                if (cmd_lockmode > lockmode)
                        lockmode = cmd_lockmode;

The thing is that, as mentioned by Alvaro and Andres on this thread,
we have no guarantee that the different relation locks compared have a
monotone hierarchy and we may finish by taking a lock that does not
behave as you would like to. We are now lucky enough that ALTER TABLE
only uses ShareUpdateExclusiveLock, ShareRowExclusiveLock and
AccessExclusiveLock that actually have a hierarchy so this is not a
problem yet.
However it may become a problem if we add in the future more lock
modes and that are used by ALTER TABLE.

One idea mentioned in the ALTER TABLE SET thread was to enforce the
lock to AccessExclusiveLock should two locks of any subcommands
differ. Another idea was to select all the locks of the subcommands
found to be present, and open the different relations with all of
them. Tracking all those locks is going to require some heavy
rewriting, like for example the use of a LOCKMASK to track the locks
that need to be taken, and more extended routines for relation_open.
Other possibilities may be to add a comment on top of
AlterTableGetLockLevel so as we do not use a lock in ALTER TABLE
commands that may result in a choice conflict with the other ones, to
simply do nothing because committers are very careful people usually,
or to track all the locks taken in AlterTableGetLockLevel and then run
DoLockModesConflict and ERROR if there are incompatible locks.

I am attaching a patch that implements the first approach mentioned to
give a short idea of how things could be improved.
Thoughts?
--
Michael

Michael Paquier wrote:

> As mentioned in the thread related to lowering locks of autovacuum
> reloptions in ALTER TABLE SET
> (http://www.postgresql.org/message-id/CAFcNs+oX7jVENC_3i54fDQ3ibmOGmknc2tMevdSmvojbSXGbGg@mail.gmail.com),
> I have noticed the following code in
> AlterTableGetLockLevel@tablecmds.c:
>                 /*
>                  * Take the greatest lockmode from any subcommand
>                  */
>                 if (cmd_lockmode > lockmode)
>                         lockmode = cmd_lockmode;
> 
> The thing is that, as mentioned by Alvaro and Andres on this thread,
> we have no guarantee that the different relation locks compared have a
> monotone hierarchy and we may finish by taking a lock that does not
> behave as you would like to.

Maybe the solution to this is to add the concept of "addition" of two
lock modes, where the result is another lock mode that conflicts with
any lock that would conflict with either of the two operand lock modes.
For instance, if you add a lock mode to itself, you get the same lock
mode; if you "add" anything to AccessExclusive, you get AccessExclusive;
if you "add" anything to AccessShare, you end up with that other lock
mode.  The most interesting case in our current lock table is if you
"add" ShareUpdateExclusive to Share, where you end up with
ShareRowExclusive.  In essence, holding the result of that operation is
the same as holding both locks, which AFAICS is the behavior we want.

This can be implemented with a simple table and solves the problem for
both ALTER TABLE SET and this existing usage you cite and is not as
invasive as your first proposed solution.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Aug 4, 2015 at 2:43 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>
>> As mentioned in the thread related to lowering locks of autovacuum
>> reloptions in ALTER TABLE SET
>> (http://www.postgresql.org/message-id/CAFcNs+oX7jVENC_3i54fDQ3ibmOGmknc2tMevdSmvojbSXGbGg@mail.gmail.com),
>> I have noticed the following code in
>> AlterTableGetLockLevel@tablecmds.c:
>>                 /*
>>                  * Take the greatest lockmode from any subcommand
>>                  */
>>                 if (cmd_lockmode > lockmode)
>>                         lockmode = cmd_lockmode;
>>
>> The thing is that, as mentioned by Alvaro and Andres on this thread,
>> we have no guarantee that the different relation locks compared have a
>> monotone hierarchy and we may finish by taking a lock that does not
>> behave as you would like to.
>
> Maybe the solution to this is to add the concept of "addition" of two
> lock modes, where the result is another lock mode that conflicts with
> any lock that would conflict with either of the two operand lock modes.
> For instance, if you add a lock mode to itself, you get the same lock
> mode; if you "add" anything to AccessExclusive, you get AccessExclusive;
> if you "add" anything to AccessShare, you end up with that other lock
> mode.  The most interesting case in our current lock table is if you
> "add" ShareUpdateExclusive to Share, where you end up with
> ShareRowExclusive.  In essence, holding the result of that operation is
> the same as holding both locks, which AFAICS is the behavior we want.

That's commutative, as this is basically looking at the conflict table
to get the union of the bits to indicate what are all the locks
conflicting with lock A and lock B, and then we select the lock on the
table that includes the whole union, with a minimum number of them.

Now, let's take for example this case with locks A, B, C, D:
- Lock A conflicts with ACD
- B with BCD
- C with itself
- D with itself
What would you choose as a result of add(C,D)? A or B? Or the super
lock conflicting with all of them?
-- 
Michael

On Tue, Aug 4, 2015 at 3:35 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> Please provide the link to the discussion of this. I don't see a problem
> here right now that can't be solved by saying

Thread:
http://www.postgresql.org/message-id/CAFcNs+oX7jVENC_3i54fDQ3ibmOGmknc2tMevdSmvojbSXGbGg@mail.gmail.com

Particularly those messages:
http://www.postgresql.org/message-id/20150731022857.GC11473@alap3.anarazel.de
http://www.postgresql.org/message-id/20150731200012.GC2441@postgresql.org
http://www.postgresql.org/message-id/CAB7nPqSK-hSZG7T1tAJ_=HEYsi6P1ejgX2x5LW3LYXJ7=9cOiQ@mail.gmail.com

> Assert(locklevel==ShareUpdateExclusiveLock ||
> locklevel>ShareRowExclusiveLock);

Yep, true as things stand now. But this would get broken if we add a
new lock level between ShareRowExclusiveLock and AccessExclusiveLock
that does not respect the current monotone hierarchy between those.
-- 
Michael

Michael Paquier wrote:
> On Tue, Aug 4, 2015 at 2:43 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

> > Maybe the solution to this is to add the concept of "addition" of two
> > lock modes, where the result is another lock mode that conflicts with
> > any lock that would conflict with either of the two operand lock modes.

> That's commutative, as this is basically looking at the conflict table
> to get the union of the bits to indicate what are all the locks
> conflicting with lock A and lock B, and then we select the lock on the
> table that includes the whole union, with a minimum number of them.

Yes.

> Now, let's take for example this case with locks A, B, C, D:
> - Lock A conflicts with ACD
> - B with BCD
> - C with itself
> - D with itself
> What would you choose as a result of add(C,D)? A or B? Or the super
> lock conflicting with all of them?

This appears to me an hypothetical case that I don't think occurs in our
conflicts table, so I wouldn't worry about it.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Aug 4, 2015 at 2:41 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Yep, true as things stand now. But this would get broken if we add a
> new lock level between ShareRowExclusiveLock and AccessExclusiveLock
> that does not respect the current monotone hierarchy between those.

But we're probably not going to do that, so it doesn't matter; and if
we do do it, we can worry about it then.  I don't think this is worth
getting concerned about now.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Aug 5, 2015 at 2:23 AM, Alvaro Herrera wrote:
> Michael Paquier wrote:
>> On Tue, Aug 4, 2015 at 2:43 PM, Alvaro Herrera wrote:
>> Now, let's take for example this case with locks A, B, C, D:
>> - Lock A conflicts with ACD
>> - B with BCD
>> - C with itself
>> - D with itself
>> What would you choose as a result of add(C,D)? A or B? Or the super
>> lock conflicting with all of them?

Actually the answer is the sum of all the potential candidates. This
converges to AccessExclusiveLock.

> This appears to me an hypothetical case that I don't think occurs in our
> conflicts table, so I wouldn't worry about it.

No it doesn't. I am using a huge hypothetical "if" here :)
-- 
Michael

On Wed, Aug 5, 2015 at 3:05 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Aug 4, 2015 at 2:41 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Yep, true as things stand now. But this would get broken if we add a
>> new lock level between ShareRowExclusiveLock and AccessExclusiveLock
>> that does not respect the current monotone hierarchy between those.
>
> But we're probably not going to do that, so it doesn't matter; and if
> we do do it, we can worry about it then.  I don't think this is worth
> getting concerned about now.

OK. Then let me suggest the attached Assert safeguard then. It ensures
that all the locks used follow a monotony hierarchy per definition of
what is on the conflict table. That looks like a cheap insurance...

In any case, this means as well that we should move on with the
current logic presented by Fabrizio on the other thread.
--
Michael

On Tue, Aug 04, 2015 at 07:35:43AM +0100, Simon Riggs wrote:
> On 4 August 2015 at 05:56, Michael Paquier <michael.paquier@gmail.com> wrote:
> > The thing is that, as mentioned by Alvaro and Andres on this thread,
> > we have no guarantee that the different relation locks compared have a
> > monotone hierarchy and we may finish by taking a lock that does not
> > behave as you would like to. We are now lucky enough that ALTER TABLE
> > only uses ShareUpdateExclusiveLock, ShareRowExclusiveLock and
> > AccessExclusiveLock that actually have a hierarchy so this is not a
> > problem yet.
> > However it may become a problem if we add in the future more lock
> > modes and that are used by ALTER TABLE.
> >
> 
> Please provide the link to the discussion of this. I don't see a problem
> here right now that can't be solved by saying
> 
> Assert(locklevel==ShareUpdateExclusiveLock ||
> locklevel>ShareRowExclusiveLock);

Agreed; that addresses the foreseeable future of this threat.

On Wed, Aug 5, 2015 at 11:47 AM, Noah Misch <noah@leadboat.com> wrote:
> On Tue, Aug 04, 2015 at 07:35:43AM +0100, Simon Riggs wrote:
>> On 4 August 2015 at 05:56, Michael Paquier <michael.paquier@gmail.com> wrote:
>> > The thing is that, as mentioned by Alvaro and Andres on this thread,
>> > we have no guarantee that the different relation locks compared have a
>> > monotone hierarchy and we may finish by taking a lock that does not
>> > behave as you would like to. We are now lucky enough that ALTER TABLE
>> > only uses ShareUpdateExclusiveLock, ShareRowExclusiveLock and
>> > AccessExclusiveLock that actually have a hierarchy so this is not a
>> > problem yet.
>> > However it may become a problem if we add in the future more lock
>> > modes and that are used by ALTER TABLE.
>> >
>>
>> Please provide the link to the discussion of this. I don't see a problem
>> here right now that can't be solved by saying
>>
>> Assert(locklevel==ShareUpdateExclusiveLock ||
>> locklevel>ShareRowExclusiveLock);
>
> Agreed; that addresses the foreseeable future of this threat.

Some sub-commands are using ShareRowExclusiveLock... So this one is better :)
Assert(locklevel==ShareUpdateExclusiveLock || locklevel >=
ShareRowExclusiveLock);
Or we simply list all the locks allowed individually... But that's a
minor point.
-- 
Michael