public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v5 2/3] Allow CREATE INDEX CONCURRENTLY on partitioned table 4+ messages / 3 participants [nested] [flat]
* [PATCH v5 2/3] Allow CREATE INDEX CONCURRENTLY on partitioned table @ 2020-06-06 22:42 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Justin Pryzby @ 2020-06-06 22:42 UTC (permalink / raw) Note, this effectively reverts 050098b14, so take care to not reintroduce the bug it fixed. --- doc/src/sgml/ref/create_index.sgml | 9 --- src/backend/commands/indexcmds.c | 104 +++++++++++++++++++------ src/test/regress/expected/indexing.out | 57 +++++++++++++- src/test/regress/sql/indexing.sql | 16 +++- 4 files changed, 148 insertions(+), 38 deletions(-) diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml index 33aa64e81d..c780dc9547 100644 --- a/doc/src/sgml/ref/create_index.sgml +++ b/doc/src/sgml/ref/create_index.sgml @@ -657,15 +657,6 @@ Indexes: cannot. </para> - <para> - Concurrent builds for indexes on partitioned tables are currently not - supported. However, you may concurrently build the index on each - partition individually and then finally create the partitioned index - non-concurrently in order to reduce the time where writes to the - partitioned table will be locked out. In this case, building the - partitioned index is a metadata only operation. - </para> - </refsect2> </refsect1> diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 68d75916ae..cdf31a47d8 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -655,17 +655,6 @@ DefineIndex(Oid relationId, partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE; if (partitioned) { - /* - * Note: we check 'stmt->concurrent' rather than 'concurrent', so that - * the error is thrown also for temporary tables. Seems better to be - * consistent, even though we could do it on temporary table because - * we're not actually doing it concurrently. - */ - if (stmt->concurrent) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot create index on partitioned table \"%s\" concurrently", - RelationGetRelationName(rel)))); if (stmt->excludeOpNames) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), @@ -1100,6 +1089,9 @@ DefineIndex(Oid relationId, if (pd->nparts != 0) flags |= INDEX_CREATE_INVALID; } + else if (concurrent && OidIsValid(parentIndexId)) + /* Initial concurrent build of index partition is invalid */ + flags |= INDEX_CREATE_INVALID; if (stmt->deferrable) constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE; @@ -1152,8 +1144,17 @@ DefineIndex(Oid relationId, */ if (!stmt->relation || stmt->relation->inh) { + /* + * Need to close the relation before recursing into children, so + * copy needed data into a longlived context. + */ + + MemoryContext ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX", + ALLOCSET_DEFAULT_SIZES); + MemoryContext oldcontext = MemoryContextSwitchTo(ind_context); PartitionDesc partdesc = RelationGetPartitionDesc(rel); int nparts = partdesc->nparts; + char *relname = pstrdup(RelationGetRelationName(rel)); Oid *part_oids = palloc(sizeof(Oid) * nparts); bool invalidate_parent = false; TupleDesc parentDesc; @@ -1163,8 +1164,10 @@ DefineIndex(Oid relationId, nparts); memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts); + parentDesc = CreateTupleDescCopy(RelationGetDescr(rel)); + table_close(rel, NoLock); + MemoryContextSwitchTo(oldcontext); - parentDesc = RelationGetDescr(rel); opfamOids = palloc(sizeof(Oid) * numberOfKeyAttributes); for (i = 0; i < numberOfKeyAttributes; i++) opfamOids[i] = get_opclass_family(classObjectId[i]); @@ -1199,18 +1202,20 @@ DefineIndex(Oid relationId, ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot create unique index on partitioned table \"%s\"", - RelationGetRelationName(rel)), + relname), errdetail("Table \"%s\" contains partitions that are foreign tables.", - RelationGetRelationName(rel)))); + relname))); table_close(childrel, lockmode); continue; } + oldcontext = MemoryContextSwitchTo(ind_context); childidxs = RelationGetIndexList(childrel); attmap = build_attrmap_by_name(RelationGetDescr(childrel), parentDesc); + MemoryContextSwitchTo(oldcontext); foreach(cell, childidxs) { @@ -1336,10 +1341,17 @@ DefineIndex(Oid relationId, createdConstraintId, is_alter_table, check_rights, check_not_in_use, skip_build, quiet); + if (concurrent) + { + PushActiveSnapshot(GetTransactionSnapshot()); + invalidate_parent = true; + } } - pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, - i + 1); + /* For concurrent build, this is a catalog-only stage */ + if (!concurrent) + pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, + i + 1); free_attrmap(attmap); } @@ -1366,23 +1378,72 @@ DefineIndex(Oid relationId, table_close(pg_index, RowExclusiveLock); heap_freetuple(newtup); } + } else + table_close(rel, NoLock); + + if (concurrent) + { + List *childs; + ListCell *lc; + int npart = 0; + + /* Reindex invalid child indexes created earlier */ + MemoryContext ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX", + ALLOCSET_DEFAULT_SIZES); + MemoryContext oldcontext = MemoryContextSwitchTo(ind_context); + childs = find_inheritance_children(indexRelationId, NoLock); + MemoryContextSwitchTo(oldcontext); + + /* Make the catalog changes visible to get_partition_parent */ + CommandCounterIncrement(); + + foreach (lc, childs) + { + Oid indexrelid = lfirst_oid(lc); + Relation cldidx; + bool isvalid; + + if (!OidIsValid(parentIndexId)) + pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, + npart++); + + cldidx = index_open(indexrelid, ShareUpdateExclusiveLock); + isvalid = cldidx->rd_index->indisvalid; + index_close(cldidx, NoLock); + if (isvalid) + continue; + + /* This may be a partitioned index, which is fine too */ + ReindexRelationConcurrently(indexrelid, 0); + PushActiveSnapshot(GetTransactionSnapshot()); + } + + PopActiveSnapshot(); + CommitTransactionCommand(); + StartTransactionCommand(); + + /* + * CIC needs to mark a partitioned index as VALID, which itself + * requires setting READY, which is unset for CIC (even though + * it's meaningless for an index without storage). + */ + index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY); + CommandCounterIncrement(); + index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID); } /* * Indexes on partitioned tables are not themselves built, so we're * done here. */ - table_close(rel, NoLock); if (!OidIsValid(parentIndexId)) pgstat_progress_end_command(); return address; } + table_close(rel, NoLock); if (!concurrent) { - /* Close the heap and we're done, in the non-concurrent case */ - table_close(rel, NoLock); - /* If this is the top-level index, we're done. */ if (!OidIsValid(parentIndexId)) pgstat_progress_end_command(); @@ -1390,10 +1451,9 @@ DefineIndex(Oid relationId, return address; } - /* save lockrelid and locktag for below, then close rel */ + /* save lockrelid and locktag for below */ heaprelid = rel->rd_lockInfo.lockRelId; SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId); - table_close(rel, NoLock); /* * For a concurrent build, it's important to make the catalog entries diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out index f78865ef81..1ae58e110f 100644 --- a/src/test/regress/expected/indexing.out +++ b/src/test/regress/expected/indexing.out @@ -50,11 +50,60 @@ select relname, relkind, relhassubclass, inhparent::regclass (8 rows) drop table idxpart; --- Some unsupported features +-- CIC on partitioned table create table idxpart (a int, b int, c text) partition by range (a); -create table idxpart1 partition of idxpart for values from (0) to (10); -create index concurrently on idxpart (a); -ERROR: cannot create index on partitioned table "idxpart" concurrently +create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a); +create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a); +create table idxpart2 partition of idxpart for values from (10) to (20); +insert into idxpart2 values(10),(10); -- not unique +create index concurrently on idxpart (a); -- partitioned +create index concurrently on idxpart1 (a); -- partitioned and partition +create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves +create index concurrently on idxpart2 (a); -- leaf +create unique index concurrently on idxpart (a); -- partitioned, unique failure +ERROR: could not create unique index "idxpart2_a_idx2" +DETAIL: Key (a)=(10) is duplicated. +\d idxpart + Partitioned table "public.idxpart" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | + c | text | | | +Partition key: RANGE (a) +Indexes: + "idxpart_a_idx" btree (a) + "idxpart_a_idx1" UNIQUE, btree (a) INVALID +Number of partitions: 2 (Use \d+ to list them.) + +\d idxpart1 + Partitioned table "public.idxpart1" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | + c | text | | | +Partition of: idxpart FOR VALUES FROM (0) TO (10) +Partition key: RANGE (a) +Indexes: + "idxpart1_a_idx" btree (a) + "idxpart1_a_idx1" btree (a) + "idxpart1_a_idx2" UNIQUE, btree (a) +Number of partitions: 1 (Use \d+ to list them.) + +\d idxpart2 + Table "public.idxpart2" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | + c | text | | | +Partition of: idxpart FOR VALUES FROM (10) TO (20) +Indexes: + "idxpart2_a_idx" btree (a) + "idxpart2_a_idx1" btree (a) + "idxpart2_a_idx2" UNIQUE, btree (a) INVALID + drop table idxpart; -- Verify bugfix with query on indexed partitioned table with no partitions -- https://postgr.es/m/[email protected] diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql index 35d159f41b..d908ee6d17 100644 --- a/src/test/regress/sql/indexing.sql +++ b/src/test/regress/sql/indexing.sql @@ -29,10 +29,20 @@ select relname, relkind, relhassubclass, inhparent::regclass where relname like 'idxpart%' order by relname; drop table idxpart; --- Some unsupported features +-- CIC on partitioned table create table idxpart (a int, b int, c text) partition by range (a); -create table idxpart1 partition of idxpart for values from (0) to (10); -create index concurrently on idxpart (a); +create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a); +create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a); +create table idxpart2 partition of idxpart for values from (10) to (20); +insert into idxpart2 values(10),(10); -- not unique +create index concurrently on idxpart (a); -- partitioned +create index concurrently on idxpart1 (a); -- partitioned and partition +create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves +create index concurrently on idxpart2 (a); -- leaf +create unique index concurrently on idxpart (a); -- partitioned, unique failure +\d idxpart +\d idxpart1 +\d idxpart2 drop table idxpart; -- Verify bugfix with query on indexed partitioned table with no partitions -- 2.17.0 --hYooF8G/hrfVAmum Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v5-0003-Implement-CLUSTER-of-partitioned-table.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Timeout control within tests @ 2022-02-18 15:26 Tom Lane <[email protected]> 2022-02-19 02:41 ` Re: Timeout control within tests Noah Misch <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Tom Lane @ 2022-02-18 15:26 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers Noah Misch <[email protected]> writes: > On Thu, Feb 17, 2022 at 09:48:25PM -0800, Andres Freund wrote: >> Meson's test runner has the concept of a "timeout multiplier" for ways of >> running tests. Meson's stuff is about entire tests (i.e. one tap test), so >> doesn't apply here, but I wonder if we shouldn't do something similar? > Hmmm. It is good if the user can express an intent that continues to make > sense if we change the default timeout. For the buildfarm use case, a > multiplier is moderately better on that axis (PG_TEST_TIMEOUT_MULTIPLIER=100 > beats PG_TEST_TIMEOUT_DEFAULT=18000). For the hacker use case, an absolute > value is substantially better on that axis (PG_TEST_TIMEOUT_DEFAULT=3 beats > PG_TEST_TIMEOUT_MULTIPLIER=.016666). FWIW, I'm fairly sure that PGISOLATIONTIMEOUT=300 was selected after finding that smaller values didn't work reliably in the buildfarm. Now maybe 741d7f1 fixed that, but I wouldn't count on it. So while I approve of the idea to remove PGISOLATIONTIMEOUT in favor of using this centralized setting, I think that we might need to have a multiplier there, or else we'll end up with PG_TEST_TIMEOUT_DEFAULT set to 300 across the board. Perhaps the latter is fine, but a multiplier seems a bit more flexible. On the other hand, I also support your point that an absolute setting is easier to think about / adjust for special uses. So maybe we should just KISS and use a single absolute setting until we find a hard reason why that doesn't work well. regards, tom lane ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Timeout control within tests 2022-02-18 15:26 Re: Timeout control within tests Tom Lane <[email protected]> @ 2022-02-19 02:41 ` Noah Misch <[email protected]> 2022-04-18 04:23 ` Re: Timeout control within tests Noah Misch <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Noah Misch @ 2022-02-19 02:41 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers On Fri, Feb 18, 2022 at 10:26:52AM -0500, Tom Lane wrote: > Noah Misch <[email protected]> writes: > > On Thu, Feb 17, 2022 at 09:48:25PM -0800, Andres Freund wrote: > >> Meson's test runner has the concept of a "timeout multiplier" for ways of > >> running tests. Meson's stuff is about entire tests (i.e. one tap test), so > >> doesn't apply here, but I wonder if we shouldn't do something similar? > > > Hmmm. It is good if the user can express an intent that continues to make > > sense if we change the default timeout. For the buildfarm use case, a > > multiplier is moderately better on that axis (PG_TEST_TIMEOUT_MULTIPLIER=100 > > beats PG_TEST_TIMEOUT_DEFAULT=18000). For the hacker use case, an absolute > > value is substantially better on that axis (PG_TEST_TIMEOUT_DEFAULT=3 beats > > PG_TEST_TIMEOUT_MULTIPLIER=.016666). > > FWIW, I'm fairly sure that PGISOLATIONTIMEOUT=300 was selected after > finding that smaller values didn't work reliably in the buildfarm. > Now maybe 741d7f1 fixed that, but I wouldn't count on it. So while I > approve of the idea to remove PGISOLATIONTIMEOUT in favor of using this > centralized setting, I think that we might need to have a multiplier > there, or else we'll end up with PG_TEST_TIMEOUT_DEFAULT set to 300 > across the board. Perhaps the latter is fine, but a multiplier seems a > bit more flexible. The PGISOLATIONTIMEOUT replacement was 2*timeout_default, so isolation suites would get 2*180s=360s. (I don't want to lower any default timeouts, but I don't mind raising them.) In a sense, PG_TEST_TIMEOUT_DEFAULT is a multiplier with as many sites as possible multiplying it by 1. The patch has multiples at two code sites. > On the other hand, I also support your point that an absolute setting > is easier to think about / adjust for special uses. So maybe we should > just KISS and use a single absolute setting until we find a hard reason > why that doesn't work well. ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Timeout control within tests 2022-02-18 15:26 Re: Timeout control within tests Tom Lane <[email protected]> 2022-02-19 02:41 ` Re: Timeout control within tests Noah Misch <[email protected]> @ 2022-04-18 04:23 ` Noah Misch <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Noah Misch @ 2022-04-18 04:23 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers (I pushed the main patch as f2698ea, on 2022-03-04.) On Fri, Feb 18, 2022 at 06:41:36PM -0800, Noah Misch wrote: > On Fri, Feb 18, 2022 at 10:26:52AM -0500, Tom Lane wrote: > > Noah Misch <[email protected]> writes: > > > On Thu, Feb 17, 2022 at 09:48:25PM -0800, Andres Freund wrote: > > >> Meson's test runner has the concept of a "timeout multiplier" for ways of > > >> running tests. Meson's stuff is about entire tests (i.e. one tap test), so > > >> doesn't apply here, but I wonder if we shouldn't do something similar? > > > > > Hmmm. It is good if the user can express an intent that continues to make > > > sense if we change the default timeout. For the buildfarm use case, a > > > multiplier is moderately better on that axis (PG_TEST_TIMEOUT_MULTIPLIER=100 > > > beats PG_TEST_TIMEOUT_DEFAULT=18000). For the hacker use case, an absolute > > > value is substantially better on that axis (PG_TEST_TIMEOUT_DEFAULT=3 beats > > > PG_TEST_TIMEOUT_MULTIPLIER=.016666). > > > > FWIW, I'm fairly sure that PGISOLATIONTIMEOUT=300 was selected after > > finding that smaller values didn't work reliably in the buildfarm. > > Now maybe 741d7f1 fixed that, but I wouldn't count on it. So while I > > approve of the idea to remove PGISOLATIONTIMEOUT in favor of using this > > centralized setting, I think that we might need to have a multiplier > > there, or else we'll end up with PG_TEST_TIMEOUT_DEFAULT set to 300 > > across the board. Perhaps the latter is fine, but a multiplier seems a > > bit more flexible. > > The PGISOLATIONTIMEOUT replacement was 2*timeout_default, so isolation suites > would get 2*180s=360s. (I don't want to lower any default timeouts, but I > don't mind raising them.) In a sense, PG_TEST_TIMEOUT_DEFAULT is a multiplier > with as many sites as possible multiplying it by 1. The patch has multiples > at two code sites. Here's the PGISOLATIONTIMEOUT replacement patch. I waffled on whether to back-patch. Since it affects only isolation suite testing, only on systems too slow for the default timeout, it's not a major decision. I currently plan not to back-patch, since slow systems that would have wanted a back-patch can just set both variables. Author: Noah Misch <[email protected]> Commit: Noah Misch <[email protected]> Replace PGISOLATIONTIMEOUT with 2 * PG_TEST_TIMEOUT_DEFAULT. Now that the more-generic variable exists, use it. Reviewed by FIXME. Discussion: https://postgr.es/m/[email protected] diff --git a/src/test/isolation/README b/src/test/isolation/README index 8457a56..5818ca5 100644 --- a/src/test/isolation/README +++ b/src/test/isolation/README @@ -47,10 +47,10 @@ pg_isolation_regress is a tool similar to pg_regress, but instead of using psql to execute a test, it uses isolationtester. It accepts all the same command-line arguments as pg_regress. -By default, isolationtester will wait at most 300 seconds (5 minutes) +By default, isolationtester will wait at most 360 seconds (6 minutes) for any one test step to complete. If you need to adjust this, set -the environment variable PGISOLATIONTIMEOUT to the desired timeout -in seconds. +the environment variable PG_TEST_TIMEOUT_DEFAULT to half the desired +timeout in seconds. Test specification @@ -138,10 +138,11 @@ Each step may contain commands that block until further action has been taken deadlock). A test that uses this ability must manually specify valid permutations, i.e. those that would not expect a blocked session to execute a command. If a test fails to follow that rule, isolationtester will cancel it -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. +after 2 * PG_TEST_TIMEOUT_DEFAULT seconds. If the cancel doesn't work, +isolationtester will exit uncleanly after a total of 4 * +PG_TEST_TIMEOUT_DEFAULT. 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. 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 12179f2..095db8f 100644 --- a/src/test/isolation/isolationtester.c +++ b/src/test/isolation/isolationtester.c @@ -46,7 +46,7 @@ static int nconns = 0; static bool any_new_notice = false; /* Maximum time to wait before giving up on a step (in usec) */ -static int64 max_step_wait = 300 * USECS_PER_SEC; +static int64 max_step_wait = 360 * USECS_PER_SEC; static void check_testspec(TestSpec *testspec); @@ -128,12 +128,12 @@ main(int argc, char **argv) conninfo = "dbname = postgres"; /* - * If PGISOLATIONTIMEOUT is set in the environment, adopt its value (given - * in seconds) as the max time to wait for any one step to complete. + * If PG_TEST_TIMEOUT_DEFAULT is set, adopt its value (given in seconds) + * as half the max time to wait for any one step to complete. */ - env_wait = getenv("PGISOLATIONTIMEOUT"); + env_wait = getenv("PG_TEST_TIMEOUT_DEFAULT"); if (env_wait != NULL) - max_step_wait = ((int64) atoi(env_wait)) * USECS_PER_SEC; + max_step_wait = 2 * ((int64) atoi(env_wait)) * USECS_PER_SEC; /* Read the test spec from stdin */ spec_yyparse(); Attachments: [text/plain] PGISOLATIONTIMEOUT-decom-v1.patch (3.3K, ../../[email protected]/2-PGISOLATIONTIMEOUT-decom-v1.patch) download | inline diff: Author: Noah Misch <[email protected]> Commit: Noah Misch <[email protected]> Replace PGISOLATIONTIMEOUT with 2 * PG_TEST_TIMEOUT_DEFAULT. Now that the more-generic variable exists, use it. Reviewed by FIXME. Discussion: https://postgr.es/m/[email protected] diff --git a/src/test/isolation/README b/src/test/isolation/README index 8457a56..5818ca5 100644 --- a/src/test/isolation/README +++ b/src/test/isolation/README @@ -47,10 +47,10 @@ pg_isolation_regress is a tool similar to pg_regress, but instead of using psql to execute a test, it uses isolationtester. It accepts all the same command-line arguments as pg_regress. -By default, isolationtester will wait at most 300 seconds (5 minutes) +By default, isolationtester will wait at most 360 seconds (6 minutes) for any one test step to complete. If you need to adjust this, set -the environment variable PGISOLATIONTIMEOUT to the desired timeout -in seconds. +the environment variable PG_TEST_TIMEOUT_DEFAULT to half the desired +timeout in seconds. Test specification @@ -138,10 +138,11 @@ Each step may contain commands that block until further action has been taken deadlock). A test that uses this ability must manually specify valid permutations, i.e. those that would not expect a blocked session to execute a command. If a test fails to follow that rule, isolationtester will cancel it -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. +after 2 * PG_TEST_TIMEOUT_DEFAULT seconds. If the cancel doesn't work, +isolationtester will exit uncleanly after a total of 4 * +PG_TEST_TIMEOUT_DEFAULT. 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. 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 12179f2..095db8f 100644 --- a/src/test/isolation/isolationtester.c +++ b/src/test/isolation/isolationtester.c @@ -46,7 +46,7 @@ static int nconns = 0; static bool any_new_notice = false; /* Maximum time to wait before giving up on a step (in usec) */ -static int64 max_step_wait = 300 * USECS_PER_SEC; +static int64 max_step_wait = 360 * USECS_PER_SEC; static void check_testspec(TestSpec *testspec); @@ -128,12 +128,12 @@ main(int argc, char **argv) conninfo = "dbname = postgres"; /* - * If PGISOLATIONTIMEOUT is set in the environment, adopt its value (given - * in seconds) as the max time to wait for any one step to complete. + * If PG_TEST_TIMEOUT_DEFAULT is set, adopt its value (given in seconds) + * as half the max time to wait for any one step to complete. */ - env_wait = getenv("PGISOLATIONTIMEOUT"); + env_wait = getenv("PG_TEST_TIMEOUT_DEFAULT"); if (env_wait != NULL) - max_step_wait = ((int64) atoi(env_wait)) * USECS_PER_SEC; + max_step_wait = 2 * ((int64) atoi(env_wait)) * USECS_PER_SEC; /* Read the test spec from stdin */ spec_yyparse(); ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2022-04-18 04:23 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-06-06 22:42 [PATCH v5 2/3] Allow CREATE INDEX CONCURRENTLY on partitioned table Justin Pryzby <[email protected]> 2022-02-18 15:26 Re: Timeout control within tests Tom Lane <[email protected]> 2022-02-19 02:41 ` Re: Timeout control within tests Noah Misch <[email protected]> 2022-04-18 04:23 ` Re: Timeout control within tests Noah Misch <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox