Re: Add an optional timeout clause to isolationtester step.

Michael Paquier <michael@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