Hi,
On 2018-01-12 09:53:51 -0500, Curt Tilmes wrote:
> I love centralizing connection service definitions in
> <sysconfdir>/pg_service.conf, but for a
> large enterprise, sometimes we have multiple sets of connection
> service definitions
> independently managed.
>
> The convention widely adopted for this type of thing is to allow
> multiple config files to be in
> a directory, usually the '.d' version of the config filename. See, for
> example:
>
> What do you think?
Seems like a reasonable idea to me.
> @@ -4492,10 +4493,14 @@ parseServiceInfo(PQconninfoOption *options,
> PQExpBuffer errorMessage)
> {
> const char *service = conninfo_getval(options, "service");
> char serviceFile[MAXPGPATH];
> + char serviceDirPath[MAXPGPATH];
> char *env;
> bool group_found = false;
> int status;
> struct stat stat_buf;
> + DIR *serviceDir;
> + struct dirent *direntry;
> +
>
> /*
> * We have to special-case the environment variable PGSERVICE here,
> since
> @@ -4539,21 +4544,45 @@ next_file:
> snprintf(serviceFile, MAXPGPATH, "%s/pg_service.conf",
> getenv("PGSYSCONFDIR") ? getenv("PGSYSCONFDIR") :
> SYSCONFDIR);
> if (stat(serviceFile, &stat_buf) != 0)
> + goto conf_dir;
> +
> + status = parseServiceFile(serviceFile, service, options, errorMessage,
> &group_found);
> + if (group_found || status != 0)
> + return status;
> +
> +conf_dir:
> +
This is already pretty crufty, can't we make this look a bit prettier,
rather than extending this approach?
> + /*
> + * Try every file in pg_service.conf.d/*
> + */
> + snprintf(serviceDirPath, MAXPGPATH, "%s/pg_service.conf.d",
> + getenv("PGSYSCONFDIR") ? getenv("PGSYSCONFDIR") :
> SYSCONFDIR);
> +
> + if (stat(serviceDirPath, &stat_buf) != 0 || !S_ISDIR(stat_buf.st_mode)
> ||
> + (serviceDir = opendir(serviceDirPath)) == NULL)
> goto last_file;
This looks a bit ugly.
> - status = parseServiceFile(serviceFile, service, options, errorMessage,
> &group_found);
> - if (status != 0)
> - return status;
> + while ((direntry = readdir(serviceDir)) != NULL)
So there's no really well defined order in which we parse these?
> + {
> + snprintf(serviceFile, MAXPGPATH, "%s/%s", serviceDirPath,
> direntry->d_name);
> +
In my experience with such .conf.d directories it's very useful to
filter names not matching a common pattern. Otherwise you end up with
editor tempfiles and such being used, which gets confusing.
> + if (stat(serviceFile, &stat_buf) != 0 ||
> !S_ISREG(stat_buf.st_mode) ||
> + access(serviceFile, R_OK))
> + continue;
This doesn't look particularly pretty, but ...
Greetings,
Andres Freund
