pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedAdd an optional timeout clause to isolationtester step.
26+ messages / 5 participants
[nested] [flat]
* Add an optional timeout clause to isolationtester step.
@ 2020-03-06 13:15 Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 26+ messages in thread
From: Julien Rouhaud @ 2020-03-06 13:15 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Michael Paquier <michael.paquier@gmail.com>; pgsql-hackers
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 have doubts when it comes to use a strategy based on
> pg_cancel_backend() and a match of application_name (see for example
> 5ad72ce but I cannot find the associated thread). I think that we
> could design something more robust here and usable by all tests, with
> two things coming into my mind:
> - A new meta-command for isolation tests to be able to cancel a
> session with PQcancel().
> - Fault injection in the backend.
> For the case of this thread, the cancellation command would be a better
> match.
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 cancel
once the defined timeout is reached. I also added as a POC the previous
regression tests for invalid TOAST indexes, updated to use this new
infrastructure (which won't pass as long as the original bug for invalid TOAST
indexes isn't fixed).
I'll park that in the next commitfest, with a v14 target version.
Attachments:
[text/x-diff] 0001-Add-an-optional-timeout-value-to-isolationtester-ste-v1.patch (7.9K, ../../20200306131547.GA2904@nol/2-0001-Add-an-optional-timeout-value-to-isolationtester-ste-v1.patch)
download | inline diff:
From 8480fa105032e15ef76926f9247b2f5af97223d0 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 6 Mar 2020 13:40:56 +0100
Subject: [PATCH 1/2] Add an optional timeout value to isolationtester steps.
Some sanity checks can require a command to wait on a lock and eventually be
cancelled. The only way to do that was to rely on calls to pg_cancel_backend()
filtering pg_stat_activity view, but this isn't a satisfactory solution as
there's no way to guarantee that only the wanted backend will be canceled.
Instead, add a new optional "timeout val" clause to the step definition. When
this clause is specified, isolationtester will actively wait on that command,
and issue a cancel when the given timeout is reached.
Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/20200305035354.GQ2593%40paquier.xyz
---
src/test/isolation/README | 11 ++++++--
src/test/isolation/isolationtester.c | 42 ++++++++++++++++++++++++----
src/test/isolation/isolationtester.h | 2 ++
src/test/isolation/specparse.y | 18 ++++++++++--
src/test/isolation/specscanner.l | 8 ++++++
5 files changed, 70 insertions(+), 11 deletions(-)
diff --git a/src/test/isolation/README b/src/test/isolation/README
index 217953d183..827bea0c42 100644
--- a/src/test/isolation/README
+++ b/src/test/isolation/README
@@ -86,10 +86,12 @@ session "<name>"
Each step has the syntax
- step "<name>" { <SQL> }
+ step "<name>" { <SQL> } [ TIMEOUT seconds ]
- where <name> is a name identifying this step, and SQL is a SQL statement
- (or statements, separated by semicolons) that is executed in the step.
+ where <name> is a name identifying this step, SQL is a SQL statement
+ (or statements, separated by semicolons) that is executed in the step and
+ seconds an optional timeout for the given SQL statement(s) to wait on before
+ canceling it.
Step names must be unique across the whole spec file.
permutation "<step name>" ...
@@ -125,6 +127,9 @@ after PGISOLATIONTIMEOUT seconds. If the cancel doesn't work, isolationtester
will exit uncleanly after a total of twice PGISOLATIONTIMEOUT. Testing
invalid permutations should be avoided because they can make the isolation
tests take a very long time to run, and they serve no useful testing purpose.
+If a test specified the option timeout specification, then isolationtester will
+actively wait for the step commands completion rather than continuing with the
+permutation next step, and send a cancel once the given timeout is reached.
Note that isolationtester recognizes that a command has blocked by looking
to see if it is shown as waiting in the pg_locks view; therefore, only
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index f80261c022..dd5d335027 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -120,10 +120,28 @@ main(int argc, char **argv)
spec_yyparse();
testspec = &parseresult;
- /* Create a lookup table of all steps. */
+ /*
+ * Create a lookup table of all steps and validate any timeout
+ * specification.
+ */
nallsteps = 0;
for (i = 0; i < testspec->nsessions; i++)
+ {
nallsteps += testspec->sessions[i]->nsteps;
+ for (j = 0; j < testspec->sessions[i]->nsteps; j++)
+ {
+ if ((testspec->sessions[i]->steps[j]->timeout * USECS_PER_SEC) >=
+ (max_step_wait /2))
+ {
+ fprintf(stderr, "step %s: step timeout (%d) should be less"
+ " than global timeout (%ld)",
+ testspec->sessions[i]->steps[j]->name,
+ testspec->sessions[i]->steps[j]->timeout,
+ (max_step_wait / USECS_PER_SEC));
+ exit(1);
+ }
+ }
+ }
allsteps = pg_malloc(nallsteps * sizeof(Step *));
@@ -587,8 +605,14 @@ run_permutation(TestSpec *testspec, int nsteps, Step **steps)
exit(1);
}
- /* Try to complete this step without blocking. */
- mustwait = try_complete_step(testspec, step, STEP_NONBLOCK);
+ /*
+ * Try to complete this step without blocking, unless the step has a
+ * timeout.
+ */
+ mustwait = try_complete_step(testspec, step,
+ (step->timeout == 0 ? STEP_NONBLOCK : 0));
+ if (step->timeout != 0)
+ report_error_message(step);
/* Check for completion of any steps that were previously waiting. */
w = 0;
@@ -721,6 +745,7 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
{
struct timeval current_time;
int64 td;
+ int64 step_timeout;
/* If it's OK for the step to block, check whether it has. */
if (flags & STEP_NONBLOCK)
@@ -778,6 +803,11 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
td *= USECS_PER_SEC;
td += (int64) current_time.tv_usec - (int64) start_time.tv_usec;
+ if (step->timeout)
+ step_timeout = step->timeout * USECS_PER_SEC;
+ else
+ step_timeout = max_step_wait;
+
/*
* After max_step_wait microseconds, try to cancel the query.
*
@@ -787,7 +817,7 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
* failing, but remaining permutations and tests should still be
* OK.
*/
- if (td > max_step_wait && !canceled)
+ if (td > step_timeout && !canceled)
{
PGcancel *cancel = PQgetCancel(conn);
@@ -812,13 +842,13 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
}
/*
- * After twice max_step_wait, just give up and die.
+ * After twice the step timeout, just give up and die.
*
* Since cleanup steps won't be run in this case, this may cause
* later tests to fail. That stinks, but it's better than waiting
* forever for the server to respond to the cancel.
*/
- if (td > 2 * max_step_wait)
+ if (td > 2 * step_timeout)
{
fprintf(stderr, "step %s timed out after %d seconds\n",
step->name, (int) (td / USECS_PER_SEC));
diff --git a/src/test/isolation/isolationtester.h b/src/test/isolation/isolationtester.h
index 9cf5012416..0a0dd54ff3 100644
--- a/src/test/isolation/isolationtester.h
+++ b/src/test/isolation/isolationtester.h
@@ -33,6 +33,8 @@ struct Step
char *name;
char *sql;
char *errormsg;
+ int timeout;
+ struct timeval start_time;
};
typedef struct
diff --git a/src/test/isolation/specparse.y b/src/test/isolation/specparse.y
index 5e007e1bf0..76842c42ab 100644
--- a/src/test/isolation/specparse.y
+++ b/src/test/isolation/specparse.y
@@ -25,6 +25,7 @@ TestSpec parseresult; /* result of parsing is left here */
%union
{
char *str;
+ int ival;
Session *session;
Step *step;
Permutation *permutation;
@@ -43,9 +44,11 @@ TestSpec parseresult; /* result of parsing is left here */
%type <session> session
%type <step> step
%type <permutation> permutation
+%type <ival> opt_timeout
%token <str> sqlblock string_literal
-%token PERMUTATION SESSION SETUP STEP TEARDOWN TEST
+%token <ival> INTEGER
+%token PERMUTATION SESSION SETUP STEP TEARDOWN TEST TIMEOUT
%%
@@ -140,16 +143,27 @@ step_list:
step:
- STEP string_literal sqlblock
+ STEP string_literal sqlblock opt_timeout
{
$$ = pg_malloc(sizeof(Step));
$$->name = $2;
$$->sql = $3;
$$->used = false;
$$->errormsg = NULL;
+ $$->timeout = $4;
}
;
+opt_timeout:
+ TIMEOUT INTEGER
+ {
+ $$ = $2;
+ }
+ | /* EMPTY */
+ {
+ $$ = 0;
+ }
+ ;
opt_permutation_list:
permutation_list
diff --git a/src/test/isolation/specscanner.l b/src/test/isolation/specscanner.l
index 410f17727e..1ec1812569 100644
--- a/src/test/isolation/specscanner.l
+++ b/src/test/isolation/specscanner.l
@@ -53,6 +53,7 @@ session { return SESSION; }
setup { return SETUP; }
step { return STEP; }
teardown { return TEARDOWN; }
+timeout { return TIMEOUT; }
[\n] { yyline++; }
{comment} { /* ignore */ }
@@ -96,6 +97,13 @@ teardown { return TEARDOWN; }
yyerror("unterminated sql block");
}
+ /* integer */
+[0-9]+ {
+ yylval.ival = atoi(yytext);
+ return INTEGER;
+
+ }
+
. {
fprintf(stderr, "syntax error at line %d: unexpected character \"%s\"\n", yyline, yytext);
exit(1);
--
2.25.1
[text/x-diff] 0002-Add-regression-tests-for-failed-REINDEX-TABLE-CONCUR-v2.patch (4.7K, ../../20200306131547.GA2904@nol/3-0002-Add-regression-tests-for-failed-REINDEX-TABLE-CONCUR-v2.patch)
download | inline diff:
From 1d112c1bbb563a76198435d60047e8a2b96bcd4a Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 6 Mar 2020 13:51:35 +0100
Subject: [PATCH 2/2] Add regression tests for failed REINDEX TABLE
CONCURRENTLY.
If a REINDEX TABLE CONCURRENTLY fails on a table having a TOAST table, an
invalid index will be present for the TOAST table. As we only allow to drop
invalid indexes on TOAST tables, reindexing those would lead to useless
duplicated indexes that can't be dropped anymore.
Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
---
.../expected/reindex-concurrently.out | 54 ++++++++++++++++++-
.../isolation/specs/reindex-concurrently.spec | 19 +++++++
2 files changed, 72 insertions(+), 1 deletion(-)
diff --git a/src/test/isolation/expected/reindex-concurrently.out b/src/test/isolation/expected/reindex-concurrently.out
index 9e04169b2f..69a55d3788 100644
--- a/src/test/isolation/expected/reindex-concurrently.out
+++ b/src/test/isolation/expected/reindex-concurrently.out
@@ -1,4 +1,4 @@
-Parsed test spec with 3 sessions
+Parsed test spec with 5 sessions
starting permutation: reindex sel1 upd2 ins2 del2 end1 end2
step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
@@ -76,3 +76,55 @@ step end1: COMMIT;
step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
step end2: COMMIT;
step reindex: <... completed>
+
+starting permutation: check_invalid lock reindex_timeout unlock check_invalid nowarn normal_reindex check_invalid reindex check_invalid
+step check_invalid: SELECT i.indisvalid
+ FROM pg_class c
+ JOIN pg_class t ON t.oid = c.reltoastrelid
+ JOIN pg_index i ON i.indrelid = t.oid
+ WHERE c.relname = 'reind_con_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+t
+step lock: BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE;
+data
+
+aa
+isolationtester: canceling step reindex_timeout after 1 seconds
+step reindex_timeout: REINDEX TABLE CONCURRENTLY reind_con_tab;
+ERROR: canceling statement due to user request
+step unlock: COMMIT;
+step check_invalid: SELECT i.indisvalid
+ FROM pg_class c
+ JOIN pg_class t ON t.oid = c.reltoastrelid
+ JOIN pg_index i ON i.indrelid = t.oid
+ WHERE c.relname = 'reind_con_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
+step nowarn: SET client_min_messages = 'ERROR';
+step normal_reindex: REINDEX TABLE reind_con_tab;
+step check_invalid: SELECT i.indisvalid
+ FROM pg_class c
+ JOIN pg_class t ON t.oid = c.reltoastrelid
+ JOIN pg_index i ON i.indrelid = t.oid
+ WHERE c.relname = 'reind_con_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
+step check_invalid: SELECT i.indisvalid
+ FROM pg_class c
+ JOIN pg_class t ON t.oid = c.reltoastrelid
+ JOIN pg_index i ON i.indrelid = t.oid
+ WHERE c.relname = 'reind_con_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
diff --git a/src/test/isolation/specs/reindex-concurrently.spec b/src/test/isolation/specs/reindex-concurrently.spec
index eb59fe0cba..dc67e2afd2 100644
--- a/src/test/isolation/specs/reindex-concurrently.spec
+++ b/src/test/isolation/specs/reindex-concurrently.spec
@@ -31,6 +31,22 @@ step "end2" { COMMIT; }
session "s3"
step "reindex" { REINDEX TABLE CONCURRENTLY reind_con_tab; }
+step "reindex_timeout" { REINDEX TABLE CONCURRENTLY reind_con_tab; } timeout 1
+step "nowarn" { SET client_min_messages = 'ERROR'; }
+
+session "s4"
+step "lock" { BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE; }
+step "unlock" { COMMIT; }
+
+session "s5"
+setup { SET client_min_messages = 'WARNING'; }
+step "normal_reindex" { REINDEX TABLE reind_con_tab; }
+step "check_invalid" {SELECT i.indisvalid
+ FROM pg_class c
+ JOIN pg_class t ON t.oid = c.reltoastrelid
+ JOIN pg_index i ON i.indrelid = t.oid
+ WHERE c.relname = 'reind_con_tab'
+ ORDER BY i.indisvalid::text COLLATE "C"; }
permutation "reindex" "sel1" "upd2" "ins2" "del2" "end1" "end2"
permutation "sel1" "reindex" "upd2" "ins2" "del2" "end1" "end2"
@@ -38,3 +54,6 @@ permutation "sel1" "upd2" "reindex" "ins2" "del2" "end1" "end2"
permutation "sel1" "upd2" "ins2" "reindex" "del2" "end1" "end2"
permutation "sel1" "upd2" "ins2" "del2" "reindex" "end1" "end2"
permutation "sel1" "upd2" "ins2" "del2" "end1" "reindex" "end2"
+permutation "check_invalid" "lock" "reindex_timeout" "unlock" "check_invalid"
+ "nowarn" "normal_reindex" "check_invalid"
+ "reindex" "check_invalid"
--
2.25.1
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-07 01:41 Michael Paquier <michael@paquier.xyz>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 26+ messages in thread
From: Michael Paquier @ 2020-03-07 01:41 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Michael Paquier <michael.paquier@gmail.com>; pgsql-hackers
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 cancel
> once the defined timeout is reached. I also added as a POC the previous
> regression tests for invalid TOAST indexes, updated to use this new
> infrastructure (which won't pass as long as the original bug for invalid TOAST
> indexes isn't fixed).
One problem with this approach is that it does address the stability
of the test on very slow machines, and there are some of them in the
buildfarm. Taking your patch, I can make the test fail by applying
the following sleep because the query would be cancelled before some
of the indexes are marked as invalid:
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3046,6 +3046,8 @@ ReindexRelationConcurrently(Oid relationOid, int
options)
CommitTransactionCommand();
StartTransactionCommand();
+ pg_usleep(100000L * 10); /* 10s */
+
/*
* Phase 2 of REINDEX CONCURRENTLY
Another problem is that on faster machines the test is slow because of
the timeout used. What are your thoughts about having instead a
cancel meta-command instead?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200307014142.GC1531@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-07 06:16 Julien Rouhaud <rjuju123@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 26+ messages in thread
From: Julien Rouhaud @ 2020-03-07 06:16 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Michael Paquier <michael.paquier@gmail.com>; pgsql-hackers
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
> > rather than continuing with the permutation next step, and will issue a cancel
> > once the defined timeout is reached. I also added as a POC the previous
> > regression tests for invalid TOAST indexes, updated to use this new
> > infrastructure (which won't pass as long as the original bug for invalid TOAST
> > indexes isn't fixed).
>
> One problem with this approach is that it does address the stability
> of the test on very slow machines, and there are some of them in the
> buildfarm. Taking your patch, I can make the test fail by applying
> the following sleep because the query would be cancelled before some
> of the indexes are marked as invalid:
> --- a/src/backend/commands/indexcmds.c
> +++ b/src/backend/commands/indexcmds.c
> @@ -3046,6 +3046,8 @@ ReindexRelationConcurrently(Oid relationOid, int
> options)
> CommitTransactionCommand();
> StartTransactionCommand();
>
> + pg_usleep(100000L * 10); /* 10s */
> +
> /*
> * Phase 2 of REINDEX CONCURRENTLY
>
> Another problem is that on faster machines the test is slow because of
> the timeout used. What are your thoughts about having instead a
> cancel meta-command instead?
Looking at timeouts.spec and e.g. a7921f71a3c, it seems that we already chose
to fix this problem by having a timeout long enough to statisfy the slower
buildfarm members, even when running on fast machines, so I assumed that the
same approach could be used here.
I agree that the 1s timeout I used is maybe too low, but that's easy enough to
change. Another point is that it's possible to have a close behavior without
this patch by using a statement_timeout (the active wait does change things
though), but the spec files would be more verbose.
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-07 15:46 Tom Lane <tgl@sss.pgh.pa.us>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 2 replies; 26+ messages in thread
From: Tom Lane @ 2020-03-07 15:46 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Michael Paquier <michael.paquier@gmail.com>; pgsql-hackers
Julien Rouhaud <rjuju123@gmail.com> 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 actively wait on the query
>>> rather than continuing with the permutation next step, and will issue a cancel
>>> once the defined timeout is reached.
>> One problem with this approach is that it does address the stability
>> of the test on very slow machines, and there are some of them in the
>> buildfarm.
> Looking at timeouts.spec and e.g. a7921f71a3c, it seems that we already chose
> to fix this problem by having a timeout long enough to statisfy the slower
> buildfarm members, even when running on fast machines, so I assumed that the
> same approach could be used here.
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 particularly overloaded, they cause the tests to
take far longer than necessary on decently-fast machines. So ideally
we'd get rid of those entirely in favor of some more-dynamic approach.
Admittedly, I have no proposal for what that would be. But adding yet
more ways to set a (guaranteed-to-be-wrong) timeout seems like the
wrong direction to be going in. What's the actual need that you're
trying to deal with?
regards, tom lane
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-07 20:53 Julien Rouhaud <rjuju123@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 26+ messages in thread
From: Julien Rouhaud @ 2020-03-07 20:53 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; Michael Paquier <michael.paquier@gmail.com>; pgsql-hackers
On Sat, Mar 07, 2020 at 10:46:34AM -0500, Tom Lane wrote:
> Julien Rouhaud <rjuju123@gmail.com> 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 actively wait on the query
> >>> rather than continuing with the permutation next step, and will issue a cancel
> >>> once the defined timeout is reached.
>
> >> One problem with this approach is that it does address the stability
> >> of the test on very slow machines, and there are some of them in the
> >> buildfarm.
>
> > Looking at timeouts.spec and e.g. a7921f71a3c, it seems that we already chose
> > to fix this problem by having a timeout long enough to statisfy the slower
> > buildfarm members, even when running on fast machines, so I assumed that the
> > same approach could be used here.
>
> 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 particularly overloaded, they cause the tests to
> take far longer than necessary on decently-fast machines.
Yeah, I have no doubt that it has been a pain, and this patch is clearly not a
bullet-proof solution.
> So ideally
> we'd get rid of those entirely in favor of some more-dynamic approach.
> Admittedly, I have no proposal for what that would be.
The fault injection framework that was previously discussed would cover most of
the usecase I can think of, but that's a way bigger project.
> But adding yet
> more ways to set a (guaranteed-to-be-wrong) timeout seems like the
> wrong direction to be going in.
Fair enough, I'll mark the patch as rejected then.
> 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.
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-07 21:09 Tom Lane <tgl@sss.pgh.pa.us>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 26+ messages in thread
From: Tom Lane @ 2020-03-07 21:09 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers
Julien Rouhaud <rjuju123@gmail.com> 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 timeout helps with that?
regards, tom lane
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-07 21:17 Julien Rouhaud <rjuju123@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 26+ messages in thread
From: Julien Rouhaud @ 2020-03-07 21:17 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers
On Sat, Mar 07, 2020 at 04:09:31PM -0500, Tom Lane wrote:
> Julien Rouhaud <rjuju123@gmail.com> 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 timeout helps with that?
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.
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-07 21:23 Tom Lane <tgl@sss.pgh.pa.us>
parent: Julien Rouhaud <rjuju123@gmail.com>
1 sibling, 1 reply; 26+ messages in thread
From: Tom Lane @ 2020-03-07 21:23 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers
Julien Rouhaud <rjuju123@gmail.com> writes:
> On Sat, Mar 07, 2020 at 04:09:31PM -0500, Tom Lane wrote:
>> Julien Rouhaud <rjuju123@gmail.com> 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 timeout helps with that?
> 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.
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 divide-by-zero triggered by one of the tuples
to be indexed.
regards, tom lane
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-08 03:44 Michael Paquier <michael@paquier.xyz>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 26+ messages in thread
From: Michael Paquier @ 2020-03-08 03:44 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; pgsql-hackers
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 divide-by-zero triggered by one of the tuples
> to be indexed.
I am not sure that I think that's very tricky to get an invalid index
_ccold after the swap phase with what the existing test facility
provides, because the new index is already built at the point where
the dependencies are switched so you cannot rely on a failure when
building the index. Note also that some tests of CREATE INDEX
CONCURRENTLY rely on the uniqueness to create invalid index entries
(division by zero is fine as well). And, actually, if you rely on
that, you can get invalid _ccnew entries easily:
create table aa (a int);
insert into aa values (1),(1);
create unique index concurrently aai on aa (a);
reindex index concurrently aai;
=# \d aa
Table "public.aa"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
a | integer | | |
Indexes:
"aai" UNIQUE, btree (a) INVALID
"aai_ccnew" UNIQUE, btree (a) INVALID
That's before the dependency swapping is done though... With a fault
injection facility, it would be possible to test the stability of
the operation by enforcing for example failures after the start of
each inner transaction of REINDEX CONCURRENTLY.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200308034401.GB56468@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-09 07:47 Michael Paquier <michael@paquier.xyz>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 26+ messages in thread
From: Michael Paquier @ 2020-03-09 07:47 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Michael Paquier <michael.paquier@gmail.com>; pgsql-hackers
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 particularly overloaded, they cause the tests to
> take far longer than necessary on decently-fast machines. So ideally
> we'd get rid of those entirely in favor of some more-dynamic approach.
> Admittedly, I have no proposal for what that would be. But adding yet
> more ways to set a (guaranteed-to-be-wrong) timeout seems like the
> wrong direction to be going in. What's the actual need that you're
> trying to deal with?
As a matter of fact, the buildfarm member petalura just reported a
failure with the isolation test "timeouts", the machine being
extremely slow:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=petalura&dt=2020-03-08%2011%3A20%3A05
test timeouts ... FAILED 60330 ms
[...]
-step update: DELETE FROM accounts WHERE accountid = 'checking'; <waiting ...>
-step update: <... completed>
+step update: DELETE FROM accounts WHERE accountid = 'checking';
ERROR: canceling statement due to statement timeout
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200309074727.GE96055@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-09 08:39 Julien Rouhaud <rjuju123@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 26+ messages in thread
From: Julien Rouhaud @ 2020-03-09 08:39 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Michael Paquier <michael.paquier@gmail.com>; pgsql-hackers
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 again in the future. Aside from occasionally failing
> > when a machine is particularly overloaded, they cause the tests to
> > take far longer than necessary on decently-fast machines. So ideally
> > we'd get rid of those entirely in favor of some more-dynamic approach.
> > Admittedly, I have no proposal for what that would be. But adding yet
> > more ways to set a (guaranteed-to-be-wrong) timeout seems like the
> > wrong direction to be going in. What's the actual need that you're
> > trying to deal with?
>
> As a matter of fact, the buildfarm member petalura just reported a
> failure with the isolation test "timeouts", the machine being
> extremely slow:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=petalura&dt=2020-03-08%2011%3A20%3A05
>
> test timeouts ... FAILED 60330 ms
> [...]
> -step update: DELETE FROM accounts WHERE accountid = 'checking'; <waiting ...>
> -step update: <... completed>
> +step update: DELETE FROM accounts WHERE accountid = 'checking';
> ERROR: canceling statement due to statement timeout
Indeed. I guess we could add some kind of environment variable facility in
isolationtester to let slow machine owner put a way bigger timeout without
making the test super slow for everyone else, but that seems overkill for just
one test, and given the other thread about deploying REL_11 build-farm client,
that wouldn't be an immediate fix either.
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-09 22:15 Andres Freund <andres@anarazel.de>
parent: Julien Rouhaud <rjuju123@gmail.com>
1 sibling, 1 reply; 26+ messages in thread
From: Andres Freund @ 2020-03-09 22:15 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Michael Paquier <michael@paquier.xyz>; pgsql-hackers
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 of thing can already be done using statement_timeout or
lock_timeout, no?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-10 02:14 Michael Paquier <michael@paquier.xyz>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 26+ messages in thread
From: Michael Paquier @ 2020-03-10 02:14 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
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 enough timeout reproduces the original faulty behavior.
>
> 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 already know. For slower
machines, you need a statement_timeout large enough so as you make
sure that the state you want the query to wait for is reached, which
has a cost on all other faster machines as it makes the tests slower.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200310021459.GA4369@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-10 02:32 Tom Lane <tgl@sss.pgh.pa.us>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 26+ messages in thread
From: Tom Lane @ 2020-03-10 02:32 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Andres Freund <andres@anarazel.de>; Julien Rouhaud <rjuju123@gmail.com>; pgsql-hackers
Michael Paquier <michael@paquier.xyz> 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 already know. For slower
> machines, you need a statement_timeout large enough so as you make
> sure that the state you want the query to wait for is reached, which
> has a cost on all other faster machines as it makes the tests slower.
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 isolationtester was designed, IIRC, so it's not
surprising that they're not used in the current design. But we could
use them perhaps to detect that a backend has arrived at some state
that's not a heavyweight-lock-wait state.
regards, tom lane
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-10 02:55 Michael Paquier <michael@paquier.xyz>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 26+ messages in thread
From: Michael Paquier @ 2020-03-10 02:55 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andres Freund <andres@anarazel.de>; Julien Rouhaud <rjuju123@gmail.com>; pgsql-hackers
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 isolationtester was designed, IIRC, so it's not
> surprising that they're not used in the current design. But we could
> use them perhaps to detect that a backend has arrived at some state
> that's not a heavyweight-lock-wait state.
Interesting idea. So that would be basically an equivalent of
PostgresNode::poll_query_until but for the isolation tester? In short
we gain a meta-command that runs a SELECT query that waits until the
query defined in the command returns true. The polling interval may
be tricky to set though. If set too low it would consume resources
for nothing, and if set too large it would make the tests using this
meta-command slower than they actually need to be. Perhaps something
like 100ms may be fine..
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200310025536.GC4369@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-10 04:09 Tom Lane <tgl@sss.pgh.pa.us>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 26+ messages in thread
From: Tom Lane @ 2020-03-10 04:09 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Andres Freund <andres@anarazel.de>; Julien Rouhaud <rjuju123@gmail.com>; pgsql-hackers
Michael Paquier <michael@paquier.xyz> 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 columns
>> didn't exist when isolationtester was designed, IIRC, so it's not
>> surprising that they're not used in the current design. But we could
>> use them perhaps to detect that a backend has arrived at some state
>> that's not a heavyweight-lock-wait state.
> Interesting idea. So that would be basically an equivalent of
> PostgresNode::poll_query_until but for the isolation tester?
No, more like the existing isolationtester wait query, which watches
for something being blocked on a heavyweight lock. Right now, that
one depends on a bespoke function pg_isolation_test_session_is_blocked(),
but it used to be a query on pg_stat_activity/pg_locks.
> In short
> we gain a meta-command that runs a SELECT query that waits until the
> query defined in the command returns true. The polling interval may
> be tricky to set though.
I think it'd be just the same as the polling interval for the existing
wait query. We'd have to have some way to mark a script step to say
what to check to decide that it's blocked ...
regards, tom lane
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-10 13:53 Julien Rouhaud <rjuju123@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 26+ messages in thread
From: Julien Rouhaud @ 2020-03-10 13:53 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; Andres Freund <andres@anarazel.de>; pgsql-hackers
On Tue, Mar 10, 2020 at 12:09:12AM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> 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 columns
> >> didn't exist when isolationtester was designed, IIRC, so it's not
> >> surprising that they're not used in the current design. But we could
> >> use them perhaps to detect that a backend has arrived at some state
> >> that's not a heavyweight-lock-wait state.
>
> > Interesting idea. So that would be basically an equivalent of
> > PostgresNode::poll_query_until but for the isolation tester?
>
> No, more like the existing isolationtester wait query, which watches
> for something being blocked on a heavyweight lock. Right now, that
> one depends on a bespoke function pg_isolation_test_session_is_blocked(),
> but it used to be a query on pg_stat_activity/pg_locks.
Ah interesting indeed!
> > In short
> > we gain a meta-command that runs a SELECT query that waits until the
> > query defined in the command returns true. The polling interval may
> > be tricky to set though.
>
> I think it'd be just the same as the polling interval for the existing
> wait query. We'd have to have some way to mark a script step to say
> what to check to decide that it's blocked ...
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
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.
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.
Attachments:
[text/x-diff] v2-0001-Add-an-optional-cancel-on-clause-to-isolationtest.patch (13.1K, ../../20200310135336.zi3mgmq6fub3jfek@nol/2-v2-0001-Add-an-optional-cancel-on-clause-to-isolationtest.patch)
download | inline diff:
From 77c214c9fb2fa7f9a003b96db0e5ec6506217e38 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 6 Mar 2020 13:40:56 +0100
Subject: [PATCH v2 1/2] Add an optional cancel on clause to isolationtester
steps.
Some sanity checks can require a command to wait on a lock and eventually be
cancelled. The only way to do that was to rely on calls to pg_cancel_backend()
filtering pg_stat_activity view, but this isn't a satisfactory solution as
there's no way to guarantee that only the wanted backend will be canceled.
Instead, add a new optional "cancel on <wait_event_type> <wait_event>" clause
to the step definition. When this clause is specified, isolationtester will
actively wait on that command, and issue a cancel when the query is waiting on
the given wait event.
Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/20200305035354.GQ2593%40paquier.xyz
---
src/backend/utils/adt/lockfuncs.c | 54 ++++++++++++++++++++++++++--
src/include/catalog/pg_proc.dat | 5 ++-
src/test/isolation/README | 12 +++++--
src/test/isolation/isolationtester.c | 43 +++++++++++++++++-----
src/test/isolation/isolationtester.h | 8 +++++
src/test/isolation/specparse.y | 20 +++++++++--
src/test/isolation/specscanner.l | 2 ++
7 files changed, 127 insertions(+), 17 deletions(-)
diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c
index ecb1bf92ff..cc2bc94d0d 100644
--- a/src/backend/utils/adt/lockfuncs.c
+++ b/src/backend/utils/adt/lockfuncs.c
@@ -17,7 +17,9 @@
#include "catalog/pg_type.h"
#include "funcapi.h"
#include "miscadmin.h"
+#include "pgstat.h"
#include "storage/predicate_internals.h"
+#include "storage/procarray.h"
#include "utils/array.h"
#include "utils/builtins.h"
@@ -578,17 +580,28 @@ pg_safe_snapshot_blocking_pids(PG_FUNCTION_ARGS)
Datum
pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
{
+ TupleDesc tupdesc;
int blocked_pid = PG_GETARG_INT32(0);
ArrayType *interesting_pids_a = PG_GETARG_ARRAYTYPE_P(1);
ArrayType *blocking_pids_a;
int32 *interesting_pids;
int32 *blocking_pids;
+ Datum values[3];
+ bool nulls[3];
+ PGPROC *proc;
+ uint32 raw_wait_event = 0;
+ const char *wait_event_type = NULL;
+ const char *wait_event = NULL;
int num_interesting_pids;
int num_blocking_pids;
int dummy;
int i,
j;
+ /* Initialise values and NULL flags arrays */
+ MemSet(values, 0, sizeof(values));
+ MemSet(nulls, 0, sizeof(nulls));
+
/* Validate the passed-in array */
Assert(ARR_ELEMTYPE(interesting_pids_a) == INT4OID);
if (array_contains_nulls(interesting_pids_a))
@@ -597,6 +610,34 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
num_interesting_pids = ArrayGetNItems(ARR_NDIM(interesting_pids_a),
ARR_DIMS(interesting_pids_a));
+ /* Initialise attributes information in the tuple descriptor */
+ tupdesc = CreateTemplateTupleDesc(3);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 1, "blocked",
+ BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 2, "wait_event_type",
+ TEXTOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 3, "wait_even",
+ TEXTOID, -1, 0);
+
+ BlessTupleDesc(tupdesc);
+
+ proc = BackendPidGetProc(blocked_pid);
+ if (proc)
+ {
+#define UINT32_ACCESS_ONCE(var) ((uint32)(*((volatile uint32 *)&(var))))
+ raw_wait_event = UINT32_ACCESS_ONCE(proc->wait_event_info);
+ wait_event_type = pgstat_get_wait_event_type(raw_wait_event);
+ wait_event = pgstat_get_wait_event(raw_wait_event);
+
+ if (wait_event_type != NULL)
+ values[1] = CStringGetTextDatum(wait_event_type);
+ if (wait_event != NULL)
+ values[2] = CStringGetTextDatum(wait_event);
+ }
+
+ nulls[1] = (wait_event_type == NULL);
+ nulls[2] = (wait_event == NULL);
+
/*
* Get the PIDs of all sessions blocking the given session's attempt to
* acquire heavyweight locks.
@@ -623,7 +664,11 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
for (j = 0; j < num_interesting_pids; j++)
{
if (blocking_pids[i] == interesting_pids[j])
- PG_RETURN_BOOL(true);
+ {
+ values[0] = BoolGetDatum(true);
+ PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc,
+ values, nulls)));
+ }
}
/*
@@ -636,9 +681,12 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
* buffer and check if the number of safe snapshot blockers is non-zero.
*/
if (GetSafeSnapshotBlockingPids(blocked_pid, &dummy, 1) > 0)
- PG_RETURN_BOOL(true);
+ values[0] = BoolGetDatum(true);
+
+ values[0] = BoolGetDatum(false);
- PG_RETURN_BOOL(false);
+ PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc,
+ values, nulls)));
}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7fb574f9dc..77529e181d 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5869,7 +5869,10 @@
prosrc => 'pg_safe_snapshot_blocking_pids' },
{ oid => '3378', descr => 'isolationtester support function',
proname => 'pg_isolation_test_session_is_blocked', provolatile => 'v',
- prorettype => 'bool', proargtypes => 'int4 _int4',
+ prorettype => 'record', proargtypes => 'int4 _int4',
+ proallargtypes => '{int4,_int4,bool,text,text}',
+ proargmodes => '{i,i,o,o,o}',
+ proargnames => '{blocking_pid,interesting_pids,blocked,wait_event_type,wait_event}',
prosrc => 'pg_isolation_test_session_is_blocked' },
{ oid => '1065', descr => 'view two-phase transactions',
proname => 'pg_prepared_xact', prorows => '1000', proretset => 't',
diff --git a/src/test/isolation/README b/src/test/isolation/README
index 217953d183..04aed5cd17 100644
--- a/src/test/isolation/README
+++ b/src/test/isolation/README
@@ -86,10 +86,12 @@ session "<name>"
Each step has the syntax
- step "<name>" { <SQL> }
+ step "<name>" { <SQL> } [ cancel on "<wait_event_type>" "<wait_event>" ]
- where <name> is a name identifying this step, and SQL is a SQL statement
- (or statements, separated by semicolons) that is executed in the step.
+ where <name> is a name identifying this step, SQL is a SQL statement
+ (or statements, separated by semicolons) that is executed in the step and
+ <wait_event_type> and <wait_event> a wait event specification for which
+ isolationtester will cancel the query if it's blocked on it.
Step names must be unique across the whole spec file.
permutation "<step name>" ...
@@ -125,6 +127,10 @@ after PGISOLATIONTIMEOUT seconds. If the cancel doesn't work, isolationtester
will exit uncleanly after a total of twice PGISOLATIONTIMEOUT. Testing
invalid permutations should be avoided because they can make the isolation
tests take a very long time to run, and they serve no useful testing purpose.
+If a test specified the optionnal cancel on specification, then isolationtester
+will actively wait for the step commands completion rather than continuing with
+the permutation next step, and send a cancel once the given wait event is
+blocking the query.
Note that isolationtester recognizes that a command has blocked by looking
to see if it is shown as waiting in the pg_locks view; therefore, only
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index f80261c022..f530d9923d 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -43,6 +43,7 @@ static void run_permutation(TestSpec *testspec, int nsteps, Step **steps);
#define STEP_NONBLOCK 0x1 /* return 0 as soon as cmd waits for a lock */
#define STEP_RETRY 0x2 /* this is a retry of a previously-waiting cmd */
+#define STEP_WAIT 0x4 /* active wait for a given wait event */
static bool try_complete_step(TestSpec *testspec, Step *step, int flags);
static int step_qsort_cmp(const void *a, const void *b);
@@ -212,7 +213,7 @@ main(int argc, char **argv)
*/
initPQExpBuffer(&wait_query);
appendPQExpBufferStr(&wait_query,
- "SELECT pg_catalog.pg_isolation_test_session_is_blocked($1, '{");
+ "SELECT * FROM pg_catalog.pg_isolation_test_session_is_blocked($1, '{");
/* The spec syntax requires at least one session; assume that here. */
appendPQExpBufferStr(&wait_query, backend_pid_strs[1]);
for (i = 2; i < nconns; i++)
@@ -587,8 +588,14 @@ run_permutation(TestSpec *testspec, int nsteps, Step **steps)
exit(1);
}
- /* Try to complete this step without blocking. */
- mustwait = try_complete_step(testspec, step, STEP_NONBLOCK);
+ /*
+ * Try to complete this step without blocking, unless the step has a
+ * cancel on clause.
+ */
+ mustwait = try_complete_step(testspec, step,
+ (step->waitinfo ? STEP_WAIT : STEP_NONBLOCK));
+ if (step->waitinfo)
+ report_error_message(step);
/* Check for completion of any steps that were previously waiting. */
w = 0;
@@ -720,10 +727,12 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
else if (ret == 0) /* select() timeout: check for lock wait */
{
struct timeval current_time;
+ char *wait_event_type = "";
+ char *wait_event = "";
int64 td;
/* If it's OK for the step to block, check whether it has. */
- if (flags & STEP_NONBLOCK)
+ if (flags & (STEP_NONBLOCK | STEP_WAIT))
{
bool waiting;
@@ -738,9 +747,17 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
exit(1);
}
waiting = ((PQgetvalue(res, 0, 0))[0] == 't');
+ if (waiting && (flags & STEP_WAIT))
+ {
+ wait_event_type = pg_strdup(PQgetvalue(res, 0, 1));
+ Assert(wait_event_type != NULL);
+
+ wait_event = pg_strdup(PQgetvalue(res, 0, 2));
+ Assert(wait_event != NULL);
+ }
PQclear(res);
- if (waiting) /* waiting to acquire a lock */
+ if (waiting && (flags & STEP_NONBLOCK)) /* waiting to acquire a lock */
{
/*
* Since it takes time to perform the lock-check query,
@@ -787,7 +804,11 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
* failing, but remaining permutations and tests should still be
* OK.
*/
- if (td > max_step_wait && !canceled)
+ if ((td > max_step_wait ||
+ (step->waitinfo
+ && strcmp(step->waitinfo->wait_event_type, wait_event_type) == 0
+ && strcmp(step->waitinfo->wait_event, wait_event) == 0))
+ && !canceled)
{
PGcancel *cancel = PQgetCancel(conn);
@@ -801,8 +822,14 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
* print to stdout not stderr, as this should appear
* in the test case's results
*/
- printf("isolationtester: canceling step %s after %d seconds\n",
- step->name, (int) (td / USECS_PER_SEC));
+ if (!step->waitinfo)
+ printf("isolationtester: canceling step %s after %d seconds\n",
+ step->name, (int) (td / USECS_PER_SEC));
+ else
+ printf("isolationtester: canceling step %s on wait event %s/%s\n",
+ step->name,
+ step->waitinfo->wait_event_type,
+ step->waitinfo->wait_event);
canceled = true;
}
else
diff --git a/src/test/isolation/isolationtester.h b/src/test/isolation/isolationtester.h
index 9cf5012416..5df540485c 100644
--- a/src/test/isolation/isolationtester.h
+++ b/src/test/isolation/isolationtester.h
@@ -26,6 +26,12 @@ struct Session
int nsteps;
};
+typedef struct WaitInfo
+{
+ char *wait_event_type;
+ char *wait_event;
+} WaitInfo;
+
struct Step
{
int session;
@@ -33,6 +39,8 @@ struct Step
char *name;
char *sql;
char *errormsg;
+ WaitInfo *waitinfo;
+ struct timeval start_time;
};
typedef struct
diff --git a/src/test/isolation/specparse.y b/src/test/isolation/specparse.y
index 5e007e1bf0..c8e72f4316 100644
--- a/src/test/isolation/specparse.y
+++ b/src/test/isolation/specparse.y
@@ -27,6 +27,7 @@ TestSpec parseresult; /* result of parsing is left here */
char *str;
Session *session;
Step *step;
+ WaitInfo *waitinfo;
Permutation *permutation;
struct
{
@@ -43,9 +44,11 @@ TestSpec parseresult; /* result of parsing is left here */
%type <session> session
%type <step> step
%type <permutation> permutation
+%type <waitinfo> opt_cancel
%token <str> sqlblock string_literal
-%token PERMUTATION SESSION SETUP STEP TEARDOWN TEST
+%token <str> LITERAL
+%token CANCEL ON PERMUTATION SESSION SETUP STEP TEARDOWN TEST
%%
@@ -140,16 +143,29 @@ step_list:
step:
- STEP string_literal sqlblock
+ STEP string_literal sqlblock opt_cancel
{
$$ = pg_malloc(sizeof(Step));
$$->name = $2;
$$->sql = $3;
$$->used = false;
$$->errormsg = NULL;
+ $$->waitinfo = $4;
}
;
+opt_cancel:
+ CANCEL ON string_literal string_literal
+ {
+ $$ = pg_malloc(sizeof(WaitInfo));
+ $$->wait_event_type = $3;
+ $$->wait_event = $4;
+ }
+ | /* EMPTY */
+ {
+ $$ = NULL;
+ }
+ ;
opt_permutation_list:
permutation_list
diff --git a/src/test/isolation/specscanner.l b/src/test/isolation/specscanner.l
index 410f17727e..672e5fa2fc 100644
--- a/src/test/isolation/specscanner.l
+++ b/src/test/isolation/specscanner.l
@@ -48,6 +48,8 @@ comment ("#"{non_newline}*)
litbufsize = LITBUF_INIT;
%}
+cancel { return CANCEL; }
+on { return ON; }
permutation { return PERMUTATION; }
session { return SESSION; }
setup { return SETUP; }
--
2.25.1
[text/x-diff] v2-0002-Add-regression-tests-for-failed-REINDEX-TABLE-CON.patch (4.7K, ../../20200310135336.zi3mgmq6fub3jfek@nol/3-v2-0002-Add-regression-tests-for-failed-REINDEX-TABLE-CON.patch)
download | inline diff:
From 60267b2fb3ff8e3b632ee07d12ee4031543601d3 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 6 Mar 2020 13:51:35 +0100
Subject: [PATCH v2 2/2] Add regression tests for failed REINDEX TABLE
CONCURRENTLY.
If a REINDEX TABLE CONCURRENTLY fails on a table having a TOAST table, an
invalid index will be present for the TOAST table. As we only allow to drop
invalid indexes on TOAST tables, reindexing those would lead to useless
duplicated indexes that can't be dropped anymore.
Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
---
.../expected/reindex-concurrently.out | 54 ++++++++++++++++++-
.../isolation/specs/reindex-concurrently.spec | 19 +++++++
2 files changed, 72 insertions(+), 1 deletion(-)
diff --git a/src/test/isolation/expected/reindex-concurrently.out b/src/test/isolation/expected/reindex-concurrently.out
index 9e04169b2f..674a8890bd 100644
--- a/src/test/isolation/expected/reindex-concurrently.out
+++ b/src/test/isolation/expected/reindex-concurrently.out
@@ -1,4 +1,4 @@
-Parsed test spec with 3 sessions
+Parsed test spec with 5 sessions
starting permutation: reindex sel1 upd2 ins2 del2 end1 end2
step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
@@ -76,3 +76,55 @@ step end1: COMMIT;
step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
step end2: COMMIT;
step reindex: <... completed>
+
+starting permutation: check_invalid lock reindex_fail unlock check_invalid nowarn normal_reindex check_invalid reindex check_invalid
+step check_invalid: SELECT i.indisvalid
+ FROM pg_class c
+ JOIN pg_class t ON t.oid = c.reltoastrelid
+ JOIN pg_index i ON i.indrelid = t.oid
+ WHERE c.relname = 'reind_con_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+t
+step lock: BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE;
+data
+
+aa
+isolationtester: canceling step reindex_fail on wait event Lock/virtualxid
+step reindex_fail: REINDEX TABLE CONCURRENTLY reind_con_tab;
+ERROR: canceling statement due to user request
+step unlock: COMMIT;
+step check_invalid: SELECT i.indisvalid
+ FROM pg_class c
+ JOIN pg_class t ON t.oid = c.reltoastrelid
+ JOIN pg_index i ON i.indrelid = t.oid
+ WHERE c.relname = 'reind_con_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
+step nowarn: SET client_min_messages = 'ERROR';
+step normal_reindex: REINDEX TABLE reind_con_tab;
+step check_invalid: SELECT i.indisvalid
+ FROM pg_class c
+ JOIN pg_class t ON t.oid = c.reltoastrelid
+ JOIN pg_index i ON i.indrelid = t.oid
+ WHERE c.relname = 'reind_con_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
+step check_invalid: SELECT i.indisvalid
+ FROM pg_class c
+ JOIN pg_class t ON t.oid = c.reltoastrelid
+ JOIN pg_index i ON i.indrelid = t.oid
+ WHERE c.relname = 'reind_con_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
diff --git a/src/test/isolation/specs/reindex-concurrently.spec b/src/test/isolation/specs/reindex-concurrently.spec
index eb59fe0cba..a3cd8b72a5 100644
--- a/src/test/isolation/specs/reindex-concurrently.spec
+++ b/src/test/isolation/specs/reindex-concurrently.spec
@@ -31,6 +31,22 @@ step "end2" { COMMIT; }
session "s3"
step "reindex" { REINDEX TABLE CONCURRENTLY reind_con_tab; }
+step "reindex_fail" { REINDEX TABLE CONCURRENTLY reind_con_tab; } cancel on "Lock" "virtualxid"
+step "nowarn" { SET client_min_messages = 'ERROR'; }
+
+session "s4"
+step "lock" { BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE; }
+step "unlock" { COMMIT; }
+
+session "s5"
+setup { SET client_min_messages = 'ERROR'; }
+step "normal_reindex" { REINDEX TABLE reind_con_tab; }
+step "check_invalid" {SELECT i.indisvalid
+ FROM pg_class c
+ JOIN pg_class t ON t.oid = c.reltoastrelid
+ JOIN pg_index i ON i.indrelid = t.oid
+ WHERE c.relname = 'reind_con_tab'
+ ORDER BY i.indisvalid::text COLLATE "C"; }
permutation "reindex" "sel1" "upd2" "ins2" "del2" "end1" "end2"
permutation "sel1" "reindex" "upd2" "ins2" "del2" "end1" "end2"
@@ -38,3 +54,6 @@ permutation "sel1" "upd2" "reindex" "ins2" "del2" "end1" "end2"
permutation "sel1" "upd2" "ins2" "reindex" "del2" "end1" "end2"
permutation "sel1" "upd2" "ins2" "del2" "reindex" "end1" "end2"
permutation "sel1" "upd2" "ins2" "del2" "end1" "reindex" "end2"
+permutation "check_invalid" "lock" "reindex_fail" "unlock" "check_invalid"
+ "nowarn" "normal_reindex" "check_invalid"
+ "reindex" "check_invalid"
--
2.25.1
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-11 04:10 Michael Paquier <michael@paquier.xyz>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 26+ messages in thread
From: Michael Paquier @ 2020-03-11 04:10 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Andres Freund <andres@anarazel.de>; pgsql-hackers
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200311041002.GC3099@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-11 20:33 Tom Lane <tgl@sss.pgh.pa.us>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 26+ messages in thread
From: Tom Lane @ 2020-03-11 20:33 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Andres Freund <andres@anarazel.de>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-11 20:52 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 26+ messages in thread
From: Alvaro Herrera @ 2020-03-11 20:52 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; Julien Rouhaud <rjuju123@gmail.com>; Andres Freund <andres@anarazel.de>; pgsql-hackers
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 "<name>" { <SQL> } [ blocked if "<wait_event_type>" "<wait_event>" ]
>
> and then those items would need to be passed as parameters of the prepared
> query.
I think for test readability's sake, it'd be better to put the BLOCKED
IF clause ahead of the SQL, so you can write it in the same line and let
the SQL flow to the next one:
STEP "long_select" BLOCKED IF "lwlock" "ClogControlLock"
{ select foo from pg_class where ... some more long clauses ... }
otherwise I think a step would require more lines to write.
> 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.
+1. Those long timeouts are annoying enough that infrastructure to make
a run shorter in normal circumstances might be sufficient justification
for this patch ...
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-12 07:49 Michael Paquier <michael@paquier.xyz>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 1 reply; 26+ messages in thread
From: Michael Paquier @ 2020-03-12 07:49 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Julien Rouhaud <rjuju123@gmail.com>; Andres Freund <andres@anarazel.de>; pgsql-hackers
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 "<name>" { <SQL> } [ blocked if "<wait_event_type>" "<wait_event>" ]
>>
>> and then those items would need to be passed as parameters of the prepared
>> query.
>
> I think for test readability's sake, it'd be better to put the BLOCKED
> IF clause ahead of the SQL, so you can write it in the same line and let
> the SQL flow to the next one:
>
> STEP "long_select" BLOCKED IF "lwlock" "ClogControlLock"
> { select foo from pg_class where ... some more long clauses ... }
>
> otherwise I think a step would require more lines to write.
I prefer this version.
>> 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.
>
> +1. Those long timeouts are annoying enough that infrastructure to make
> a run shorter in normal circumstances might be sufficient justification
> for this patch ...
+1. A patch does not seem to be that complicated. Now isn't it too
late for v13?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200312074941.GB1739@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-12 13:48 Tom Lane <tgl@sss.pgh.pa.us>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 26+ messages in thread
From: Tom Lane @ 2020-03-12 13:48 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Julien Rouhaud <rjuju123@gmail.com>; Andres Freund <andres@anarazel.de>; pgsql-hackers
Michael Paquier <michael@paquier.xyz> 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 let's see it first.
regards, tom lane
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-13 09:04 Julien Rouhaud <rjuju123@gmail.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 1 reply; 26+ messages in thread
From: Julien Rouhaud @ 2020-03-13 09:04 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Michael Paquier <michael@paquier.xyz>; Andres Freund <andres@anarazel.de>; pgsql-hackers
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 "<name>" { <SQL> } [ blocked if "<wait_event_type>" "<wait_event>" ]
> >
> > and then those items would need to be passed as parameters of the prepared
> > query.
>
> I think for test readability's sake, it'd be better to put the BLOCKED
> IF clause ahead of the SQL, so you can write it in the same line and let
> the SQL flow to the next one:
>
> STEP "long_select" BLOCKED IF "lwlock" "ClogControlLock"
> { select foo from pg_class where ... some more long clauses ... }
>
> otherwise I think a step would require more lines to write.
>
> > 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.
>
> +1. Those long timeouts are annoying enough that infrastructure to make
> 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 server 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 reducing
the server side timeouts?
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 blocked on
a heavyweight lock, but being able to reliably cancel a specific query as early
as possible, which AFAICS isn't possible with current isolation tester:
- either we reliably cancel the query using a statement timeout, but we'll make
it slow for everyone
- either we send a blind pg_cancel_backend() hoping that we don't catch
anything else (and also make it slower than required to make sure that it's
not canceled to early)
So we would actually only need something like this to make it work:
step "<name>" [ CANCEL IF BLOCKED ] { <SQL }
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-13 14:12 Tom Lane <tgl@sss.pgh.pa.us>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 26+ messages in thread
From: Tom Lane @ 2020-03-13 14:12 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Michael Paquier <michael@paquier.xyz>; Andres Freund <andres@anarazel.de>; pgsql-hackers
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 make
>> 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 server 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 reducing
> 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 blocked on
> a heavyweight lock, but being able to reliably cancel a specific query as 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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-13 16:25 Julien Rouhaud <rjuju123@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 26+ messages in thread
From: Julien Rouhaud @ 2020-03-13 16:25 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Michael Paquier <michael@paquier.xyz>; Andres Freund <andres@anarazel.de>; pgsql-hackers
On Fri, Mar 13, 2020 at 10:12:20AM -0400, Tom Lane wrote:
> Julien Rouhaud <rjuju123@gmail.com> 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, statement and lock timeout). I'm not sure how
> > adding a client-side facility to detect locks earlier is going to help reducing
> > 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.
Ah I see. I'll try to see if that could help the deadlock tests, but for sure
such feature would allow us to get rid of the two pg_sleep(5) in
tuplelock-update.
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 "<name>" [ WAIT UNTIL BLOCKED ] { <SQL> }
without any change to the blocking detection function?
> > 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 blocked on
> > a heavyweight lock, but being able to reliably cancel a specific query as 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.)
I agree. A new step option to kill a session rather than executing sql would
go perfectly with the above new active-wait-for-blocking-state feature.
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Add an optional timeout clause to isolationtester step.
@ 2020-03-13 16:58 Tom Lane <tgl@sss.pgh.pa.us>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 0 replies; 26+ messages in thread
From: Tom Lane @ 2020-03-13 16:58 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Michael Paquier <michael@paquier.xyz>; Andres Freund <andres@anarazel.de>; pgsql-hackers
Julien Rouhaud <rjuju123@gmail.com> 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 "<name>" [ WAIT UNTIL BLOCKED ] { <SQL> }
> without any change to the blocking detection function?
Um, isn't that the existing built-in behavior?
I could actually imagine some uses for the reverse option, *don't* wait
for it to become blocked but just immediately continue with issuing
the next step.
regards, tom lane
^ permalink raw reply [nested|flat] 26+ messages in thread
end of thread, other threads:[~2020-03-13 16:58 UTC | newest]
Thread overview: 26+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-06 13:15 Add an optional timeout clause to isolationtester step. Julien Rouhaud <rjuju123@gmail.com>
2020-03-07 01:41 ` Michael Paquier <michael@paquier.xyz>
2020-03-07 06:16 ` Julien Rouhaud <rjuju123@gmail.com>
2020-03-07 15:46 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-07 20:53 ` Julien Rouhaud <rjuju123@gmail.com>
2020-03-07 21:09 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-07 21:17 ` Julien Rouhaud <rjuju123@gmail.com>
2020-03-07 21:23 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-08 03:44 ` Michael Paquier <michael@paquier.xyz>
2020-03-09 22:15 ` Andres Freund <andres@anarazel.de>
2020-03-10 02:14 ` Michael Paquier <michael@paquier.xyz>
2020-03-10 02:32 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-10 02:55 ` Michael Paquier <michael@paquier.xyz>
2020-03-10 04:09 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-10 13:53 ` Julien Rouhaud <rjuju123@gmail.com>
2020-03-11 04:10 ` Michael Paquier <michael@paquier.xyz>
2020-03-11 20:33 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-11 20:52 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-03-12 07:49 ` Michael Paquier <michael@paquier.xyz>
2020-03-12 13:48 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-13 09:04 ` Julien Rouhaud <rjuju123@gmail.com>
2020-03-13 14:12 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-13 16:25 ` Julien Rouhaud <rjuju123@gmail.com>
2020-03-13 16:58 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-09 07:47 ` Michael Paquier <michael@paquier.xyz>
2020-03-09 08:39 ` Julien Rouhaud <rjuju123@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox