Re: Allow file inclusion in pg_hba and pg_ident files

On Thu, Nov 17, 2022 at 11:33:05AM +0900, Michael Paquier wrote:
> By the way, I am wondering whether process_included_authfile() is
> the most intuitive interface here. The only thing that prevents a
> common routine to process the include commands is the failure on
> GetConfFilesInDir(), where we need to build a TokenizedAuthLine when
> the others have already done so tokenize_file_with_context(). Could
> it be cleaner to have a small routine like makeTokenizedAuthLine()
> that gets reused when we fail scanning a directory to build a
> TokenizedAuthLine, in combination with a small-ish routine working on
> a directory like ParseConfigDirectory() but for HBA/ident? Or we
> could just drop process_included_authfile() entirely? On failure,
> this would make the code do a next_line all the time for all the
> include clauses.

I have been waiting for your reply for some time, so I have taken some
to look at this patch by myself and hacked on it.

At the end, the thing I was not really happy about is the
MemoryContext used to store the set of TokenizedAuthLines. I have
looked at a couple of approaches, like passing around the context as
you do, but at the end there is something I found annoying: we may
tokenize a file in the line context of a different file, storing in
much more data than just the TokenizedAuthLines. Then I got a few
steps back and began using a static memory context that only stored
the TokenizedAuthLines, switching to it in one place as of the end of
tokenize_auth_file() when inserting an item. This makes hbafuncs.c a
bit less aware of the memory context, but I think that we could live
with that with a cleaner tokenization interface. That's close to what
GUCs do, in some way. Note that this may be better as an independent
patch, actually, as it impacts the current @ inclusions.

I have noticed a bug in the logic of include_if_exists: we should
ignore only files on ENOENT, but complain for other errnos after
opening the file.

The docs and the sample files have been tweaked a bit, giving a
cleaner separation between the main record types and the inclusion
ones.

+ /* XXX: this should stick to elevel for some cases? */
+ ereport(LOG,
+ (errmsg("skipping missing authentication file \"%s\"",
+ inc_fullname)));
Should we always issue a LOG here? In some cases we use an elevel of
DEBUG3.

So, what do you think about something like the attached? I have begun
a lookup at the tests, but I don't have enough material for an actual
review of this part yet. Note that I have removed temporarily
process_included_authfile(), as I was looking at all the code branches
in details. The final result ought to include AbsoluteConfigLocation(),
open_auth_file() and tokenize_auth_file() with an extra missing_ok
argument, I guess ("strict" as proposed originally is not the usual
PG-way for ENOENT-ish problems). process_line should be used only
when we have no err_msg, meaning that these have been consumed in some
TokenizedAuthLines already.
--
Michael