Re: Allow file inclusion in pg_hba and pg_ident files

On Mon, Nov 14, 2022 at 03:47:27PM +0800, Julien Rouhaud wrote:
> On Mon, Nov 14, 2022 at 02:40:37PM +0900, Michael Paquier wrote:
>> If the caller passes NULL for *linectx as the initial line context,
>> just create it as we do now. If *linectx is not NULL, just reuse it.
>> That may be cleaner than returning the created MemoryContext as
>> returned result from tokenize_auth_file().
>
> I originally used two functions as it's a common pattern (e.g. ReadBuffer /
> ReadBufferExtended or all the *_internal versions of functions) and to avoid
> unnecessary branch, but I agree that here having an extra branch is unlikely to
> make any measurable difference. It would only matter on -DEXEC_BACKEND /
> win32, and in that case the extra overhead (over an already expensive new
> backend mechanism) is way more than that, so +1 to keep things simple here.

Oh, Okay. So that was your intention here.

>> This aggregates all the error messages for all the files included in a
>> given repository. As the patch stands, it seems to me that we would
>> get errors related to an include_dir clause for two cases:
>> - The specified path does not exist, in which case we have only one
>> err_msg to consume and report back.
>> - Multiple failures in opening and/or reading included files.
>> In the second case, aggregating the reports would provide a full set
>> of information, but that's not something a user would be able to act
>> on directly as this is system-related. Or there is a case to know a
>> full list of files in the case of multiple files that cannot be read
>> because of permission issues? We may be fine with just the first
>> system error here. Note that in the case where files can be read and
>> opened, these would have their own TokenizedAuthLines for each line
>> parsed, meaning one line in the SQL views once translated to an
>> HbaLine or an IdentLine.
>
> 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.

> 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, 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. 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? 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).
--
Michael