Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 66d6086

Browse files
Speed up pg_regress server readiness testing.
Instead of connecting to the server with psql to check if it is ready for running tests, this changes pg_regress to use PQPing which avoids performing system() calls which are expensive on some platforms, like Windows. The frequency of tests is also increased in order to connect to the server faster. This patch is part of a larger effort to make testing consume fewer resources in order to be able to fit more tests into the available CI system constraints. Reviewed-by: Andres Freund <andres@anarazel.de> Discussion: https://postgr.es/m/20230823192239.jxew5s3sjru63lio@awork3.anarazel.de
1 parent 387f9ed commit 66d6086

File tree

7 files changed

+59
-32
lines changed

7 files changed

+59
-32
lines changed

src/interfaces/ecpg/test/Makefile

+2-1
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ include $(top_builddir)/src/Makefile.global
1010
override CPPFLAGS := \
1111
'-I$(top_builddir)/src/port' \
1212
'-I$(top_srcdir)/src/test/regress' \
13+
'-I$(libpq_srcdir)' \
1314
'-DHOST_TUPLE="$(host_tuple)"' \
1415
'-DSHELLPROG="$(SHELL)"' \
1516
$(CPPFLAGS)
@@ -45,7 +46,7 @@ clean distclean maintainer-clean:
4546
all: pg_regress$(X)
4647

4748
pg_regress$(X): pg_regress_ecpg.o $(WIN32RES) $(top_builddir)/src/test/regress/pg_regress.o
48-
$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
49+
$(CC) $(CFLAGS) $^ $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
4950

5051
$(top_builddir)/src/test/regress/pg_regress.o:
5152
$(MAKE) -C $(dir $@) $(notdir $@)

src/interfaces/ecpg/test/meson.build

