On Mon, Jan 6, 2014 at 3:32 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Robert Haas <robertmh...@gmail.com> writes: >> On Mon, Jan 6, 2014 at 2:48 PM, Tom Lane <t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
