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

Hi,

On Tue, Mar 01, 2022 at 04:45:48PM +0900, Michael Paquier wrote:
>
> Hmm.  The diffs of 0001 are really hard to read.  Do you know why this
> is happening?  Is that because some code has been moved around?

Yes, I followed the file convention to put the static functions first and then
the exposed functions, and git-diff makes a terrible mess out of it :(

> I
> have been doing a comparison of all the routines showing up in the
> diffs, to note that the contents of load_hba(), load_ident(),
> hba_getauthmethod() & friends are actually the same, but this makes
> the change history harder to follow.  Moving around fill_hba_line()
> and fill_hba_view() should be enough, indeed.

There's no functional change apart from exposing some functions and moving some
in another file, so I though it's still ok to keep some consistency.  There
isn't much changes backpatched in that file, so it shouldn't create more
maintenance burden than simply splitting the file.

If you prefer to interleave static and non static function I can change it.

> +#include "utils/guc.h"
> +//#include "utils/tuplestore.h"

Yes I noticed this one this morning.  I didn't want to send a new patch version
just for that, but I already fixed it locally.

> +   /* Build a tuple descriptor for our result type */
> +   if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
> +       elog(ERROR, "return type must be a row type");
> 
> Worth noting that I was planning to apply a patch from Melanie
> Plageman to simplify the creation of tupledesc and tuplestores for
> set-returning functions like this one, so this would cut a bit of code
> here.  This is not directly related to your patch, though, that's my
> business :)

Yes I'm aware of that thread.  I will be happy to change the patch to use
MakeFuncResultTuplestore() as soon as it lands.  Thanks for the notice though.

> Well, as of 0002, one thing that makes things harder to follow is that
> parse_ident_line() is moved at a different place in hba.c, one slight
> difference being err_msg to store the error message in the token
> line..  But shouldn't the extension of parse_ident_line() with its
> elevel be included in 0001?

No, because 0001 is unrelated.  The changes you mentioned (exposing the
function and adding the error reporting) are only needed for the new view
introduced in 0002, thus not part of 0001.

> Or, well, it could just be done in its own patch to make for a cleaner
> history, so as 0002 could be shaped as two commits itself.

It seems strange to me to add a commit (or include in a commit) just to make a
single function exposed while nothing needs it, but I can do it this way if you
prefer.

> Also, it seems to me that we'd better have some TAP tests for that to
> make sure of its contents?

As I mentioned in my initial email, I intentionally didn't add any test in the
patchset yet, except the exact same coverage for the new view as there's for
pg_hba_file_rules.  Ideally I'd like to add tests only once, to cover both 002
and 0003.  But I don't want to waste time for that right now, especially since
no one seems to be interested in 0003.

> +-- We expect no user mapping in this test
> +select count(*) = 0 as ok from pg_ident_file_mappings;
> 
> It could be possible to do installcheck on an instance that has user
> mappings meaning that this had better be ">= 0", no?

I thought about it, and supposed it would bring a bit more value with the test
like that.  I can change it if you prefer.

> Does this pass
> on Windows where pg_regress sets some mappings for SSPI when creating
> one or more roles?

According to CI and cfbot yes.  E.g.
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/37/3558.
Note that the failed runs are the warning I mentioned for mingw32 and the POC
0004, which is now fixed.