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] do need LD_LIBRARY_PATH preserved. Once the
TAP suites support[2] 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.

nm

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