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