From: Tom Lane <tgl@sss.pgh.pa.us>
To: Julien Rouhaud <rjuju123@gmail.com>
cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
        Michael Paquier <michael@paquier.xyz>,
        Andres Freund <andres@anarazel.de>,
        pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Add an optional timeout clause to isolationtester step.
In-reply-to: <20200313090450.6crpgoula5nrgadl@nol>
References: <24078.1583958800@sss.pgh.pa.us>
 <20200311205254.GA2648@alvherre.pgsql> <20200313090450.6crpgoula5nrgadl@nol>
Comments: In-reply-to Julien Rouhaud <rjuju123@gmail.com>
	message dated "Fri, 13 Mar 2020 10:04:50 +0100"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <32139.1584108740.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 13 Mar 2020 10:12:20 -0400
Message-ID: <32140.1584108740@sss.pgh.pa.us>
Precedence: bulk

Julien Rouhaud <rjuju123@gmail.com> writes:
> On Wed, Mar 11, 2020 at 05:52:54PM -0300, Alvaro Herrera wrote:
>> On 2020-Mar-11, Tom Lane wrote:
>>> 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.

>> +1.  Those long timeouts are annoying enough that infrastructure to mak=
e
>> a run shorter in normal circumstances might be sufficient justification
>> for this patch ...

> I'm not familiar with those test so I'm probably missing something, but =
looks
> like all isolation tests that setup a timeout are doing so to test serve=
r side
> features (deadlock detection, statement and lock timeout).  I'm not sure=
 how
> adding a client-side facility to detect locks earlier is going to help r=
educing
> the server side timeouts?

The point is that those timeouts have to be set long enough for even a
very slow machine to reach a desired state before the timeout happens;
on faster machines the test is just uselessly sleeping for a long time,
because of the fixed timeout.  My thought was that maybe the tests could
be recast as "watch for session to reach $expected_state and then do
the next thing", allowing them to be automatically adaptive to the
machine's speed.  This might require some rather subtle test redesign
and/or addition of more infrastructure (to allow recognition of the
desired state and/or taking an appropriate next action).  I'm prepared
to believe that not much can be done about timeouts.spec in particular,
but it seems to me that the long delays in the deadlock tests are not
inherent in what we need to test.

> For the REINDEX CONCURRENTLY failure test, the problem that needs to be =
solved
> isn't detecting that the command is blocked as it's already getting bloc=
ked on
> a heavyweight lock, but being able to reliably cancel a specific query a=
s early
> as possible, which AFAICS isn't possible with current isolation tester:

Right, it's the same thing of needing to wait till the backend has reached
a particular state before you do the next thing.

> So we would actually only need something like this to make it work:
> step "<name>" [ CANCEL IF BLOCKED ] { <SQL }

I continue to resist the idea of hard-wiring this feature to query cancel
as the action-to-take.  That will more or less guarantee that it's not
good for anything but this one test case.  I think that the feature
should have the behavior of "treat this step as blocked once it's reached
state X", and then you make the next step in the permutation be one that
issues a query cancel.  (Possibly, using pg_stat_activity and
pg_cancel_backend for that will be painful enough that we'd want to
invent separate script syntax that says "send a cancel to session X".
But that's a separate discussion.)

			regards, tom lane