Re: [PATCH] Find additional connection service files in pg_service.conf.d directory

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