Re: Add an optional timeout clause to isolationtester step.

2020-03-13 Thread Tom Lane
Julien Rouhaud writes: > It seems that for all the possibly interesting cases, what we want to wait on > is an heavyweight lock, which is already what isolationtester detects. Maybe > we could simply implement something like > step "" [ WAIT UNTIL BLOCKED ] { } > without any change to the

Re: Add an optional timeout clause to isolationtester step.

2020-03-13 Thread Julien Rouhaud
On Fri, Mar 13, 2020 at 10:12:20AM -0400, Tom Lane wrote: > Julien Rouhaud writes: > > > 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 server > > side > > features (deadlock detection,

Re: Add an optional timeout clause to isolationtester step.

2020-03-13 Thread Tom Lane
Julien Rouhaud 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

Re: Add an optional timeout clause to isolationtester step.

2020-03-13 Thread Julien Rouhaud
On Wed, Mar 11, 2020 at 05:52:54PM -0300, Alvaro Herrera wrote: > On 2020-Mar-11, Tom Lane wrote: > > > We could re-use Julien's ideas about the isolation spec syntax by > > making it be, roughly, > > > > step "" { } [ blocked if "" "" ] > > > > and then those items would need to be passed as

Re: Add an optional timeout clause to isolationtester step.

2020-03-12 Thread Tom Lane
Michael Paquier writes: > +1. A patch does not seem to be that complicated. Now isn't it too > late for v13? I think we've generally given new tests more slack than new features so far as schedule goes. If the patch ends up being complicated/invasive, I might vote to hold it for v14, but

Re: Add an optional timeout clause to isolationtester step.

2020-03-12 Thread Michael Paquier
On Wed, Mar 11, 2020 at 05:52:54PM -0300, Alvaro Herrera wrote: > On 2020-Mar-11, Tom Lane wrote: >> We could re-use Julien's ideas about the isolation spec syntax by >> making it be, roughly, >> >> step "" { } [ blocked if "" "" ] >> >> and then those items would need to be passed as

Re: Add an optional timeout clause to isolationtester step.

2020-03-11 Thread Alvaro Herrera
On 2020-Mar-11, Tom Lane wrote: > We could re-use Julien's ideas about the isolation spec syntax by > making it be, roughly, > > step "" { } [ blocked if "" "" ] > > and then those items would need to be passed as parameters of the prepared > query. I think for test readability's sake, it'd

Re: Add an optional timeout clause to isolationtester step.

2020-03-11 Thread Tom Lane
Michael Paquier 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

Re: Add an optional timeout clause to isolationtester step.

2020-03-10 Thread Michael Paquier
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 ""

Re: Add an optional timeout clause to isolationtester step.

2020-03-10 Thread Julien Rouhaud
On Tue, Mar 10, 2020 at 12:09:12AM -0400, Tom Lane wrote: > Michael Paquier writes: > > On Mon, Mar 09, 2020 at 10:32:27PM -0400, Tom Lane wrote: > >> It strikes me to wonder whether we could improve matters by teaching > >> isolationtester to watch for particular values in a connected backend's

Re: Add an optional timeout clause to isolationtester step.

2020-03-09 Thread Tom Lane
Michael Paquier writes: > On Mon, Mar 09, 2020 at 10:32:27PM -0400, Tom Lane wrote: >> It strikes me to wonder whether we could improve matters by teaching >> isolationtester to watch for particular values in a connected backend's >> pg_stat_activity.wait_event_type/wait_event columns. Those

Re: Add an optional timeout clause to isolationtester step.

2020-03-09 Thread Michael Paquier
On Mon, Mar 09, 2020 at 10:32:27PM -0400, Tom Lane wrote: > It strikes me to wonder whether we could improve matters by teaching > isolationtester to watch for particular values in a connected backend's > pg_stat_activity.wait_event_type/wait_event columns. Those columns > didn't exist when

Re: Add an optional timeout clause to isolationtester step.

2020-03-09 Thread Tom Lane
Michael Paquier writes: > On Mon, Mar 09, 2020 at 03:15:58PM -0700, Andres Freund wrote: >> That kind of thing can already be done using statement_timeout or >> lock_timeout, no? > Yep, still that's not something I would recommend to commit in the > tree as that's a double-edged sword as you

Re: Add an optional timeout clause to isolationtester step.

2020-03-09 Thread Michael Paquier
On Mon, Mar 09, 2020 at 03:15:58PM -0700, Andres Freund wrote: > On 2020-03-07 22:17:09 +0100, Julien Rouhaud wrote: >> For reindex concurrently, a SELECT FOR UPDATE on a different connection can >> ensure that the reindex will be stuck at some point, so canceling the command >> after a long

Re: Add an optional timeout clause to isolationtester step.

2020-03-09 Thread Andres Freund
Hi, On 2020-03-07 22:17:09 +0100, Julien Rouhaud wrote: > For reindex concurrently, a SELECT FOR UPDATE on a different connection can > ensure that the reindex will be stuck at some point, so canceling the command > after a long enough timeout reproduces the original faulty behavior. That kind

Re: Add an optional timeout clause to isolationtester step.

2020-03-09 Thread Julien Rouhaud
On Mon, Mar 09, 2020 at 04:47:27PM +0900, Michael Paquier wrote: > On Sat, Mar 07, 2020 at 10:46:34AM -0500, Tom Lane wrote: > > The arbitrarily-set timeouts that exist in some of the isolation tests > > are horrid kluges that have caused us lots of headaches in the past > > and no doubt will

Re: Add an optional timeout clause to isolationtester step.

2020-03-09 Thread Michael Paquier
On Sat, Mar 07, 2020 at 10:46:34AM -0500, Tom Lane wrote: > The arbitrarily-set timeouts that exist in some of the isolation tests > are horrid kluges that have caused us lots of headaches in the past > and no doubt will again in the future. Aside from occasionally failing > when a machine is

Re: Add an optional timeout clause to isolationtester step.

2020-03-07 Thread Michael Paquier
On Sat, Mar 07, 2020 at 04:23:58PM -0500, Tom Lane wrote: > Hmm, seems like a pretty arbitrary (and slow) way to test that. I'd > envision testing that by setting up a case with an expression index > where the expression is designed to fail at some point partway through > the build -- say, with a

Re: Add an optional timeout clause to isolationtester step.

2020-03-07 Thread Tom Lane
Julien Rouhaud writes: > On Sat, Mar 07, 2020 at 04:09:31PM -0500, Tom Lane wrote: >> Julien Rouhaud writes: >>> On Sat, Mar 07, 2020 at 10:46:34AM -0500, Tom Lane wrote: What's the actual need that you're trying to deal with? >>> Testing the correct behavior of non trivial commands, such

Re: Add an optional timeout clause to isolationtester step.

2020-03-07 Thread Julien Rouhaud
On Sat, Mar 07, 2020 at 04:09:31PM -0500, Tom Lane wrote: > Julien Rouhaud writes: > > On Sat, Mar 07, 2020 at 10:46:34AM -0500, Tom Lane wrote: > >> What's the actual need that you're trying to deal with? > > > Testing the correct behavior of non trivial commands, such as CIC/reindex > >

Re: Add an optional timeout clause to isolationtester step.

2020-03-07 Thread Tom Lane
Julien Rouhaud writes: > On Sat, Mar 07, 2020 at 10:46:34AM -0500, Tom Lane wrote: >> What's the actual need that you're trying to deal with? > Testing the correct behavior of non trivial commands, such as CIC/reindex > concurrently, that fails during the execution. Hmm ... don't see how a

Re: Add an optional timeout clause to isolationtester step.

2020-03-07 Thread Julien Rouhaud
On Sat, Mar 07, 2020 at 10:46:34AM -0500, Tom Lane wrote: > Julien Rouhaud writes: > > On Sat, Mar 07, 2020 at 10:41:42AM +0900, Michael Paquier wrote: > >> On Fri, Mar 06, 2020 at 02:15:47PM +0100, Julien Rouhaud wrote: > >>> Here's a patch to add an optional "timeout val" clause to > >>>

Re: Add an optional timeout clause to isolationtester step.

2020-03-07 Thread Tom Lane
Julien Rouhaud writes: > On Sat, Mar 07, 2020 at 10:41:42AM +0900, Michael Paquier wrote: >> On Fri, Mar 06, 2020 at 02:15:47PM +0100, Julien Rouhaud wrote: >>> Here's a patch to add an optional "timeout val" clause to isolationtester's >>> step definition. When used, isolationtester will

Re: Add an optional timeout clause to isolationtester step.

2020-03-06 Thread Julien Rouhaud
On Sat, Mar 07, 2020 at 10:41:42AM +0900, Michael Paquier wrote: > On Fri, Mar 06, 2020 at 02:15:47PM +0100, Julien Rouhaud wrote: > > Here's a patch to add an optional "timeout val" clause to isolationtester's > > step definition. When used, isolationtester will actively wait on the query > >

Re: Add an optional timeout clause to isolationtester step.

2020-03-06 Thread Michael Paquier
On Fri, Mar 06, 2020 at 02:15:47PM +0100, Julien Rouhaud wrote: > Here's a patch to add an optional "timeout val" clause to isolationtester's > step definition. When used, isolationtester will actively wait on the query > rather than continuing with the permutation next step, and will issue a

Add an optional timeout clause to isolationtester step.

2020-03-06 Thread Julien Rouhaud
On Thu, Mar 05, 2020 at 12:53:54PM +0900, Michael Paquier wrote: > On Wed, Mar 04, 2020 at 09:21:45AM +0100, Julien Rouhaud wrote: > > > Should we add some regression > > tests for that? I guess most of it could be borrowed from the patch > > to fix the toast index issue I sent last week. > > I