Re: Allow file inclusion in pg_hba and pg_ident files

On Wed, Nov 02, 2022 at 09:06:02PM +0800, Julien Rouhaud wrote:
> Maybe one alternative approach would be to keep modifying the current test, and
> only do the split when committing the first part? The split itself should be
> easy and mechanical: just remove everything unneeded (different file names
> including the file name argument from the add_(hba|ident)_line, inclusion
> directive and so on). Even if the diff is then difficult to read it's not
> really a problem as it should have already been reviewed.

I have not reviewed the test part much yet, TBH. The path
manipulations because of WIN32 are a bit annoying. I was wondering if
we could avoid all that by using the basenames, as one option.

>> I have spent some time starting at the logic today of the whole, and
>> GetConfFilesInDir() is really the first thing that stood out. I am
>> not sure that it makes much sense to keep that in guc-file.c, TBH,
>> once we feed it into hba.c. Perhaps we could put the refactored
>> routine (and AbsoluteConfigLocation as a side effect) into a new file
>> in misc/?
>
> Yes I was wondering about it too. A new fine in misc/ looks sensible.

conffiles.c is the best thing I could come up with, conf.c and
config.c being too generic and these are routines that work on
configuration files, so..

>> Your patch proposes a different alternative, which is to pass down the
>> memory context created in tokenize_auth_file() down to the callers
>> with tokenize_file_with_context() dealing with all the internals.
>> process_included_auth_file() is different extension of that.
>> This may come down to a matter of taste, but could be be cleaner to
>> take an approach similar to tokenize_inc_file() and just create a copy
>> of the AuthToken list coming from a full file and append it to the
>> result rather than passing around the memory context for the lines?
>> This would lead to some simplifications, it seems, at least with the
>> number of arguments passed down across the layers.
>
> I guess it goes in the same direction as 8fea86830e1 where infrastructure to
> copy AuthTokens was added, I'm fine either way.

I won't hide that I am trying to make the changes a maximum
incremental for this thread so as the final part is easy-ish.

>> The addition of a check for the depth in two places seems unnecessary,
>> and it looks like we should do this kind of check in only one place.
>
> I usually prefer to add a maybe unnecessary depth check since it's basically
> free rather than risking an unfriendly stack size error (including in possible
> later refactoring), but no objection to get rid of it.

The depth check is a good idea, though I guess that there is an
argument for having it in only one code path, not two.

>> I have not tried yet, but if we actually move the AllocateFile() and
>> FreeFile() calls within tokenize_auth_file(), it looks like we may be
>> able to get to a simpler logic without the need of the with_context()
>> flavor (even no process_included_auth_file required)? That could make
>> the interface easier to follow as a whole, while limiting the presence
>> of AllocateFile() and FreeFile() to a single code path, impacting
>> open_inc_file() that relies on what the patch uses for
>> SecondaryAuthFile and IncludedAuthFile (we could attempt to use the
>> same error message everywhere as well, as one could expect that
>> expanded and included files have different names which is enough to
>> guess which one is an inclusion and which one is a secondary).
>
> You meant tokenize_auth_file_internal? Yes possibly, in general I tried
> to avoid changing too much the existing code to ease the patch acceptance, but
> I agree it would make things simpler.

I *guess* that my mind implied tokenize_auth_file() as of
yesterday's.
--
Michael