Re: [HACKERS] separate serial_schedule useful?

[Ashutosh Bapat]

On Sat, Oct 7, 2017 at 10:19 PM, Tom Lane  wrote:
> I wrote:
>> Robert Haas  writes:

Sorry, my bad. I wasn't aware of this rule. I should have looked at
the beginning of the file for any rules.

>>> There's no reason why pg_regress couldn't have a
>>> --bail-if-group-size-exceeds=N argument, or why we couldn't have a
>>> separate Perl script to validate the schedule file as part of the
>>> build process.
>
>> I'd go for the former approach; seems like less new code and fewer cycles
>> used to enforce the rule.
>
> Concretely, how about the attached?  (Obviously we'd have to fix
> parallel_schedule before committing this.)
>

Thanks, this will help. May be we should set default to 20 instead of unlimited.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] separate serial_schedule useful?

[Tom Lane]

I wrote:
> Robert Haas  writes:
>> There's no reason why pg_regress couldn't have a
>> --bail-if-group-size-exceeds=N argument, or why we couldn't have a
>> separate Perl script to validate the schedule file as part of the
>> build process.

> I'd go for the former approach; seems like less new code and fewer cycles
> used to enforce the rule.

Concretely, how about the attached?  (Obviously we'd have to fix
parallel_schedule before committing this.)

regards, tom lane

diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index b923ea1..56cd202 100644
*** a/src/test/regress/GNUmakefile
--- b/src/test/regress/GNUmakefile
*** tablespace-setup:
*** 124,130 
  ## Run tests
  ##
  
! REGRESS_OPTS = --dlpath=. $(EXTRA_REGRESS_OPTS)
  
  check: all tablespace-setup
  	$(pg_regress_check) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
--- 124,130 
  ## Run tests
  ##
  
! REGRESS_OPTS = --dlpath=. --max-concurrent-tests=20 $(EXTRA_REGRESS_OPTS)
  
  check: all tablespace-setup
  	$(pg_regress_check) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index abb742b..ff979b8 100644
*** a/src/test/regress/pg_regress.c
--- b/src/test/regress/pg_regress.c
*** char	   *launcher = NULL;
*** 78,83 
--- 78,84 
  static _stringlist *loadlanguage = NULL;
  static _stringlist *loadextension = NULL;
  static int	max_connections = 0;
+ static int	max_concurrent_tests = 0;
  static char *encoding = NULL;
  static _stringlist *schedulelist = NULL;
  static _stringlist *extra_tests = NULL;
*** run_schedule(const char *schedule, test_
*** 1691,1696 
--- 1692,1704 
  			wait_for_tests(pids, statuses, NULL, 1);
  			/* status line is finished below */
  		}
+ 		else if (max_concurrent_tests > 0 && max_concurrent_tests < num_tests)
+ 		{
+ 			/* can't print scbuf here, it's already been trashed */
+ 			fprintf(stderr, _("too many tests (more than %d) in schedule file \"%s\" line %d\n"),
+ 	max_concurrent_tests, schedule, line_num);
+ 			exit(2);
+ 		}
  		else if (max_connections > 0 && max_connections < num_tests)
  		{
  			int			oldest = 0;
*** help(void)
*** 1999,2004 
--- 2007,2014 
  	printf(_("tests; can appear multiple times\n"));
  	printf(_("  --max-connections=N   maximum number of concurrent connections\n"));
  	printf(_("(default is 0, meaning unlimited)\n"));
+ 	printf(_("  --max-concurrent-tests=N  maximum number of concurrent tests in schedule\n"));
+ 	printf(_("(default is 0, meaning unlimited)\n"));
  	printf(_("  --outputdir=DIR   place output files in DIR (default \".\")\n"));
  	printf(_("  --schedule=FILE   use test ordering schedule from FILE\n"));
  	printf(_("(can be used multiple times to concatenate)\n"));
*** regression_main(int argc, char *argv[], 
*** 2048,2053 
--- 2058,2064 
  		{"launcher", required_argument, NULL, 21},
  		{"load-extension", required_argument, NULL, 22},
  		{"config-auth", required_argument, NULL, 24},
+ 		{"max-concurrent-tests", required_argument, NULL, 25},
  		{NULL, 0, NULL, 0}
  	};
  
*** regression_main(int argc, char *argv[], 
*** 2161,2166 
--- 2172,2180 
  			case 24:
  config_auth_datadir = pg_strdup(optarg);
  break;
+ 			case 25:
+ max_concurrent_tests = atoi(optarg);
+ break;
  			default:
  /* getopt_long already emitted a complaint */
  fprintf(stderr, _("\nTry \"%s -h\" for more information.\n"),

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] separate serial_schedule useful?

[Tom Lane]

Robert Haas  writes:
> On Fri, Oct 6, 2017 at 4:16 PM, Tom Lane  wrote:
>> The other routine mistake, which I see Robert just made again,
>> is to break the at-most-twenty-parallel-tests-at-once convention.
>> I wonder if we can get in some sort of automated check for that.

> There's no reason why pg_regress couldn't have a
> --bail-if-group-size-exceeds=N argument, or why we couldn't have a
> separate Perl script to validate the schedule file as part of the
> build process.

I'd go for the former approach; seems like less new code and fewer cycles
used to enforce the rule.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] separate serial_schedule useful?

[Robert Haas]

On Fri, Oct 6, 2017 at 4:16 PM, Tom Lane  wrote:
> The other routine mistake, which I see Robert just made again,
> is to break the at-most-twenty-parallel-tests-at-once convention.
> I wonder if we can get in some sort of automated check for that.

Argh.  We can argue about whether that's my mistake or Ashutosh's
mistake, but I do try to catch these things.  It's just that there are
so many rules that require a committer to (a) know the rule and (b)
remember to enforce the rule that it's really easy to miss one.  And I
do know that rule, but it slipped my mind in the course of trying to
make sure that we'd covered all the bases in terms of the feature
itself.

There's no reason why pg_regress couldn't have a
--bail-if-group-size-exceeds=N argument, or why we couldn't have a
separate Perl script to validate the schedule file as part of the
build process.

I feel like the need to manually enforce so many tedious coding rules
is a real limiting factor on our ability to (a) involve new people in
the project and (b) get their work committed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] separate serial_schedule useful?

[Tom Lane]

Peter Eisentraut  writes:
> I noticed that the test "hash_func" was listed in parallel_schedule but
> not in serial_schedule.  I have seen that a few times recently where a
> patch proposes to add a new test file but forgets to add it to the
> serial_schedule.

Yeah, this is way too routine :-(

> I wonder whether it's still useful to keep two separate test lists.  I
> think we could just replace make installcheck with what make
> installcheck-parallel MAX_CONNECTIONS=1 does.  Thoughts?

Hm, that seems like potentially a good idea.  I can't see an argument
against it offhand.

The other routine mistake, which I see Robert just made again,
is to break the at-most-twenty-parallel-tests-at-once convention.
I wonder if we can get in some sort of automated check for that.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] separate serial_schedule useful?

[Peter Eisentraut]

I noticed that the test "hash_func" was listed in parallel_schedule but
not in serial_schedule.  I have seen that a few times recently where a
patch proposes to add a new test file but forgets to add it to the
serial_schedule.

I wonder whether it's still useful to keep two separate test lists.  I
think we could just replace make installcheck with what make
installcheck-parallel MAX_CONNECTIONS=1 does.  Thoughts?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers