Re: Allow file inclusion in pg_hba and pg_ident files

On Thu, Oct 27, 2022 at 12:26:25PM +0800, Julien Rouhaud wrote:
> On Thu, Oct 27, 2022 at 12:08:31PM +0900, Michael Paquier wrote:
>>
>> Putting things afresh, there are two different things here (sorry I
>> need to see that typed ;p):
>> 1) How do we want to check reliably the loading of the HBA and ident
>> files on errors?
>
> I guess you meant the failure to load HBA / ident files containing invalid
> data?

Yeah.

>> Hmm. And what if we just gave up on the checks for error patterns in
>> pg_hba_file_rules?

One part that I was thinking about when typing this part yesterday is
that an EXEC_BACKEND build should work in non-WIN32 in TAP even if
pg_ident.conf cannot be loaded, but I forgot entirely about the part
where we need a user mapping for the SSPI authentication on WIN32, as
set by pg_regress.

> We discussed this problem in the past (1), and my understanding was that
> detecting a -DEXEC_BACKEND/Win32 build and skipping those tests in that case
> would be an acceptable solution to make sure there's at least some coverage.
> The proposed patch adds such an approach, making sure that the failure is due
> to an invalid HBA file. If you changed you mind I can remove that part, but
> again I'd like to be sure of what you exactly want before starting to rewrite
> stuff.

I am still not completely sure what's the best way to do things here,
so let's do the following: let's keep the patch the way you think is
better for now (I may change my opinion on that but I'll hack that by
myself anyway). Using what you have as a base, could you split the
test and have it in its simplest to ease irs review? It would be able
to stress the buildfarm with a first version of the test and see how
it goes from there, and it is useful by itself IMO as HEAD has zero
coverage for this area.

> I'm not sure what you mean here. The patch does check for all the errors
> looking at LOG lines and CONTEXT lines, but to make the regexp easier it
> doesn't try to make sure that each CONTEXT line is immediately following the
> expected LOG line.

Hmm. Perhaps we'd better make sure that the LOG/CONTEXT link is
checked? The context includes the line number while a generic
sentence, and the LOG provides all the details of the error
happening.

> That's why the errors are divided in 2 steps: a first step with a single error
> using some inclusion, so we can validate that the CONTEXT line is entirely
> correct (wrt. line number and such), and then every possible error pattern
> where we assume that the CONTEXT line are still following their LOG entry if
> they're found. It also has the knowledge of which errors adds a CONTEXT line
> and which don't. And that's done twice, for HBA and ident.

Okay, so you do check the relationship between both, after all.

> The part 3 is just concatenating everything back, for HBA and ident. So
> long-term maintenance shouldn't get any harder as there won't be any need for
> more steps. We can just keep appending stuff in the 2nd step and all the tests
> should run as expected.

Hmm. Okay.
--
Michael