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
signature.asc
Description: PGP signature
