Re: Allow file inclusion in pg_hba and pg_ident files

Hi,

On Mon, Jul 11, 2022 at 10:16:44AM +0900, Michael Paquier wrote:
> On Fri, Jul 08, 2022 at 02:57:21PM +0800, Julien Rouhaud wrote:
>
> My apologies for the late reply.
>
> > I don't have an extensive knowledge of all the user facing error messages, but
> > after a quick grep I see multiple usage of OID, PID, GIN and other defined
> > acronyms. I also see multiple occurrences of "only heap AM is supported",
> > while AM isn't even a defined acronym.
>
> A lot depends on the context of the code, it seems.
>
> > It doesn't seem that my proposal would be inconsistent with existing messages
> > and will help to reduce the message length, but if you prefer to keep the full
> > name I'm fine with it. Those should be very rare and specialized errors
> > anyway.
>
> So you mean to use "HBA file" instead of pg_hba.conf and
> "authentication file" when it can be either one of an HBA file or a
> mapping file? That would be okay by me.

Yes, it seems to me like a good compromise for not being overly verbose and
still being understandable.

> We would have a full cycle
> to tune them depending on the feedback we'd get afterwards.

Agreed.

> > While on the bikeshedding part, are you ok with the proposed keywords (include
> > and include_dir), behaving exactly like for postgresql.conf, and to also add
> > include_if_exists, so that we have the exact same possibilities with
> > postgresql.conf, pg_hba.conf and pg_ident.conf?
>
> Okay, agreed for consistency. With include_dir being careful about
> the ordering of the entries and ignoring anything else than a .conf
> file (that's something you mentioned already upthread).

Ok! All those that should be covered by new regression test so it should be
clear what is being implemented.

While on the regression tests topic, I started to implement those and faced
some problems quite fast when trying to workaround the problem we previously
discussed (1).

So first, even if we can test 99% of the features with just testing the views
output, I think it's should use the TAP framework since the tests will have to
mess with the pg_ident/pg_hba files. It's way easier to modify the auth files,
and it uses a dedicated instance so we don't have to worry about breaking other
test that would run concurrently.

Also, if we want to test the views error reporting we have to use a persistent
connection (as in interactive_psql()), otherwise tests will immediately fail on
Windows / EXEC_BACKEND builds. Adding the ability to run queries and wait for
completion on top of interactive_psql() doesn't seem to cause any problem, but
interpreting the results does.

Since it's just manipulating the psql's stdin/stdout, we retrieve the prompt
and executed query too. So for instance, if you just want to test "SELECT 1",
you will have to cleanup something like

dbname=# SELECT 1;
1
dbname#

That may still be workable by splitting the output per newline (and possibly
removing the first prompt before sending the query text), and remove the first
and last entry (assuming you want to test somewhat sane data, and not e.g. run
the regression test on a database containing a newline), but then you have to
also account for possible escape sequences, for instance if you use
enable-bracketed-paste. In that case, the output becomes something like

dbname=# SELECT 1;
[?2004l
1
[?2004hpostgres=#

It could probably be handled with some regexp to remove escape sequences and
remove empty lines, but it seems like really fragile, and thus a very bad idea.

I'm not really sure what should be done here. The best compromise I can think
of is to split the tests in 3 parts:

1) view reporting with various inclusions using safe_psql()
2) log error reporting
3) view reporting with various inclusions errors, using safe_psql()

And when testing 3), detect first if we can still connect after introducing
errors. If not, assume this is Windows / EXEC_BACKEND and give up here without
reporting an error. Otherwise continue, and fail the test if we later can't
connect anymore. As discussed previously, detecting that the build is using
the fork emulation code path doesn't seem worthwhile so guessing it from the
psql error may be a better approach.

Do you have any better idea, or do you have comments on this approach?

[1]: https://www.postgresql.org/message-id/YkFhpydhyeNNo3Xl@paquier.xyz