On Mon, Jan 20, 2014 at 11:23 PM, KaiGai Kohei <kai...@ak.jp.nec.com> wrote: > I briefly checked the patch. Most of lines are mechanical replacement > from LWLockId to LWLock *, and compiler didn't claim anything with > -Wall -Werror option. > > My concern is around LWLockTranche mechanism. Isn't it too complicated > towards the purpose? > For example, it may make sense to add "char lockname[NAMEDATALEN];" at > the tail of LWLock structure if LOCK_DEBUG is enabled. Probably, it > also adds an argument of LWLockAssign() to gives the human readable > name. Is the additional 64bytes (= NAMEDATALEN) per lock too large > for recent hardware?
Well, we'd need it when either LOCK_DEBUG was defined or when LWLOCK_STATS was defined or when --enable-dtrace was used, and while the first two are probably rarely enough used that that would be OK, I think the third case is probably fairly common, and I don't think we want to have such a potentially performance-critical difference between builds with and without dtrace. Also... yeah, it's a lot of memory. If we add an additional 64 bytes to the structure, then we're looking at 96 bytes per lwlock instead of 32, after padding out to a 32-byte boundary to avoid crossing cache lines. We need 2 lwlocks per buffer, so that's an additional 128 bytes per 8kB buffer. For shared_buffers = 8GB, that's an extra 128MB for storing lwlocks. I'm not willing to assume nobody cares about that. And while I agree that this is a bit complex, I don't think it's really as bad as all that. We've gotten by for a long time without people being able to put lwlocks in other parts of memory, and most extension authors have gotten by with LWLockAssign() just fine and can continue doing things that way; only users with particularly sophisticated needs should bother to worry about the tranche stuff. One idea I just had is to improve the dsm_toc module so that it can optionally set up a tranche of lwlocks for you, and provide some analogues of RequestAddinLWLocks and LWLockAssign for that case. That would probably make this quite a bit simpler to use, at least for people using it with dynamic shared memory. But I think that's a separate patch. > Below is minor comments. > > It seems to me this newer form increased direct reference to > the MainLWLockArray. Is it really graceful? > My recommendation is to define a macro that wraps the reference to > MainLWLockArray. > > For example: > #define PartitionLock(i) \ > (&MainLWLockArray[LOCK_MANAGER_LWLOCK_OFFSET + (i)].lock) Yeah, that's probably a good idea. > This chunk is enclosed by #ifdef EXEC_BACKEND, the legacy LWLockArray > was not removed. Oops, will fix. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
