On Mon, Feb 28, 2022 at 07:42:17PM +0800, Julien Rouhaud wrote:
> Done in attached v2. I did the split in a separate commit, as the diff is
> otherwise unreadable. While at it I also fixed a few minor issues (I missed a
> MemoryContextDelete, and now avoid relying on inet_net_pton which apparently
> doesn't exist in cygwin).
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? 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.
+#include "utils/guc.h"
+//#include "utils/tuplestore.h"
Ditto.
+ /* 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 :)
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? 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.
Also, it seems to me that we'd better have some TAP tests for that to
make sure of its contents? One place would be src/test/auth/.
Another place where we make use of user mapping is the krb5 tests but
these are run in a limited fashion in the buildfarm. We also set some
mappings for SSPI on Windows all the time, so we'd better be careful
about that and paint some $windows_os in the tests when looking at the
output of the view.
+-- 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? Does this pass
on Windows where pg_regress sets some mappings for SSPI when creating
one or more roles?
--
Michael
