libpq environment variables in the server

Started by Peter Eisentrautalmost 7 years ago10 messages

peter.eisentraut@2ndquadrant.com

almost 7 years ago

When libpq is loaded in the server (libpqwalreceiver, dblink,
postgres_fdw), it may use libpq environment variables set in the
postmaster environment for connection parameter defaults. I have
noticed that this has some confusing effects in our test suites. I
wonder if this is a good idea in general.

For example, the TAP test infrastructure sets PGAPPNAME to allow
identifying clients in the server log. But this environment variable is
also inherited by temporary servers started with pg_ctl and is then in
turn used by libpqwalreceiver as the application_name for connecting to
remote servers where it then shows up in pg_stat_replication and is
relevant for things like synchronous_standby_names.

Also, the pg_rewind tests appear to rely implicitly on PGHOST and PGPORT
being set in the server environment to find the other node via
primary_conninfo. That is easy to fix, but it shows that this kind of
thing can creep in unintentionally.

I was thinking that maybe we should clear all libpq environment
variables in the server, or perhaps have a mode in libpq to ignore all
environment variables. Then again, maybe setting something like
PGSSLMODE globally in the server could be useful, so just removing
everything might not be the right answer.

Maybe pg_ctl should have some functionality to clear the environment,
and maybe there could be a facility in postgresql.conf to set
environment variables.

Thoughts?

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

Dmitry Igrishin

dmitigr@gmail.com

almost 7 years ago