+1-1
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ pg_regress_ecpg = executable('pg_regress_ecpg',
1818
pg_regress_ecpg_sources,
1919
c_args: pg_regress_cflags,
2020
include_directories: [pg_regress_inc, include_directories('.')],
21-
dependencies: [frontend_code],
21+
dependencies: [frontend_code, libpq],
2222
kwargs: default_bin_args + {
2323
'install': false
2424
},

src/test/isolation/Makefile

+1-1
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ pg_regress.o: | submake-regress
3838
rm -f $@ && $(LN_S) $(top_builddir)/src/test/regress/pg_regress.o .
3939

4040
pg_isolation_regress$(X): isolation_main.o pg_regress.o $(WIN32RES)
41-
$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
41+
$(CC) $(CFLAGS) $^ $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
4242

4343
isolationtester$(X): $(OBJS) | submake-libpq submake-libpgport
4444
$(CC) $(CFLAGS) $^ $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@

src/test/isolation/meson.build

+1-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ pg_isolation_regress = executable('pg_isolation_regress',
3535
isolation_sources,
3636
c_args: pg_regress_cflags,
3737
include_directories: pg_regress_inc,
38-
dependencies: frontend_code,
38+
dependencies: [frontend_code, libpq],
3939
kwargs: default_bin_args + {
4040
'install_dir': dir_pgxs / 'src/test/isolation',
4141
},

src/test/regress/GNUmakefile

+2-2
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,11 @@ EXTRADEFS = '-DHOST_TUPLE="$(host_tuple)"' \
3636
all: pg_regress$(X)
3737

3838
pg_regress$(X): pg_regress.o pg_regress_main.o $(WIN32RES) | submake-libpgport
39-
$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
39+
$(CC) $(CFLAGS) $^ $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
4040

4141
# dependencies ensure that path changes propagate
4242
pg_regress.o: pg_regress.c $(top_builddir)/src/port/pg_config_paths.h
43-
pg_regress.o: override CPPFLAGS += -I$(top_builddir)/src/port $(EXTRADEFS)
43+
pg_regress.o: override CPPFLAGS += -I$(top_builddir)/src/port -I$(libpq_srcdir) $(EXTRADEFS)
4444

4545
# note: because of the submake dependency, this rule's action is really a no-op
4646
$(top_builddir)/src/port/pg_config_paths.h: | submake-libpgport

src/test/regress/meson.build

+1-1
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ endif
3030
pg_regress = executable('pg_regress',
3131
regress_sources,
3232
c_args: pg_regress_cflags,
33-
dependencies: [frontend_code],
33+
dependencies: [frontend_code, libpq],
3434
kwargs: default_bin_args + {
3535
'install_dir': dir_pgxs / 'src/test/regress',
3636
},

src/test/regress/pg_regress.c

+51-25
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include "common/username.h"
3333
#include "getopt_long.h"
3434
#include "lib/stringinfo.h"
35+
#include "libpq-fe.h"
3536
#include "libpq/pqcomm.h" /* needed for UNIXSOCK_PATH() */
3637
#include "pg_config_paths.h"
3738
#include "pg_regress.h"
@@ -75,6 +76,12 @@ const char *pretty_diff_opts = "-w -U3";
7576
*/
7677
#define TESTNAME_WIDTH 36
7778

79+
/*
80+
* The number times per second that pg_regress checks to see if the test
81+
* instance server has started and is available for connection.
82+
*/
83+
#define WAIT_TICKS_PER_SECOND 20
84+
7885
typedef enum TAPtype
7986
{
8087
DIAG = 0,
@@ -107,6 +114,7 @@ static bool nolocale = false;
107114
static bool use_existing = false;
108115
static char *hostname = NULL;
109116
static int port = -1;
117+
static char portstr[16];
110118
static bool port_specified_by_user = false;
111119
static char *dlpath = PKGLIBDIR;
112120
static char *user = NULL;
@@ -2107,7 +2115,6 @@ regression_main(int argc, char *argv[],
21072115
int i;
21082116
int option_index;
21092117
char buf[MAXPGPATH * 4];
2110-
char buf2[MAXPGPATH * 4];
21112118

21122119
pg_logging_init(argv[0]);
21132120
progname = get_progname(argv[0]);
@@ -2296,6 +2303,9 @@ regression_main(int argc, char *argv[],
22962303
const char *env_wait;
22972304
int wait_seconds;
22982305
const char *initdb_template_dir;
2306+
const char *keywords[4];
2307+
const char *values[4];
2308+
PGPing rv;
22992309

23002310
/*
23012311
* Prepare the temp instance
@@ -2436,21 +2446,28 @@ regression_main(int argc, char *argv[],
24362446
#endif
24372447

24382448
/*
2439-
* Check if there is a postmaster running already.
2449+
* Prepare the connection params for checking the state of the server
2450+
* before starting the tests.
24402451
*/
2441-
snprintf(buf2, sizeof(buf2),
2442-
"\"%s%spsql\" -X postgres <%s 2>%s",
2443-
bindir ? bindir : "",
2444-
bindir ? "/" : "",
2445-
DEVNULL, DEVNULL);
2452+
sprintf(portstr, "%d", port);
2453+
keywords[0] = "dbname";
2454+
values[0] = "postgres";
2455+
keywords[1] = "port";
2456+
values[1] = portstr;
2457+
keywords[2] = "host";
2458+
values[2] = hostname ? hostname : sockdir;
2459+
keywords[3] = NULL;
2460+
values[3] = NULL;
24462461

2462+
/*
2463+
* Check if there is a postmaster running already.
2464+
*/
24472465
for (i = 0; i < 16; i++)
24482466
{
2449-
fflush(NULL);
2450-
if (system(buf2) == 0)
2451-
{
2452-
char s[16];
2467+
rv = PQpingParams(keywords, values, 1);
24532468

2469+
if (rv == PQPING_OK)
2470+
{
24542471
if (port_specified_by_user || i == 15)
24552472
{
24562473
note("port %d apparently in use", port);
@@ -2461,8 +2478,8 @@ regression_main(int argc, char *argv[],
24612478

24622479
note("port %d apparently in use, trying %d", port, port + 1);
24632480
port++;
2464-
sprintf(s, "%d", port);
2465-
setenv("PGPORT", s, 1);
2481+
sprintf(portstr, "%d", port);
2482+
setenv("PGPORT", portstr, 1);
24662483
}
24672484
else
24682485
break;
@@ -2485,11 +2502,11 @@ regression_main(int argc, char *argv[],
24852502
bail("could not spawn postmaster: %s", strerror(errno));
24862503

24872504
/*
2488-
* Wait till postmaster is able to accept connections; normally this
2489-
* is only a second or so, but Cygwin is reportedly *much* slower, and
2490-
* test builds using Valgrind or similar tools might be too. Hence,
2491-
* allow the default timeout of 60 seconds to be overridden from the
2492-
* PGCTLTIMEOUT environment variable.
2505+
* Wait till postmaster is able to accept connections; normally takes
2506+
* only a fraction of a second or so, but Cygwin is reportedly *much*
2507+
* slower, and test builds using Valgrind or similar tools might be
2508+
* too. Hence, allow the default timeout of 60 seconds to be
2509+
* overridden from the PGCTLTIMEOUT environment variable.
24932510
*/
24942511
env_wait = getenv("PGCTLTIMEOUT");
24952512
if (env_wait != NULL)
@@ -2501,13 +2518,24 @@ regression_main(int argc, char *argv[],
25012518
else
25022519
wait_seconds = 60;
25032520

2504-
for (i = 0; i < wait_seconds; i++)
2521+
for (i = 0; i < wait_seconds * WAIT_TICKS_PER_SECOND; i++)
25052522
{
2506-
/* Done if psql succeeds */
2507-
fflush(NULL);
2508-
if (system(buf2) == 0)
2523+
/*
2524+
* It's fairly unlikely that the server is responding immediately
2525+
* so we start with sleeping before checking instead of the other
2526+
* way around.
2527+
*/
2528+
pg_usleep(1000000L / WAIT_TICKS_PER_SECOND);
2529+
2530+
rv = PQpingParams(keywords, values, 1);
2531+
2532+
/* Done if the server is running and accepts connections */
2533+
if (rv == PQPING_OK)
25092534
break;
25102535

2536+
if (rv == PQPING_NO_ATTEMPT)
2537+
bail("attempting to connect to postmaster failed");
2538+
25112539
/*
25122540
* Fail immediately if postmaster has exited
25132541
*/
@@ -2520,10 +2548,8 @@ regression_main(int argc, char *argv[],
25202548
bail("postmaster failed, examine \"%s/log/postmaster.log\" for the reason",
25212549
outputdir);
25222550
}
2523-
2524-
pg_usleep(1000000L);
25252551
}
2526-
if (i >= wait_seconds)
2552+
if (i >= wait_seconds * WAIT_TICKS_PER_SECOND)
25272553
{
25282554
diag("postmaster did not respond within %d seconds, examine \"%s/log/postmaster.log\" for the reason",
25292555
wait_seconds, outputdir);

0 commit comments

Comments
 (0)