Re: SYSTEM_USER reserved word implementation

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