On Thu, Feb 25, 2016 at 10:31 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: >> There's no requirement that every session have every tranche >> registered. I think we should consider displaying "extension" for any >> tranche that's not built-in, or at least for tranches that are not >> registered (rather than "unknown wait event"). > > I think it is better to display as an "extension" for unregistered tranches, > but do you see any case where we will have wait event information > for any unregistered tranche?
Sure. If backend A has the tranche registered - because it loaded some .so which registered it in PG_init() - and is waiting on the lock, and backend B does not have the tranche registered - because it didn't do that - and backend B selects from pg_stat_activity, then you'll see a wait in an unregistered tranche. >> The changes to LockBufferForCleanup() don't look right to me. Waiting >> for a buffer pin might be a reasonable thing to define as a wait >> event, but it shouldn't reported as if we were waiting on the LWLock >> itself. > > makes sense, how about having a new wait class, something like > WAIT_BUFFER and then have wait event type as Buffer and > wait event as BufferPin. At this moment, I think there will be > only one event in this class, but it seems to me waiting on buffer > has merit to be considered as a separate class. I would just go with BufferPin/BufferPin for now. I can't think what else I'd want to group with BufferPins in the same class. >> What happens if an error is thrown while we're in a wait? > > For LWLocks, only FATAL error is possible which will anyway lead > to initialization of all backend states. For lock.c, if an error is > thrown, then state is reset in Catch block. In > LockBufferForCleanup(), after we set the wait event and before we > reset it, there is only a chance of FATAL error, if any system call > fails. We do have one error in enable_timeouts which is called from > ResolveRecoveryConflictWithBufferPin(), but that doesn't seem to > be possible. Now one question to answer is that what if tomorrow > some one adds new error after we set the wait state, so may be > it is better to clear wait event in AbortTransaction()? Yeah, I think so. Probably everywhere that we do LWLockReleaseAll() we should also clear the wait event. >> Does this patch hurt performance? > > This patch adds additional code in the path where we are going > to sleep/wait, but we have changed some shared memory structure, so > it is good to verify performance tests. I am planning to run read-only > and read-write pgbench tests (when data fits in shared), is that > sufficient or do you expect anything more? I'm open to ideas from others on tests that would be good to run, but I don't have any great ideas myself right now. -- 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
