Re: Let's start using setenv()

On Wed, Dec 30, 2020 at 5:45 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Since the cfbot seems happy with v1, here's a v2 that runs around
> and converts all putenv() uses to setenv(). In some places there's
> no notational savings, but it seems to me that standardizing
> on just one way to do it is beneficial.

+ if (setenv("KRB5_KTNAME", pg_krb_server_keyfile, 0) != 0)
{
- size_t kt_len = strlen(pg_krb_server_keyfile) + 14;
- char *kt_path = malloc(kt_len);
-
- if (!kt_path ||
- snprintf(kt_path, kt_len, "KRB5_KTNAME=%s",
- pg_krb_server_keyfile) != kt_len - 2 ||
- putenv(kt_path) != 0)
- {
- ereport(LOG,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("out of memory")));
- return STATUS_ERROR;
- }
+ /* We assume the error must be "out of memory" */
+ ereport(LOG,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
+ return STATUS_ERROR;
}

Wouldn't it be better to do "cannot set environment: %m" or similar,
and let ENOMEM do its thing if that be the cause? It's true that
POSIX only allows EINVAL or ENOMEM and we can see no reason for EINVAL
here, but still it seems unnecessary to assume.

- if (getenv("PGLOCALEDIR") == NULL)
- {
- /* set for libpq to use */
- snprintf(env_path, sizeof(env_path), "PGLOCALEDIR=%s", path);
- canonicalize_path(env_path + 12);
- dup_path = strdup(env_path);
- if (dup_path)
- putenv(dup_path);
- }
+ /* set for libpq to use, but don't override existing setting */
+ setenv("PGLOCALEDIR", path, 0);

Did you want to drop the canonicalize_path() logic here and nearby?