Re: [PATCH] Expose port->authn_id to extensions and triggers

Hi,

On 8/12/22 12:28 AM, Jacob Champion wrote:
> On Wed, Aug 10, 2022 at 10:48 PM Drouvot, Bertrand <bdrouvot@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