Michael Paquier <mich...@paquier.xyz> writes: > On Tue, Mar 10, 2020 at 02:53:36PM +0100, Julien Rouhaud wrote: >> So basically we could just change pg_isolation_test_session_is_blocked() to >> also return the wait_event_type and wait_event, and adding something like
> Hmm. I think that Tom has in mind the reasons behind 511540d here. Yeah, that history suggests that we need to be very protective of the performance of the wait-checking query, especially in CLOBBER_CACHE_ALWAYS builds. That being the case, I'm hesitant to consider changing the test function to return a tuple. That'll add quite a lot of overhead due to the cache lookups involved, or so my gut says. I'm also finding the proposed semantics (issue a cancel if wait state X is reached) to be odd and special-purpose. I was envisioning something more like "if wait state X is reached, consider the session to be blocked, the same as if it had reached a heavyweight-lock wait". Then isolationtester would move on to issue another step, which is where I'd envision putting the cancel for that particular test usage. So that idea leads to thinking that the wait-state specification is an input to pg_isolation_test_session_is_blocked, not an output. We could re-use Julien's ideas about the isolation spec syntax by making it be, roughly, step "<name>" { <SQL> } [ blocked if "<wait_event_type>" "<wait_event>" ] and then those items would need to be passed as parameters of the prepared query. Or maybe we should use two different prepared queries depending on whether there's a BLOCKED IF spec. We probably don't need lock-wait detection if we're expecting a wait-state-based block, so maybe we should invent a separate backend function "is this process waiting with this type of wait state" and use that to check the state of a step that has this type of annotation. Just eyeing the proposed test case, I'm wondering whether this will actually be sufficiently fine-grained. It seems like "REINDEX has reached a wait on a virtual XID" is not really all that specific; it could match on other situations, such as blocking on a concurrent tuple update. Maybe it's okay given the restrictive context that we don't expect anything to be happening that the isolation test didn't ask for. I'd like to see an attempt to rewrite some of the existing timeout-dependent test cases to use this facility instead of long timeouts. If we could get rid of the timeouts in the deadlock tests, that'd go a long way towards showing that this idea is actually any good. regards, tom lane