Re: Allow file inclusion in pg_hba and pg_ident files

Hi,

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?

> EXEC_BACKEND would reload an entire new thing for
> each connection, hence we need some loops to go through that.
> 2) How to check the contents of pg_hba_file_rules and
> pg_ident_file_mappings?
>
> There is a dependency between 1) and 2) once we try to check for error
> patterns in pg_hba_file_rules, because connections would just not
> happen. This is not the case for pg_ident_file_mappings though, so we
> could still test for buggy patterns in pg_ident.conf (or any of its
> included parts) with some expected content of
> pg_ident_file_mappings.error after a successful connection.
>
> Hmm. And what if we just gave up on the checks for error patterns in
> pg_hba_file_rules?

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.

> One thing that we could do for this part is to
> include all the buggy patterns we want to check at once in pg_hba.conf
> in its included portions, then scan for all the logs produced after
> attempting to start a server as the loading of pg_hba.conf would
> produce one LOG line with its CONTEXT for each buggy entry. The patch
> checks for error patterns with generate_log_err_rows(), but it looks
> like it would make the part 3 of the new test cleaner and easier to
> maintain in the long-term.

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.

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.

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.

[1] https://www.postgresql.org/message-id/YtZLeJPlMutL9heh%40paquier.xyz