Re: Allow file inclusion in pg_hba and pg_ident files

Hi,

Sorry for the very late answer, I had quite a lot of other things going on
recently. And thanks for taking care of the patchset!

On Wed, Nov 23, 2022 at 03:05:18PM +0900, Michael Paquier wrote:
> On Tue, Nov 22, 2022 at 05:20:01PM +0900, Michael Paquier wrote:
> > + /* 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.
>
> And here I think that we need to use elevel. In hba.c, this would
> generate a LOG while hbafuncs.c uses DEBUG3, leading to less noise in
> the log files.

Yeah I agree the LOG message is only interesting if you're reloading the conf.
I actually thought that this is what I did sorry about that.

> > 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.
>
> This, however, was still brittle in terms of memory handling.
> Reloading the server a few hundred times proved that this was leaking
> memory in the tokenization.

Oh, nice catch.

> This becomes quite simple once you switch
> to an approach where tokenize_auth_file() uses its own memory context,
> while we store all the TokenizedAuthLines in the static context. It
> also occurred to me that we can create and drop the static context
> holding the tokens when opening the top-root auth file, aka with a
> depth at 0. It may be a bit simpler to switch to a single-context
> approach for all the allocations of tokenize_auth_file(), though I
> find the use of a static context much cleaner for the inclusions with
> @ files when we expand an existing list.

Agreed.

The depth 0 is getting used quite a lot now, maybe we should have a define for
it to make it easier to grep, like TOP_LEVEL_AUTH_FILE or something like that?
And also add a define for the magical 10 for the max inclusion depth, for both
auth files and GUC files while at it?

> The patch can be split, again, into more pieces. Attached 0001
> reworks the memory allocation contexts for the tokenization and 0002
> is the main feature. As of things are, I am quite happy with the
> shapes of 0001 and 0002. I have tested quite a bit its robustness,
> with valgrind for example, to make sure that there are no leaks in the
> postmaster (with[out] -DEXEC_BACKEND). The inclusion logic is
> refactored to be in a state close to your previous patches, see
> tokenize_inclusion_file().

Yep I saw that. I don't have much to say about the patch, it looks good to me.
The only nitpicking I have is:

+/*
+ * Memory context holding the list of TokenizedAuthLines when parsing
+ * HBA or ident config files. This is created when loading the top HBA
+ + or ident file (depth of 0).
+ */
+static MemoryContext tokenize_context = NULL;

The comment seems a bit ambiguous as with "loading the top..." you probably
meant something like "loading the file in memory" rather than "(re)loading the
configuration". Maybe s/loading/opening/?
>
> Note that the tests are failing for some of the Windows CIs, actually,
> due to a difference in some of the paths generated vs the file paths
> (aka c:\cirrus vs c:/cirrus, as far as I saw).

Mmm, I haven't looked deeply so I'm not sure if the perl podules are aware of
it or not, but maybe we could somehow detect the used delimiter at the
beginning after normalizing the directory, and use a $DELIM rather than a plain
"/"?