Re: Testing LISTEN/NOTIFY more effectively

Hi,

On 2019-07-27 18:20:52 -0400, Tom Lane wrote:
> diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
> index 6ab19b1..98e5bf2 100644
> --- a/src/test/isolation/isolationtester.c
> +++ b/src/test/isolation/isolationtester.c
> @@ -23,10 +23,12 @@
>  
>  /*
>   * conns[0] is the global setup, teardown, and watchdog connection.  Additional
> - * connections represent spec-defined sessions.
> + * connections represent spec-defined sessions.  We also track the backend
> + * PID, in numeric and string formats, for each connection.
>   */
>  static PGconn **conns = NULL;
> -static const char **backend_pids = NULL;
> +static int *backend_pids = NULL;
> +static const char **backend_pid_strs = NULL;
>  static int	nconns = 0;

Hm, a bit sad to have both of those around. Not worth getting bothered
about memory wise, but it does irk me somewhat.


> @@ -187,26 +191,9 @@ main(int argc, char **argv)
>  								 blackholeNoticeProcessor,
>  								 NULL);
>  
> -		/* Get the backend pid for lock wait checking. */
> -		res = PQexec(conns[i], "SELECT pg_catalog.pg_backend_pid()");
> -		if (PQresultStatus(res) == PGRES_TUPLES_OK)
> -		{
> -			if (PQntuples(res) == 1 && PQnfields(res) == 1)
> -				backend_pids[i] = pg_strdup(PQgetvalue(res, 0, 0));
> -			else
> -			{
> -				fprintf(stderr, "backend pid query returned %d rows and %d columns, expected 1 row and 1 column",
> -						PQntuples(res), PQnfields(res));
> -				exit(1);
> -			}
> -		}
> -		else
> -		{
> -			fprintf(stderr, "backend pid query failed: %s",
> -					PQerrorMessage(conns[i]));
> -			exit(1);
> -		}
> -		PQclear(res);
> +		/* Save each connection's backend PID for subsequent use. */
> +		backend_pids[i] = PQbackendPID(conns[i]);
> +		backend_pid_strs[i] = psprintf("%d", backend_pids[i]);

Heh.


> @@ -738,7 +728,7 @@ try_complete_step(Step *step, int flags)
>  				bool		waiting;
>  
>  				res = PQexecPrepared(conns[0], PREP_WAITING, 1,
> -									 &backend_pids[step->session + 1],
> +									 &backend_pid_strs[step->session + 1],
>  									 NULL, NULL, 0);
>  				if (PQresultStatus(res) != PGRES_TUPLES_OK ||
>  					PQntuples(res) != 1)

We could of course just send the pids in binary ;). No, not worth it
just to avoid a small redundant array ;)


> +	/* Report any available NOTIFY messages, too */
> +	PQconsumeInput(conn);
> +	while ((notify = PQnotifies(conn)) != NULL)
> +	{

Hm. I wonder if all that's happening with prairedog is that the notice
is sent a bit later. I think that could e.g. conceivably happen because
it TCP_NODELAY isn't supported on prariedog? Or just because the machine
is very slow?

The diff you showed with the reordering afaict only reordered the NOTIFY
around statements that are marked as <waiting ...>. As the waiting
detection is done over a separate connection, there's afaict no
guarantee that we see all notices/notifies that occurred before the
query started blocking.  It's possible we could make this practically
robust enough by checking for notice/notifies on the blocked connection
just before printing out the <waiting ...>?  That still leaves the
potential issue that the different backend connection deliver data out
of order, but that seems not very likely?


Greetings,

Andres Freund