In reply to: Peter Eisentraut (#1)

Re: libpq environment variables in the server

пн, 21 янв. 2019 г. в 13:42, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com>:

When libpq is loaded in the server (libpqwalreceiver, dblink,
postgres_fdw), it may use libpq environment variables set in the
postmaster environment for connection parameter defaults. I have
noticed that this has some confusing effects in our test suites. I
wonder if this is a good idea in general.

For example, the TAP test infrastructure sets PGAPPNAME to allow
identifying clients in the server log. But this environment variable is
also inherited by temporary servers started with pg_ctl and is then in
turn used by libpqwalreceiver as the application_name for connecting to
remote servers where it then shows up in pg_stat_replication and is
relevant for things like synchronous_standby_names.

Also, the pg_rewind tests appear to rely implicitly on PGHOST and PGPORT
being set in the server environment to find the other node via
primary_conninfo. That is easy to fix, but it shows that this kind of
thing can creep in unintentionally.

I was thinking that maybe we should clear all libpq environment
variables in the server, or perhaps have a mode in libpq to ignore all
environment variables. Then again, maybe setting something like
PGSSLMODE globally in the server could be useful, so just removing
everything might not be the right answer.

As an author of the C++ client API (Pgfe) that currently wraps libpq I
would like
to ignore all these environment variables that affects libpq's behavior, because
libpq is an implementation detail and the Pgfe API hides it
completely. So +1 from
me for such a mode in libpq.

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

almost 7 years ago

In reply to: Peter Eisentraut (#1)

2 attachment(s)

Re: libpq environment variables in the server

On 2019-01-21 11:42, Peter Eisentraut wrote:

Maybe pg_ctl should have some functionality to clear the environment,
and maybe there could be a facility in postgresql.conf to set
environment variables.

After pondering this, this seemed to be the best solution.

Many init systems already clear the environment by default, so that the
started services don't have random environment settings inherited from
the user. However, you can't easily do this when using pg_ctl directly,
so having an option to clear the environment in pg_ctl seems generally
useful. Then we can use it in the test suites and thus avoid
environment variables leaking in in unintended ways.

That revealed that we do need the second mechanism to set environment
variables after everything has been cleared. One example in the test
suite is LDAPCONF. But there are other cases that could be useful, such
as any environment settings that would support archive_command or
restore_command. I think this would also be a nice feature in general,
so that you can keep all the configuration together in the configuration
files and don't have to rely on external mechanisms to inject these
environment variables.

Attached are two patches that implement these features.

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

Attachments:

v1-0001-Add-GUC-parameter-environment.patchtext/plain; charset=UTF-8; name=v1-0001-Add-GUC-parameter-environment.patch; x-mac-creator=0; x-mac-type=0Download

From dd76033c87ddfe60c50e6bb397c123f489cadc1c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 18 Feb 2019 14:36:53 +0100
Subject: [PATCH v1 1/2] Add GUC parameter "environment"

This allows setting environment variables inside the server processes,
for use by third-party libraries, like LDAPCONF.  Previously, this had
to be done via external mechanisms such as init scripts.
---
 doc/src/sgml/config.sgml                      | 21 ++++++++++
 src/backend/utils/misc/guc.c                  | 40 +++++++++++++++++++
 src/backend/utils/misc/postgresql.conf.sample |  1 +
 src/include/utils/guc.h                       |  2 +
 4 files changed, 64 insertions(+)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8bd57f376b..acbcf5559b 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8304,6 +8304,27 @@ <title>Other Defaults</title>
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-environment" xreflabel="environment">
+      <term><varname>environment</varname> (<type>string</type>)
+      <indexterm>
+       <primary><varname>environment</varname> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        A comma-separated list of environment variable assignments to apply
+        inside the <productname>PostgreSQL</productname> server process.  This
+        can be used to supply configuration information or debugging settings
+        to third-party libraries or plugins.  Example:
+<programlisting>environment = 'LDAPCONF=/some/file,RSYNC_RSH=ssh'</programlisting>
+        This parameter can only be set in the
+        <filename>postgresql.conf</filename> file or on the server command
+        line.  Note that changing this parameter at run time will not unset
+        previously set environment variables.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="guc-gin-fuzzy-search-limit" xreflabel="gin_fuzzy_search_limit">
       <term><varname>gin_fuzzy_search_limit</varname> (<type>integer</type>)
       <indexterm>
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 156d147c85..10c9528729 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -212,6 +212,7 @@ static bool check_recovery_target_lsn(char **newval, void **extra, GucSource sou
 static void assign_recovery_target_lsn(const char *newval, void *extra);
 static bool check_primary_slot_name(char **newval, void **extra, GucSource source);
 static bool check_default_with_oids(bool *newval, void **extra, GucSource source);
+static void assign_environment_settings(const char *newval, void *extra);
 
 /* Private functions in guc-file.l that need to be called from guc.c */
 static ConfigVariable *ProcessConfigFileInternal(GucContext context,
@@ -529,6 +530,8 @@ int			tcp_keepalives_idle;
 int			tcp_keepalives_interval;
 int			tcp_keepalives_count;
 
+char	   *environment_settings;
+
 /*
  * SSL renegotiation was been removed in PostgreSQL 9.5, but we tolerate it
  * being set to zero (meaning never renegotiate) for backward compatibility.
@@ -3516,6 +3519,17 @@ static struct config_string ConfigureNamesString[] =
 		check_client_encoding, assign_client_encoding, NULL
 	},
 
+	{
+		{"environment", PGC_SIGHUP, CLIENT_CONN_OTHER,
+			gettext_noop("Environment variables to be set."),
+			NULL,
+			GUC_LIST_INPUT | GUC_LIST_QUOTE
+		},
+		&environment_settings,
+		"",
+		NULL, assign_environment_settings, NULL
+	},
+
 	{
 		{"log_line_prefix", PGC_SIGHUP, LOGGING_WHAT,
 			gettext_noop("Controls information prefixed to each log line."),
@@ -11384,4 +11398,30 @@ check_default_with_oids(bool *newval, void **extra, GucSource source)
 	return true;
 }
 
+static void
+assign_environment_settings(const char *newval, void *extra)
+{
+	char *rawstring = pstrdup(newval);
+	List *elemlist;
+	ListCell *lc;
+
+	if (!SplitGUCList(rawstring, ',', &elemlist))
+	{
+		/* syntax error in list */
+		list_free_deep(elemlist);
+		pfree(rawstring);
+		ereport(ERROR,
+				(errcode(ERRCODE_SYNTAX_ERROR),
+				 errmsg("invalid list syntax in parameter \"%s\"",
+						"parameter")));
+	}
+
+	foreach(lc, elemlist)
+	{
+		char	   *setting = lfirst(lc);
+
+		putenv(guc_strdup(ERROR, setting));
+	}
+}
+
 #include "guc-file.c"
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 194f312096..551a02d2aa 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -676,6 +676,7 @@
 # - Other Defaults -
 
 #dynamic_library_path = '$libdir'
+#environment = ''
 
 
 #------------------------------------------------------------------------------
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index c07e7b945e..fdcbb90bb2 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -273,6 +273,8 @@ extern int	tcp_keepalives_count;
 extern bool trace_sort;
 #endif
 
+extern char *environment_settings;
+
 /*
  * Functions exported by guc.c
  */

base-commit: 050710b36964dee7e1b2bf6b5ef00041fd5d2787
-- 
2.20.1

v1-0002-pg_ctl-Add-clear-environment-option.patchtext/plain; charset=UTF-8; name=v1-0002-pg_ctl-Add-clear-environment-option.patch; x-mac-creator=0; x-mac-type=0Download

From 6e90d7fb54609cbe2806d3c10e0e36c49ab4f0c5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 18 Feb 2019 14:36:53 +0100
Subject: [PATCH v1 2/2] pg_ctl: Add --clear-environment option

This clears the environment for the newly started server process.
This should be the preferred practice.  Some init systems already do
this, but this option also gives that functionality when invoking
pg_ctl directly.

Also use this option in the TAP tests.  This revealed that a test in
the pg_rewrind suite relied on some environment variables being
inherited from the test environment.  Fix that case.
---
 doc/src/sgml/ref/pg_ctl-ref.sgml  | 12 ++++++++++++
 src/bin/pg_ctl/pg_ctl.c           | 29 +++++++++++++++++++++--------
 src/bin/pg_rewind/t/RewindTest.pm |  4 ++--
 src/test/ldap/t/001_auth.pl       |  3 +++
 src/test/perl/PostgresNode.pm     |  3 ++-
 5 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/ref/pg_ctl-ref.sgml b/doc/src/sgml/ref/pg_ctl-ref.sgml
index e31275a04e..8f0f832fdb 100644
--- a/doc/src/sgml/ref/pg_ctl-ref.sgml
+++ b/doc/src/sgml/ref/pg_ctl-ref.sgml
@@ -280,6 +280,18 @@ <title>Options</title>
       </listitem>
      </varlistentry>
 
+     <varlistentry>
+      <term><option>--clear-environment</option></term>
+      <listitem>
+       <para>
+        Clear all environment variables in the started server process.  This
+        is recommended for init scripts.  Note that this needs to be specified
+        for both the <literal>start</literal> and any
+        <literal>restart</literal> actions.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry>
       <term><option>-D <replaceable class="parameter">datadir</replaceable></option></term>
       <term><option>--pgdata=<replaceable class="parameter">datadir</replaceable></option></term>
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index c82a702ffa..2eaadba3b1 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -95,6 +95,7 @@ static char *register_password = NULL;
 static char *argv0 = NULL;
 static bool allow_core_files = false;
 static time_t start_time;
+static bool clear_environment = false;
 
 static char postopts_file[MAXPGPATH];
 static char version_file[MAXPGPATH];
@@ -103,6 +104,7 @@ static char backup_file[MAXPGPATH];
 static char promote_file[MAXPGPATH];
 static char logrotate_file[MAXPGPATH];
 
+static char pg_grandparent_pid_env_var[32];
 static volatile pgpid_t postmasterPID = -1;
 
 #ifdef WIN32
@@ -448,6 +450,8 @@ static pgpid_t
 start_postmaster(void)
 {
 	char		cmd[MAXPGPATH];
+	extern char **environ;
+	static char *clean_postmaster_env[] = {"LANG=C", NULL, NULL};
 
 #ifndef WIN32
 	pgpid_t		pm_pid;
@@ -486,6 +490,14 @@ start_postmaster(void)
 	}
 #endif
 
+	if (pg_grandparent_pid_env_var[0])
+	{
+		if (clear_environment)
+			clean_postmaster_env[1] = pg_grandparent_pid_env_var;
+		else
+			putenv(pg_grandparent_pid_env_var);
+	}
+
 	/*
 	 * Since there might be quotes to handle here, it is easier simply to pass
 	 * everything to a shell to process them.  Use exec so that the postmaster
@@ -499,7 +511,8 @@ start_postmaster(void)
 		snprintf(cmd, MAXPGPATH, "exec \"%s\" %s%s < \"%s\" 2>&1",
 				 exec_path, pgdata_opt, post_opts, DEVNULL);
 
-	(void) execl("/bin/sh", "/bin/sh", "-c", cmd, (char *) NULL);
+	(void) execle("/bin/sh", "/bin/sh", "-c", cmd, (char *) NULL,
+				  clear_environment ? clean_postmaster_env : environ);
 
 	/* exec failed */
 	write_stderr(_("%s: could not start server: %s\n"),
@@ -853,13 +866,8 @@ do_start(void)
 	 * getppid() unfortunately.
 	 */
 #ifndef WIN32
-	{
-		static char env_var[32];
-
-		snprintf(env_var, sizeof(env_var), "PG_GRANDPARENT_PID=%d",
-				 (int) getppid());
-		putenv(env_var);
-	}
+	snprintf(pg_grandparent_pid_env_var, sizeof(pg_grandparent_pid_env_var),
+			 "PG_GRANDPARENT_PID=%d", (int) getppid());
 #endif
 
 	pm_pid = start_postmaster();
@@ -2057,6 +2065,7 @@ do_help(void)
 #else
 	printf(_("  -c, --core-files       not applicable on this platform\n"));
 #endif
+	printf(_("      --clear-environment  clear environment variables in postgres process\n"));
 	printf(_("  -l, --log=FILENAME     write (or append) server log to FILENAME\n"));
 	printf(_("  -o, --options=OPTIONS  command line options to pass to postgres\n"
 			 "                         (PostgreSQL server executable) or initdb\n"));
@@ -2260,6 +2269,7 @@ main(int argc, char **argv)
 		{"core-files", no_argument, NULL, 'c'},
 		{"wait", no_argument, NULL, 'w'},
 		{"no-wait", no_argument, NULL, 'W'},
+		{"clear-environment", no_argument, NULL, 1},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -2415,6 +2425,9 @@ main(int argc, char **argv)
 				case 'c':
 					allow_core_files = true;
 					break;
+				case 1:
+					clear_environment = true;
+					break;
 				default:
 					/* getopt_long already issued a suitable error message */
 					do_advice();
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 85cae7e47b..ea0466cdce 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -268,10 +268,10 @@ sub run_pg_rewind
 		"unable to set permissions for $master_pgdata/postgresql.conf");
 
 	# Plug-in rewound node to the now-promoted standby node
-	my $port_standby = $node_standby->port;
+	my $connstr_standby = $node_standby->connstr();
 	$node_master->append_conf(
 		'postgresql.conf', qq(
-primary_conninfo='port=$port_standby'
+primary_conninfo='$connstr_standby'
 ));
 
 	$node_master->set_standby_mode();
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index 431ad6442c..1fa8f7b554 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -269,6 +269,9 @@ sub test_access
 $node->append_conf('pg_hba.conf',
 	qq{local all all ldap ldapserver=$ldap_server ldapport=$ldap_port ldapbasedn="$ldap_basedn" ldapsearchfilter="(uid=\$username)" ldaptls=1}
 );
+$node->append_conf('postgresql.conf',
+	qq{environment = 'LDAPCONF=$ldap_conf'}
+);
 $node->restart;
 
 $ENV{"PGPASSWORD"} = 'secret1';
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 8a2c6fc122..25aa8c38ea 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -701,7 +701,7 @@ sub start
 	BAIL_OUT("node \"$name\" is already running") if defined $self->{_pid};
 	print("### Starting node \"$name\"\n");
 	my $ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
-		$self->logfile, 'start');
+		$self->logfile, '--clear-environment', 'start');
 
 	if ($ret != 0)
 	{
@@ -776,6 +776,7 @@ sub restart
 	my $name    = $self->name;
 	print "### Restarting node \"$name\"\n";
 	TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
+		'--clear-environment',
 		'restart');
 	$self->_update_pid(1);
 	return;
