Re: Add an optional timeout clause to isolationtester step.

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.

> step "<name>" { SQL } [ cancel on "<wait_event_type>" "<wait_event>" ]
>
> to the step definition should be enough. I'm attaching a POC patch for that.
> On my laptop, the full test now complete in about 400ms.

Not much a fan of that per the lack of flexibility, but we have a
single function to avoid a huge performance impact when using
CLOBBER_CACHE_ALWAYS, so we cannot really use a SQL-based logic
either...

> FTR the REINDEX TABLE CONCURRENTLY case is eventually locked on a virtualxid,
> I'm not sure if that's could lead to too early cancellation.

WaitForLockersMultiple() is called three times in this case, but your
test case is waiting on a lock to be released for the old index which
REINDEX CONCURRENTLY would like to drop at the beginning of step 5, so
this should work reliably here.

> + TupleDescInitEntry(tupdesc, (AttrNumber) 3, "wait_even",
> + TEXTOID, -1, 0);
Guess who is missing a 't' here.

pg_isolation_test_session_is_blocked() is not documented and it is
only used internally in the isolation test suite, so breaking its
compatibility should be fine in practice.. Now you are actually
changing it so as we get a more complex state of the blocked
session, so I think that we should use a different function name, and
a different function. Like pg_isolation_test_session_state?
--
Michael