Hi,

On 8/12/22 12:28 AM, Jacob Champion wrote:
On Wed, Aug 10, 2022 at 10:48 PM Drouvot, Bertrand <bdrou...@amazon.com> wrote:
What do you think about adding a second field in ClientConnectionInfo
for the auth method (as suggested by Michael upthread)?
Sure -- without a followup patch, it's not really tested, though.

v2 adjusts set_authn_id() to copy the auth_method over as well. It
"passes tests" but is otherwise unexercised.

Thank you!

To help with the testing I've just provided a new version (aka v2-0004-system_user-implementation.patch) of the SYSTEM_USER patch in [1] that can be applied on top of "v2-0001-Allow-parallel-workers-to-read-authn_id.patch".

But for this to work, the first comment below on your patch needs to be addressed.

Once the first comment is addressed and the new SYSTEM_USER patch applied (that adds new tap tests) then we can test the propagation to the parallel workers with:

make -C src/test/kerberos check PROVE_TESTS=t/001_auth.pl PROVE_FLAGS=-v

and

make -C src/test/authentication check PROVE_TESTS=t/001_password.pl PROVE_FLAGS=-v

Both are currently successful.

Regarding the comments on v2-0001-Allow-parallel-workers-to-read-authn_id.patch:

1)

This is the one to be applied before adding the new SYSTEM_USER one on top:

+typedef struct
+{
+       /*
+        * Authenticated identity.  The meaning of this identifier is dependent on

has to be replaced by:

+typedef struct ClientConnectionInfo
+{
+       /*
+        * Authenticated identity.  The meaning of this identifier is dependent on

2)

+        * Authenticated identity.  The meaning of this identifier is dependent on

There is one extra space before "The"

3)

+SerializeClientConnectionInfo(Size maxsize, char *start_address)
+{
+       /*
+        * First byte is an indication of whether or not authn_id has been set to
+        * non-NULL, to differentiate that case from the empty string.
+        */

is authn_id being an empty string possible?

4)

+ */
+
+ClientConnectionInfo MyClientConnectionInfo;
+
+/*
+ * Calculate the space needed to serialize MyClientConnectionInfo.
+ */
+Size
+EstimateClientConnectionInfoSpace(void)

From a coding style point of view, shouldn't "ClientConnectionInfo MyClientConnectionInfo;" be moved to the top of the file?


[1]: https://commitfest.postgresql.org/39/3703/

Regards,

--
Bertrand Drouvot
Amazon Web Services: https://aws.amazon.com



Reply via email to