-- 
2.20.1

Noah Misch

noah@leadboat.com

almost 7 years ago

In reply to: Peter Eisentraut (#3)

Re: libpq environment variables in the server

On Mon, Jan 21, 2019 at 11:42:16AM +0100, Peter Eisentraut wrote:

For example, the TAP test infrastructure sets PGAPPNAME to allow
identifying clients in the server log. But this environment variable is
also inherited by temporary servers started with pg_ctl and is then in
turn used by libpqwalreceiver as the application_name for connecting to
remote servers where it then shows up in pg_stat_replication and is
relevant for things like synchronous_standby_names.

+1 for clearing PGAPPNAME before starting a test postmaster.

Also, the pg_rewind tests appear to rely implicitly on PGHOST and PGPORT
being set in the server environment to find the other node via
primary_conninfo. That is easy to fix, but it shows that this kind of
thing can creep in unintentionally.

That's not a defect, and I wouldn't assume it's unintentional.

On Mon, Feb 18, 2019 at 02:58:09PM +0100, Peter Eisentraut wrote:

On 2019-01-21 11:42, Peter Eisentraut wrote:

Maybe pg_ctl should have some functionality to clear the environment,
and maybe there could be a facility in postgresql.conf to set
environment variables.

After pondering this, this seemed to be the best solution.

Many init systems already clear the environment by default, so that the
started services don't have random environment settings inherited from
the user. However, you can't easily do this when using pg_ctl directly,

I think this qualifies:
env -i LANG=C "$(type -p pg_ctl)" -w restart -D "$PGDATA"

so having an option to clear the environment in pg_ctl seems generally
useful.

An option would offer value on Windows, where "env" is not installed by
default. (Your current patch makes the flag a no-op on Windows, but I suppose
that would have been part of finishing the patch.) However, I'd also expect
more unintended consequences from wiping the environment on Windows. Module
loads will need %PATH% to find dependent libraries, and I'd hesitate to remove
a ubiquitous variable like %SYSTEMROOT%. Overall, I'm -1 on having this
--clear-environment option.

Then we can use it in the test suites and thus avoid
environment variables leaking in in unintended ways.

Some of my buildfarm animals[1]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hornet&dt=2019-03-02%2009%3A22%3A14 do need LD_LIBRARY_PATH preserved. Once the
TAP suites support[2]/messages/by-id/20181229021950.GA3302966@rfd.leadboat.com TEMP_CONFIG, I suppose testers could use that to mutate
the "environment" GUC. This test suite change has only cosmetic benefits,
which one can instead achieve by clearing just PGAPPNAME. Since clearing all
environment variables in test suite postmasters would break existing setups
until they adjust, -1 on that even if we end up gaining a --clear-environment
option. Testing a bare environment even reduces our test coverage somewhat.

That revealed that we do need the second mechanism to set environment
variables after everything has been cleared. One example in the test
suite is LDAPCONF. But there are other cases that could be useful, such
as any environment settings that would support archive_command or
restore_command. I think this would also be a nice feature in general,
so that you can keep all the configuration together in the configuration
files and don't have to rely on external mechanisms to inject these
environment variables.

That's slightly nice.

+        line.  Note that changing this parameter at run time will not unset
+        previously set environment variables.

I prefer to see that fixed or the setting made PGC_POSTMASTER.

PGC_SIGHUP makes this an interesting feature. Suppose you install a new
loadable module that relies on an LD_LIBRARY_PATH setting. I've never needed
it myself, but I can see value in being able to mutate LD_LIBRARY_PATH without
a postmaster restart. At PGC_POSTMASTER, the value is limited to mild init
script simplification.

When I was committing pgwin32_putenv() fixes 54aa6cc, 202dbdb and 95b9b8a, I
found that Perl, Python (https://bugs.python.org/issue1159) and Tcl all cache
the environment and ignore environment changes subsequent to cache
initialization. Hence, a post-startup GUC change would not affect Perl %ENV
of backends that initialized Perl before the config reload. That's a minor
factor in favor of PGC_POSTMASTER. In general, expect little when changing
the environment after startup.

+		{"environment", PGC_SIGHUP, CLIENT_CONN_OTHER,
+			gettext_noop("Environment variables to be set."),
+			NULL,
+			GUC_LIST_INPUT | GUC_LIST_QUOTE

This requires a variable_is_guc_list_quote() update.

+static void
+assign_environment_settings(const char *newval, void *extra)
+{
+	char *rawstring = pstrdup(newval);
+	List *elemlist;
+	ListCell *lc;
+
+	if (!SplitGUCList(rawstring, ',', &elemlist))

All other list GUCs use SplitIdentifierString(); is this right?

+	{
+		/* syntax error in list */
+		list_free_deep(elemlist);
+		pfree(rawstring);
+		ereport(ERROR,
+				(errcode(ERRCODE_SYNTAX_ERROR),
+				 errmsg("invalid list syntax in parameter \"%s\"",
+						"parameter")));

The error-throwing part belongs in a check_ function, not an assign_ function.

+	}
+
+	foreach(lc, elemlist)
+	{
+		char	   *setting = lfirst(lc);
+
+		putenv(guc_strdup(ERROR, setting));
+	}

This leaks a fresh copy on every config reload. That's too much, but I think
it's okay to leak memory when the before-reload value differs from the latest
value.

[1]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hornet&dt=2019-03-02%2009%3A22%3A14
[2]: /messages/by-id/20181229021950.GA3302966@rfd.leadboat.com

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

almost 7 years ago

In reply to: Noah Misch (#4)

1 attachment(s)

Re: libpq environment variables in the server

On 2019-03-03 09:06, Noah Misch wrote:

+1 for clearing PGAPPNAME before starting a test postmaster.

I think this qualifies:
env -i LANG=C "$(type -p pg_ctl)" -w restart -D "$PGDATA"

OK, let's make this simpler then. Here is an updated patch that just
unsets PGAPPNAME around pg_ctl invocations in the testing library, and
then adjusts the tests to remove no longer needed application_name
overrides.

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

Attachments:

0001-Don-t-propagate-PGAPPNAME-through-pg_ctl-in-tests.patchtext/plain; charset=UTF-8; name=0001-Don-t-propagate-PGAPPNAME-through-pg_ctl-in-tests.patch; x-mac-creator=0; x-mac-type=0Download

From 9b9e317882ee3e075da4249c8bd580864784d3ca Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 14 Mar 2019 09:28:36 +0100
Subject: [PATCH] Don't propagate PGAPPNAME through pg_ctl in tests

When libpq is loaded in the server (for instance, by
libpqwalreceiver), it may use libpq environment variables set in the
postmaster environment for connection parameter defaults.  This has
some confusing effects in our test suites.  For example, the TAP test
infrastructure sets PGAPPNAME to allow identifying clients in the
server log.  But this environment variable is also inherited by
temporary servers started with pg_ctl and is then in turn used by
libpqwalreceiver as the application_name for connecting to remote
servers where it then shows up in pg_stat_replication and is relevant
for things like synchronous_standby_names.  Replication already has a
suitable default for application_name, and overriding that
accidentally then requires the individual test cases to re-override
that, which is all very confusing and unnecessary.

To fix, just unset PGAPPNAME before running pg_ctl start or restart
and re-set it afterwards.

More comprehensive approaches like unsetting all environment variables
in pg_ctl were considered but might be too complicated to achieve
portably.

With this, the re-overriding of application_name by test cases can be
removed.

Discussion: https://www.postgresql.org/message-id/flat/33383613-690e-6f1b-d5ba-4957ff40f6ce@2ndquadrant.com
---
 src/bin/pg_rewind/t/RewindTest.pm          |  4 ++--
 src/test/perl/PostgresNode.pm              | 21 ++++++++++++++++-
 src/test/recovery/t/004_timeline_switch.pl |  2 +-
 src/test/subscription/t/001_rep_changes.pl | 27 +++++++++++-----------
 src/test/subscription/t/002_types.pl       | 13 +++++------
 src/test/subscription/t/003_constraints.pl | 11 ++++-----
 src/test/subscription/t/004_sync.pl        | 15 ++++++------
 src/test/subscription/t/005_encoding.pl    |  7 +++---
 src/test/subscription/t/006_rewrite.pl     | 11 ++++-----
 src/test/subscription/t/007_ddl.pl         |  5 ++--
 src/test/subscription/t/008_diff_schema.pl | 11 ++++-----
 src/test/subscription/t/009_matviews.pl    |  7 +++---
 src/test/subscription/t/010_truncate.pl    |  6 ++---
 13 files changed, 75 insertions(+), 65 deletions(-)

diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 85cae7e47b..401acb7ddc 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -160,7 +160,7 @@ sub create_standby
 
 	$node_standby->append_conf(
 		"postgresql.conf", qq(
-primary_conninfo='$connstr_master application_name=rewind_standby'
+primary_conninfo='$connstr_master'
 ));
 
 	$node_standby->set_standby_mode();
@@ -180,7 +180,7 @@ sub promote_standby
 	# up standby
 
 	# Wait for the standby to receive and write all WAL.
-	$node_master->wait_for_catchup('rewind_standby', 'write');
+	$node_master->wait_for_catchup($node_standby, 'write');
 
 	# Now promote standby and insert some new data on master, this will put
 	# the master out-of-sync with the standby.
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 0634aefd20..7640b500c4 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -698,13 +698,24 @@ sub start
 	my $port   = $self->port;
 	my $pgdata = $self->data_dir;
 	my $name   = $self->name;
+
 	BAIL_OUT("node \"$name\" is already running") if defined $self->{_pid};
+
 	print("### Starting node \"$name\"\n");
+
+	# Temporarily unset PGAPPNAME so that the server doesn't inherit
+	# it.  Otherwise this could affect libpqwalreceiver connections in
+	# confusing ways.
+	my $save_pgappname = $ENV{PGAPPNAME};
+	delete $ENV{PGAPPNAME};
+
 	# Note: We set the cluster_name here, not in postgresql.conf (in
 	# sub init) so that it does not get copied to standbys.
 	my $ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
 		$self->logfile, '-o', "--cluster-name=$name", 'start');
 
+	$ENV{PGAPPNAME} = $save_pgappname;
+
 	if ($ret != 0)
 	{
 		print "# pg_ctl start failed; logfile:\n";
@@ -776,9 +787,17 @@ sub restart
 	my $pgdata  = $self->data_dir;
 	my $logfile = $self->logfile;
 	my $name    = $self->name;
+
 	print "### Restarting node \"$name\"\n";
+
+	my $save_pgappname = $ENV{PGAPPNAME};
+	delete $ENV{PGAPPNAME};
+
 	TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
 		'restart');
+
+	$ENV{PGAPPNAME} = $save_pgappname;
+
 	$self->_update_pid(1);
 	return;
 }
@@ -835,7 +854,7 @@ sub enable_streaming
 	print "### Enabling streaming replication for node \"$name\"\n";
 	$self->append_conf(
 		'postgresql.conf', qq(
-primary_conninfo='$root_connstr application_name=$name'
+primary_conninfo='$root_connstr'
 ));
 	$self->set_standby_mode();
 	return;
diff --git a/src/test/recovery/t/004_timeline_switch.pl b/src/test/recovery/t/004_timeline_switch.pl
index 2b315854bc..65270430bf 100644
--- a/src/test/recovery/t/004_timeline_switch.pl
+++ b/src/test/recovery/t/004_timeline_switch.pl
@@ -50,7 +50,7 @@
 my $connstr_1 = $node_standby_1->connstr;
 $node_standby_2->append_conf(
 	'postgresql.conf', qq(
-primary_conninfo='$connstr_1 application_name=@{[$node_standby_2->name]}'
+primary_conninfo='$connstr_1'
 ));
 $node_standby_2->restart;
 
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index d94458e00e..40e306a810 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -63,12 +63,11 @@
 $node_publisher->safe_psql('postgres',
 	"ALTER PUBLICATION tap_pub_ins_only ADD TABLE tab_ins");
 
-my $appname = 'tap_sub';
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION tap_pub, tap_pub_ins_only"
+	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub, tap_pub_ins_only"
 );
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 # Also wait for initial table sync to finish
 my $synced_query =
@@ -103,7 +102,7 @@
 	"DELETE FROM tab_include WHERE a > 20");
 $node_publisher->safe_psql('postgres', "UPDATE tab_include SET a = -a");
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 $result = $node_subscriber->safe_psql('postgres',
 	"SELECT count(*), min(a), max(a) FROM tab_ins");
@@ -146,7 +145,7 @@
 $node_publisher->safe_psql('postgres',
 	"UPDATE tab_full2 SET x = 'bb' WHERE x = 'b'");
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 $result = $node_subscriber->safe_psql('postgres',
 	"SELECT count(*), min(a), max(a) FROM tab_full");
@@ -165,23 +164,23 @@
 # as we need to poll for a change but the test suite will fail none the less
 # when something goes wrong.
 my $oldpid = $node_publisher->safe_psql('postgres',
-	"SELECT pid FROM pg_stat_replication WHERE application_name = '$appname';"
+	"SELECT pid FROM pg_stat_replication WHERE application_name = 'tap_sub';"
 );
 $node_subscriber->safe_psql('postgres',
-	"ALTER SUBSCRIPTION tap_sub CONNECTION 'application_name=$appname $publisher_connstr'"
+	"ALTER SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr sslmode=disable'"
 );
 $node_publisher->poll_query_until('postgres',
-	"SELECT pid != $oldpid FROM pg_stat_replication WHERE application_name = '$appname';"
+	"SELECT pid != $oldpid FROM pg_stat_replication WHERE application_name = 'tap_sub';"
 ) or die "Timed out while waiting for apply to restart";
 
 $oldpid = $node_publisher->safe_psql('postgres',
-	"SELECT pid FROM pg_stat_replication WHERE application_name = '$appname';"
+	"SELECT pid FROM pg_stat_replication WHERE application_name = 'tap_sub';"
 );
 $node_subscriber->safe_psql('postgres',
 	"ALTER SUBSCRIPTION tap_sub SET PUBLICATION tap_pub_ins_only WITH (copy_data = false)"
 );
 $node_publisher->poll_query_until('postgres',
-	"SELECT pid != $oldpid FROM pg_stat_replication WHERE application_name = '$appname';"
+	"SELECT pid != $oldpid FROM pg_stat_replication WHERE application_name = 'tap_sub';"
 ) or die "Timed out while waiting for apply to restart";
 
 $node_publisher->safe_psql('postgres',
@@ -193,7 +192,7 @@
 $node_publisher->stop('fast');
 $node_publisher->start;
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 $result = $node_subscriber->safe_psql('postgres',
 	"SELECT count(*), min(a), max(a) FROM tab_ins");
@@ -216,7 +215,7 @@
 );
 $node_publisher->safe_psql('postgres', "INSERT INTO tab_full VALUES(0)");
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 # note that data are different on provider and subscriber
 $result = $node_subscriber->safe_psql('postgres',
@@ -230,12 +229,12 @@
 
 # check restart on rename
 $oldpid = $node_publisher->safe_psql('postgres',
-	"SELECT pid FROM pg_stat_replication WHERE application_name = '$appname';"
+	"SELECT pid FROM pg_stat_replication WHERE application_name = 'tap_sub';"
 );
 $node_subscriber->safe_psql('postgres',
 	"ALTER SUBSCRIPTION tap_sub RENAME TO tap_sub_renamed");
 $node_publisher->poll_query_until('postgres',
-	"SELECT pid != $oldpid FROM pg_stat_replication WHERE application_name = '$appname';"
+	"SELECT pid != $oldpid FROM pg_stat_replication WHERE application_name = 'tap_sub_renamed';"
 ) or die "Timed out while waiting for apply to restart";
 
 # check all the cleanup
diff --git a/src/test/subscription/t/002_types.pl b/src/test/subscription/t/002_types.pl
index 30a3841bca..d691bd17a6 100644
--- a/src/test/subscription/t/002_types.pl
+++ b/src/test/subscription/t/002_types.pl
@@ -107,12 +107,11 @@
 $node_publisher->safe_psql('postgres',
 	"CREATE PUBLICATION tap_pub FOR ALL TABLES");
 
-my $appname = 'tap_sub';
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION tap_pub WITH (slot_name = tap_sub_slot)"
+	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub WITH (slot_name = tap_sub_slot)"
 );
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 # Wait for initial sync to finish as well
 my $synced_query =
@@ -251,7 +250,7 @@
 	INSERT INTO tst_dom_constr VALUES (10);
 ));
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 # Check the data on subscriber
 my $result = $node_subscriber->safe_psql(
@@ -372,7 +371,7 @@
 	UPDATE tst_hstore SET b = '"also"=>"updated"' WHERE a = 3;
 ));
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 # Check the data on subscriber
 $result = $node_subscriber->safe_psql(
@@ -492,7 +491,7 @@
 	DELETE FROM tst_hstore WHERE a = 1;
 ));
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 # Check the data on subscriber
 $result = $node_subscriber->safe_psql(
@@ -554,7 +553,7 @@
 # which needs an active snapshot in order to operate.
 $node_publisher->safe_psql('postgres', "INSERT INTO tst_dom_constr VALUES (11)");
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 $result =
   $node_subscriber->safe_psql('postgres', "SELECT sum(a) FROM tst_dom_constr");
diff --git a/src/test/subscription/t/003_constraints.pl b/src/test/subscription/t/003_constraints.pl
index a5b548ecee..81547f65fa 100644
--- a/src/test/subscription/t/003_constraints.pl
+++ b/src/test/subscription/t/003_constraints.pl
@@ -34,19 +34,18 @@
 $node_publisher->safe_psql('postgres',
 	"CREATE PUBLICATION tap_pub FOR ALL TABLES;");
 
-my $appname = 'tap_sub';
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION tap_pub WITH (copy_data = false)"
+	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub WITH (copy_data = false)"
 );
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 $node_publisher->safe_psql('postgres',
 	"INSERT INTO tab_fk (bid) VALUES (1);");
 $node_publisher->safe_psql('postgres',
 	"INSERT INTO tab_fk_ref (id, bid) VALUES (1, 1);");
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 # Check data on subscriber
 my $result = $node_subscriber->safe_psql('postgres',
@@ -64,7 +63,7 @@
 $node_publisher->safe_psql('postgres',
 	"INSERT INTO tab_fk_ref (id, bid) VALUES (2, 2);");
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 # FK is not enforced on subscriber
 $result = $node_subscriber->safe_psql('postgres',
@@ -98,7 +97,7 @@ BEGIN
 $node_publisher->safe_psql('postgres',
 	"INSERT INTO tab_fk_ref (id, bid) VALUES (10, 10);");
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 # The row should be skipped on subscriber
 $result = $node_subscriber->safe_psql('postgres',
diff --git a/src/test/subscription/t/004_sync.pl b/src/test/subscription/t/004_sync.pl
index f8b8f1a3d2..e111ab9181 100644
--- a/src/test/subscription/t/004_sync.pl
+++ b/src/test/subscription/t/004_sync.pl
@@ -32,12 +32,11 @@
 $node_publisher->safe_psql('postgres',
 	"CREATE PUBLICATION tap_pub FOR ALL TABLES");
 
-my $appname = 'tap_sub';
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION tap_pub"
+	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub"
 );
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 # Also wait for initial table sync to finish
 my $synced_query =
@@ -57,7 +56,7 @@
 
 # recreate the subscription, it will try to do initial copy
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION tap_pub"
+	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub"
 );
 
 # but it will be stuck on data copy as it will fail on constraint
@@ -79,7 +78,7 @@
 
 # now check another subscription for the same node pair
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION tap_sub2 CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION tap_pub WITH (copy_data = false)"
+	"CREATE SUBSCRIPTION tap_sub2 CONNECTION '$publisher_connstr' PUBLICATION tap_pub WITH (copy_data = false)"
 );
 
 # wait for it to start
