Re: Re: [COMMITTERS] pgsql: Introduce group locking to prevent parallel processes from deadl

On Tue, Feb 16, 2016 at 3:54 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> 1. It removes the groupLeader flag from PGPROC. You're right: we don't need it.
>
> It occurs to me that this claim is bogus:
>
>> We institute a further coding rule that a process cannot join or leave a lock
>> group while owning any PROCLOCK. Therefore, given a lock manager lock
>> sufficient to examine PROCLOCK *proclock, it also safe to examine
>> proclock->tag.myProc->lockGroupLeader (but not the other fields mentioned in
>> the previous paragraph).
>
> Yes, we can legislate that workers have to join before taking any lock,
> and if processes only leave the group at death then that end of it is not
> a problem either; but it is patently not the case that a process will hold
> no locks when it calls BecomeLockGroupLeader().
>
> We may be able to salvage this simplification anyway, but it will require
> a tighter specification of when it's safe to look at
> proclock->tag.myProc->lockGroupLeader.
>
> Another possibility is to forget about executor-time
> BecomeLockGroupLeader(), and just say that any process that isn't a worker
> always becomes its own group leader at startup. Then the above para is
> true as written. I don't know what downsides this might have; do your
> changes in the lock manager try to optimize the null-lockGroupLeader case?

Hmm, that's true. I don't think it actually matters all that much,
because proclock->tag.myProc->lockGroupLeader == NULL has pretty much
the same effect as if proclock->tag.myProc->lockGroupLeader ==
proclock->tag.myProc. But not completely. One problem is that we
don't currently assume that 8-byte writes are atomic, so somebody
might see the group leader field half-set, which would be bad.

But, yes, there are some optimizations for that case. For example, in
LockCheckConflicts:

/* If no group locking, it's definitely a conflict. */
if (leader == NULL)
{
PROCLOCK_PRINT("LockCheckConflicts: conflicting (simple)",
proclock);
return STATUS_FOUND;
}

ProcSleep also has a check of this sort. In theory those
optimizations are quite important, because they can avoid extra passes
over the lock queue which theoretically turn O(N) algorithms into
O(N^2) or O(N^2) into O(N^3) or whatever. But in practice I'm not
sure that the lock queues are ever long enough for that to become an
issue. And if they are, the fact that your system is lock-bound is
probably a much bigger problem that a few extra CPU cycles inside the
lock manager to figure that out. But I don't want to be too flip
about that analysis - there might be some scenario where the extra
cycles do hurt.

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