Re: dynamic shared memory and locks - Mailing list pgsql-hackers

On Mon, Jan 6, 2014 at 3:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Jan 6, 2014 at 2:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> -1 for the any_spinlock_held business (useless overhead IMO, as it doesn't
>>> have anything whatsoever to do with enforcing the actual coding rule).
>
>> Hmm.  I thought that was a pretty well-aimed bullet myself; why do you
>> think that it isn't?  I don't particularly mind ripping it out, but it
>> seemed like a good automated test to me.
>
> The coding rule is "only short straight-line code while holding a
> spinlock".  That could be violated in any number of nasty ways without
> trying to take another spinlock.  And since the whole point of the rule is
> that spinlock-holding code segments should be quick, adding more overhead
> to them doesn't seem very nice, even if it is only done in Assert builds.
>
> I agree it'd be nicer if we had some better way than mere manual
> inspection to enforce proper use of spinlocks; but this change doesn't
> seem to me to move the ball downfield by any meaningful distance.

Well, my thought was mainly that, while it may be a bad idea to take
another spinlock while holding a spinlock under any circumstances,
somebody might do it and it might appear to work just fine.  The most
likely sequences seems to me to be something like SpinLockAcquire(...)
followed by LWLockConditionalAcquire(), thinking that things are OK
because the lock acquisition is conditional - but in fact the
conditional acquire still takes the spinlock unconditionally.  Even
so, without --disable-spinlocks, this will probably work just fine.
Most likely there won't even be a performance problem, because a
lwlock has to be *very* hot for the spinlock acquisitions to become a
problem.  But with --disable-spinlocks, it will unpredictably
self-deadlock.  And since few developers are likely to test with
--disable-spinlocks, the problem most likely won't be noticed.  When
someone does then try to use --disable-spinlocks to port to a new
problem and starts hitting the deadlocks, they may not know enough to
attribute them to the real cause.  All in all it doesn't seem like
unduly expensive insurance, but I can remove it if the consensus is
against it.

>>> And I'd suggest defining NUM_SPINLOCK_SEMAPHORES in pg_config_manual.h,
>>> and maybe dropping SpinlockSemas() altogether in favor of just referencing
>>> the constant.  Otherwise this seems reasonable.
>
>> As far as pg_config_manual.h is concerned, is this the sort of thing
>> you have in mind?
>
>> #ifndef HAVE_SPINLOCKS
>> #define NUM_SPINLOCK_SEMAPHORES 1024
>> #endif
>
> I think we can just define it unconditionally, don't you?  It shouldn't
> get referenced in HAVE_SPINLOCK builds.  Or is the point that you want
> to enforce that?

I don't think it really matters much; seems like a style question to
me.  That's why I asked.

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