@@ -101,7 +100,7 @@
 
 # recreate the subscription again
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION tap_pub"
+	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub"
 );
 
 # and wait for data sync to finish again
@@ -120,7 +119,7 @@
 $node_publisher->safe_psql('postgres',
 	"CREATE TABLE tab_rep_next (a) AS SELECT generate_series(1,10)");
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 $result = $node_subscriber->safe_psql('postgres',
 	"SELECT count(*) FROM tab_rep_next");
@@ -143,7 +142,7 @@
 $node_publisher->safe_psql('postgres',
 	"INSERT INTO tab_rep_next SELECT generate_series(1,10)");
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 $result = $node_subscriber->safe_psql('postgres',
 	"SELECT count(*) FROM tab_rep_next");
diff --git a/src/test/subscription/t/005_encoding.pl b/src/test/subscription/t/005_encoding.pl
index 1977aa5cfe..aec7a17a78 100644
--- a/src/test/subscription/t/005_encoding.pl
+++ b/src/test/subscription/t/005_encoding.pl
@@ -22,15 +22,14 @@
 $node_subscriber->safe_psql('postgres', $ddl);
 
 my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
-my $appname           = 'encoding_test';
 
 $node_publisher->safe_psql('postgres',
 	"CREATE PUBLICATION mypub FOR ALL TABLES;");
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION mysub CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION mypub;"
+	"CREATE SUBSCRIPTION mysub CONNECTION '$publisher_connstr' PUBLICATION mypub;"
 );
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('mysub');
 
 # Wait for initial sync to finish as well
 my $synced_query =
