Hi,
On 9/7/22 10:48 AM, Michael Paquier wrote:
On Fri, Aug 26, 2022 at 10:02:26AM +0900, Michael Paquier wrote:
FWIW, I was also wondering about the need for all this initialization
stanza and the extra SystemUser in TopMemoryContext. Now that we have
MyClientConnectionInfo, I was thinking to just build the string in the
SQL function as that's the only code path that needs to know about
it. True that this approach saves some extra palloc() calls each time
the function is called.
At the end, fine by me to keep this approach as that's more
consistent. I have reviewed the patch,
Thanks for looking at it!
and a few things caught my
attention:
- I think that we'd better switch InitializeSystemUser() to have two
const char * as arguments for authn_id and an auth_method, so as there
is no need to use tweaks with UserAuth or ClientConnectionInfo in
miscadmin.h to bypass an inclusion of libpq-be.h or hba.h.
Good point, thanks! And there is no need to pass the whole ClientConnectionInfo (should we add more fields to it in the future).
- The OID of the new function should be in the range 8000-9999, as
taught by unused_oids.
Thanks for pointing out!
My reasoning was to use one available OID close to the ones used for session_user and current_user.
- Environments where the code is built without krb5 support would skip
the test where SYSTEM_USER should be not NULL when authenticated, so I
have added a test for that with MD5 in src/test/authentication/.
Good point, thanks for the new test (as that would also not be tested (once added) in the new peer TAP test [1] for platforms where peer authentication is not supported).
- Docs have been reworded, and I have applied an indentation.
Thanks, looks good to me.
- No need to use 200k rows in the table used to force the parallel
scan, as long as the costs are set.
Right.
It is a bit late here, so I may have missed something. For now, how
does the attached look to you?
+# Test SYSTEM_USER <> NULL with parallel workers.
Nit: What about "Test SYSTEM_USER get the correct value with parallel workers" as that's what we are actually testing.
Except the Nit above, that looks all good to me.
[1]: https://commitfest.postgresql.org/39/3845/
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
