Re: Allow file inclusion in pg_hba and pg_ident files

On Tue, Nov 15, 2022 at 08:46:55AM +0900, Michael Paquier wrote:
> On Mon, Nov 14, 2022 at 03:47:27PM +0800, Julien Rouhaud wrote:
> >
> > If you have an include_dir directive and multiple files have wrong permission
> > (or maybe broken symlink or something like that), you will get multiple errors
> > when trying to process that single directive. I think it's friendlier to
> > report as much detail as we can, so users can make sure they fix everything
> > rather than iterating over the first error. That's especially helpful if the
> > fix is done in some external tooling (puppet or whatever) rather than directly
> > on the server.
>
> Hmm, Okay. Well, this would include only errors on I/O or permission
> problems for existing files.. I have not seen deployments that have
> dozens of sub-files for GUCs with an included dir, but perhaps I lack
> of experience on user histories.

While being the same inclusion infrastructure, it's likely that people will
have different usage. I'm assuming that for GUCs the main usage is to have
your automation tool put one of your template conf for instance (small_vm.conf,
big_vm.conf or something like that) in an included directory, so you don't
really need a lot of them. You can also rely on ALTER SYSTEM to avoid manually
handling configuration files entirely.

For authentication there are probably very different pattern. The use case I
had when writing this patch is some complex application that relies on many
services, each service having dedicated role and authentication, and services
can be enabled or disabled dynamically pretty much anytime. I had to write
code to merge possibly new entries with existing pg_hba/pg_ident configuration
files. With this feature it would be much easier (and robust) to simply have a
main pg_hba/pg_ident that includes some directory, and have the service
enable/disable simply creates/removes a dedicated file for each service, and we
then would usually have at least a dozen files there.

I'm assuming people doing multi-tenant can also have similar usage.

> > I think that the problem is that we have the same interface for processing the
> > files on a startup/reload and for filling the views, so if we want the views to
> > be helpful and report all errors we have to also allow a bogus "include" to
> > continue in the reload case too. The same problem doesn't exists for GUCs, so
> > a slightly different behavior there might be acceptable.
>
> Well, there is as well the point that we don't have yet a view for
> GUCs that does the equivalent of HBA and ident,

Yes that's what I meant by "the same problem doesn't exists for GUCs".

> but that does not
> mean, it seems to me, that there should be an inconsistency in the way
> we process those commands because one has implemented a feature but
> not the other. On the contrary, I'd rather try to make them
> consistent.

You mean stopping at the first error, even if it's only for the view reporting?
That will make the reload consistent, but the view will be a bit useless then.

> As things are in the patch, the only difference between
> "include_if_exists" and "include" is that the latter would report some
> information if the file goes missing, the former generates a LOG entry
> about the file being skipped without something in the system view.
> Now, wouldn't it be useful for the end-user to report that a file is
> skipped as an effect of "include_if_exists" in the system views? If
> not, then what's the point of having this clause to begin with?

I don't really get the argument "the proposed monitoring view doesn't give this
information, so the feature isn't needed". Also, unless I'm missing something
the other difference between "include" and "include_if_exists" is that the
first one will refuse to reload the conf (or start the server) if the file is
missing, while the other one will?

> My
> opinion is that both clauses are useful, still on the ground of
> consistency both clauses should work the same as for GUCs. Both
> should report something in err_msg even if that's just about a file
> being skipped, though I agree that this could be considered confusing
> as well for an if_exists clause (does not look like a big deal to me
> based on the debuggability gain).

It would be nice to have the information that an "include_if_exists" file
didn't exist, but having a log-level message in the "error" column is a clear
POLA violation. People will probably just do something like

SELECT file_name, line_number, error FROM pg_hba_file_rules WHERE error IS NOT
NULL;

and report an error if any row is found. Having to parse the error field to
know if that's really an error or not is going to be a huge foot-gun. Maybe we
could indeed report the problem in err_msg but for include_if_exists display it
in some other column of the view?