@@ -41,7 +40,7 @@
 $node_publisher->safe_psql('postgres',
 	q{INSERT INTO test1 VALUES (1, E'Mot\xc3\xb6rhead')}); # hand-rolled UTF-8
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('mysub');
 
 is( $node_subscriber->safe_psql(
 		'postgres', q{SELECT a FROM test1 WHERE b = E'Mot\xf6rhead'}
diff --git a/src/test/subscription/t/006_rewrite.pl b/src/test/subscription/t/006_rewrite.pl
index e470c071d2..c6cda10a19 100644
--- a/src/test/subscription/t/006_rewrite.pl
+++ b/src/test/subscription/t/006_rewrite.pl
@@ -18,15 +18,14 @@
 $node_subscriber->safe_psql('postgres', $ddl);
 
 my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
-my $appname           = 'encoding_test';
 
 $node_publisher->safe_psql('postgres',
 	"CREATE PUBLICATION mypub FOR ALL TABLES;");
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION mysub CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION mypub;"
+	"CREATE SUBSCRIPTION mysub CONNECTION '$publisher_connstr' PUBLICATION mypub;"
 );
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('mysub');
 
 # Wait for initial sync to finish as well
 my $synced_query =
@@ -37,7 +36,7 @@
 $node_publisher->safe_psql('postgres',
 	q{INSERT INTO test1 (a, b) VALUES (1, 'one'), (2, 'two');});
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('mysub');
 
 is( $node_subscriber->safe_psql('postgres', q{SELECT a, b FROM test1}),
 	qq(1|one
@@ -49,12 +48,12 @@
 $node_subscriber->safe_psql('postgres', $ddl2);
 $node_publisher->safe_psql('postgres', $ddl2);
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('mysub');
 
 $node_publisher->safe_psql('postgres',
 	q{INSERT INTO test1 (a, b, c) VALUES (3, 'three', 33);});
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('mysub');
 
 is( $node_subscriber->safe_psql('postgres', q{SELECT a, b, c FROM test1}),
 	qq(1|one|0
diff --git a/src/test/subscription/t/007_ddl.pl b/src/test/subscription/t/007_ddl.pl
index 2697ee5c58..7fe6cc6d63 100644
--- a/src/test/subscription/t/007_ddl.pl
+++ b/src/test/subscription/t/007_ddl.pl
@@ -18,15 +18,14 @@
 $node_subscriber->safe_psql('postgres', $ddl);
 
 my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
-my $appname           = 'replication_test';
 
 $node_publisher->safe_psql('postgres',
 	"CREATE PUBLICATION mypub FOR ALL TABLES;");
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION mysub CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION mypub;"
+	"CREATE SUBSCRIPTION mysub CONNECTION '$publisher_connstr' PUBLICATION mypub;"
 );
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('mysub');
 
 $node_subscriber->safe_psql(
 	'postgres', q{
diff --git a/src/test/subscription/t/008_diff_schema.pl b/src/test/subscription/t/008_diff_schema.pl
index 22b76f1b17..3ad00eae3b 100644
--- a/src/test/subscription/t/008_diff_schema.pl
+++ b/src/test/subscription/t/008_diff_schema.pl
@@ -31,12 +31,11 @@
 $node_publisher->safe_psql('postgres',
 	"CREATE PUBLICATION tap_pub FOR TABLE test_tab");
 
-my $appname = 'tap_sub';
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION tap_pub"
+	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub"
 );
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 # Also wait for initial table sync to finish
 my $synced_query =
@@ -53,7 +52,7 @@
 # subscriber didn't change
 $node_publisher->safe_psql('postgres', "UPDATE test_tab SET b = md5(b)");
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 $result =
   $node_subscriber->safe_psql('postgres',
@@ -70,7 +69,7 @@
 $node_publisher->safe_psql('postgres',
 	"UPDATE test_tab SET b = md5(a::text)");
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 $result = $node_subscriber->safe_psql('postgres',
 	"SELECT count(*), count(extract(epoch from c) = 987654321), count(d = 999) FROM test_tab"
@@ -81,7 +80,7 @@
 $node_publisher->safe_psql('postgres',
 	"INSERT INTO test_tab VALUES (3, 'baz')");
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('tap_sub');
 
 $result =
   $node_subscriber->safe_psql('postgres',
diff --git a/src/test/subscription/t/009_matviews.pl b/src/test/subscription/t/009_matviews.pl
index ea2ee420ca..7afc7bdba9 100644
--- a/src/test/subscription/t/009_matviews.pl
+++ b/src/test/subscription/t/009_matviews.pl
@@ -14,12 +14,11 @@
 $node_subscriber->start;
 
 my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
-my $appname           = 'replication_test';
 
 $node_publisher->safe_psql('postgres',
 	"CREATE PUBLICATION mypub FOR ALL TABLES;");
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION mysub CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION mypub;"
+	"CREATE SUBSCRIPTION mysub CONNECTION '$publisher_connstr' PUBLICATION mypub;"
 );
 
 $node_publisher->safe_psql('postgres',
@@ -30,7 +29,7 @@
 $node_subscriber->safe_psql('postgres',
 	q{CREATE TABLE test1 (a int PRIMARY KEY, b text);});
 
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('mysub');
 
 # Materialized views are not supported by logical replication, but
 # logical decoding does produce change information for them, so we
@@ -39,7 +38,7 @@
 # create a MV with some data
 $node_publisher->safe_psql('postgres',
 	q{CREATE MATERIALIZED VIEW testmv1 AS SELECT * FROM test1;});
-$node_publisher->wait_for_catchup($appname);
+$node_publisher->wait_for_catchup('mysub');
 
 # There is no equivalent relation on the subscriber, but MV data is
 # not replicated, so this does not hang.
diff --git a/src/test/subscription/t/010_truncate.pl b/src/test/subscription/t/010_truncate.pl
index de1443b55f..be2c0bdc35 100644
--- a/src/test/subscription/t/010_truncate.pl
+++ b/src/test/subscription/t/010_truncate.pl
@@ -52,13 +52,13 @@
 $node_publisher->safe_psql('postgres',
 	"CREATE PUBLICATION pub3 FOR TABLE tab3, tab4");
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr application_name=sub1' PUBLICATION pub1"
+	"CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION pub1"
 );
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION sub2 CONNECTION '$publisher_connstr application_name=sub2' PUBLICATION pub2"
+	"CREATE SUBSCRIPTION sub2 CONNECTION '$publisher_connstr' PUBLICATION pub2"
 );
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION sub3 CONNECTION '$publisher_connstr application_name=sub3' PUBLICATION pub3"
+	"CREATE SUBSCRIPTION sub3 CONNECTION '$publisher_connstr' PUBLICATION pub3"
 );
 
 # Wait for initial sync of all subscriptions
-- 
2.21.0

Noah Misch

noah@leadboat.com

almost 7 years ago

In reply to: Peter Eisentraut (#5)

Re: libpq environment variables in the server

Looks good.

On Thu, Mar 14, 2019 at 12:06:45PM +0100, Peter Eisentraut wrote:

+	# Temporarily unset PGAPPNAME so that the server doesn't inherit
+	# it.  Otherwise this could affect libpqwalreceiver connections in
+	# confusing ways.
+	my $save_pgappname = $ENV{PGAPPNAME};
+	delete $ENV{PGAPPNAME};
+
# Note: We set the cluster_name here, not in postgresql.conf (in
# sub init) so that it does not get copied to standbys.
my $ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
$self->logfile, '-o', "--cluster-name=$name", 'start');

+ $ENV{PGAPPNAME} = $save_pgappname;
+

I consider the following style more idiomatic:

{
local %ENV;
delete $ENV{PGAPPNAME};
...
}

I'm okay with the way you've written it, though.

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

almost 7 years ago

In reply to: Noah Misch (#6)

Re: libpq environment variables in the server

On 2019-03-15 05:00, Noah Misch wrote:

I consider the following style more idiomatic:

{
local %ENV;
delete $ENV{PGAPPNAME};
...
}

That doesn't work because the first line clears the entire environment.

What does work is

{
delete local $ENV{PGAPPNAME};
...
}

But that is documented as new in Perl 5.12.0, so we might not be able to
use it. It appears to work in the 5.8.9 I have lying around, so I'm
confused.

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

Noname

ilmari@ilmari.org

almost 7 years ago

In reply to: Peter Eisentraut (#7)

Re: libpq environment variables in the server

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 2019-03-15 05:00, Noah Misch wrote:

I consider the following style more idiomatic:

{
local %ENV;
delete $ENV{PGAPPNAME};
...
}

That doesn't work because the first line clears the entire environment.

The solution to that is to do 'local %ENV = %ENV;', to assign a copy of
the original to the localised variable. This doesn't work on VMS,
because its concept of environment variables is quite different from
UNIX, but PostgreSQL doesn't support that anyway.

What does work is

{
delete local $ENV{PGAPPNAME};
...
}

But that is documented as new in Perl 5.12.0, so we might not be able to
use it. It appears to work in the 5.8.9 I have lying around, so I'm
confused.

It "works" as in it's not a syntax error, but it doesn't actually
localise the deletion. The following program:

use strict;
use warnings;
use feature 'say';

our %env = qw(foo bar baz bat);
say "original: ", join(", ", sort keys %env);
{
delete local $env{foo};
say "localised: ", join(", ", sort keys %env);
}
say "restored? ", join(", ", sort keys %env);

on 5.12 prints:

original: baz, foo
localised: baz
restored? baz, foo

while on 5.10 it prints:

original: baz, foo
localised: baz
restored? baz

BTW, https://perl.bot/ is handy for testing things like this on various
Perl versions you don't have lying around.

- ilmari
--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl

Noah Misch

noah@leadboat.com

almost 7 years ago

In reply to: Noname (#8)

Re: libpq environment variables in the server

On Fri, Mar 15, 2019 at 10:06:29AM +0000, Dagfinn Ilmari Mannsï¿½ker wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 2019-03-15 05:00, Noah Misch wrote:

I consider the following style more idiomatic:

{
local %ENV;
delete $ENV{PGAPPNAME};
...
}

That doesn't work because the first line clears the entire environment.

The solution to that is to do 'local %ENV = %ENV;', to assign a copy of
the original to the localised variable.

That's the right thing, not what I wrote. We use that in
src/bin/initdb/t/001_initdb.pl.

#10

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

almost 7 years ago

In reply to: Noah Misch (#9)

Re: libpq environment variables in the server

On 2019-03-15 16:01, Noah Misch wrote:

{
local %ENV;
delete $ENV{PGAPPNAME};
...
}

That doesn't work because the first line clears the entire environment.

The solution to that is to do 'local %ENV = %ENV;', to assign a copy of
the original to the localised variable.

That's the right thing, not what I wrote. We use that in
src/bin/initdb/t/001_initdb.pl.

Great. Committed that way.

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