Re: Allow file inclusion in pg_hba and pg_ident files - Mailing list pgsql-hackers

On Wed, Nov 02, 2022 at 04:46:48PM +0900, Michael Paquier wrote:
> On Fri, Oct 28, 2022 at 11:49:54AM +0800, Julien Rouhaud wrote:
> > To be honest I'd rather not to.  It's excessively annoying to work on those
> > tests (I spent multiple days trying to make it as clean and readable as
> > possible), and splitting it to only test the current infrastructure will need
> > some substantial efforts.
>
> Well, I'd really like to split things as much as possible, beginning
> with some solid basics before extending its capabilities.  This
> reduces the odds of introducing issues in-between, particularly in
> areas as sensible as authentication that involves not-yet-logged-in
> users.
>
> > But more importantly, the next commit that will add tests for file inclusion
> > will then be totally unmaintainable and unreadable, so that's IMO even worse.
> > I think it will probably either be the current file overwritten or a new one
> > written from scratch if some changes are done in the simplified test, and I'm
> > not volunteering to do that.
>
> Not sure about that either.

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.

> Anyway, the last patch posted on this thread does not apply and the CF
> bot fails, so it needed a rebase.  I have first noticed that your
> patch included some doc fixes independent of this thread, so I have
> applied that as e765028 and backpatched it down to 15.

Yes I saw that this morning, thanks!

> The TAP test
> needs to be renamed to 0004, and it was missing from meson.build,
> hence the CI was not testing it.

Ah right.  This is quite annoying that we have to explicitly name every single
test file there.  For the source code it's hard to not notice a missed file and
you will get an error in the CI, but a missed test is just entirely transparent
:(

> 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.

> As of HEAD, tokenize_inc_file() is the place where we handle a list of
> tokens included with an '@' file, appending the existing set of
> AuthTokens into the list we are building by grabbing a copy of these
> before deleting the line memory context.
>
> 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.

> 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.

> 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.

> Attached are two patches: the first one is a rebase of what you have
> posted, and the second one are some changes I did while playing with
> the logic.  In the second one, except for conffiles.{h.c}, the changes
> are just a POC but that's to show the areas that I am planning to
> rework, and your tests pass with it.  I still need to think about all
> that and reconsider the design of the interface that would fit with
> the tokenization of the inclusions, without that many subroutines to
> do the work as it makes the code harder to follow.  Well, that's to
> say that I am not staying idle :)

Thanks!