Re: RFC: replace pg_stat_activity.waiting with something more descriptive

On Thu, Feb 25, 2016 at 10:31 AM, Amit Kapila <amit.kapila16@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