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
From dd76033c87ddfe60c50e6bb397c123f489cadc1c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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
From 6e90d7fb54609cbe2806d3c10e0e36c49ab4f0c5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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
