Re: SYSTEM_USER reserved word implementation

Hi,

On 8/26/22 3:02 AM, Michael Paquier wrote:
> On Thu, Aug 25, 2022 at 08:21:05PM +0200, Drouvot, Bertrand wrote:
>> system_user() now returns a text and I moved it to miscinit.c in the new
>> version attached (I think it makes more sense now).
> +/* kluge to avoid including libpq/libpq-be.h here */
> +struct ClientConnectionInfo;
> +extern void InitializeSystemUser(struct ClientConnectionInfo conninfo);
> +extern const char* GetSystemUser(void);
>
> 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.

Agree that the extra SystemUser is not needed strictly speaking and that 
we could build it each time the system_user function is called.

>    True that this approach saves some extra palloc() calls each time
> the function is called.

Right, with the current approach the SystemUser just needs to be 
constructed one time.

I also think that it's more consistent to have such a global variable 
with his friends SessionUserId/OuterUserId/CurrentUserId (but at an 
extra memory cost in TopMemoryContext).

Looks like there is pros and cons for both approach.

I'm +1 for the current approach but I don't have a strong opinion about 
it so I'm also ok to change it the way you described if you think it's 
better.

>> New version attached is also addressing Michael's remark regarding the peer
>> authentication TAP test.
> Thanks.  I've wanted some basic tests for the peer authentication for
> some time now, independently on this thread, so it would make sense to
> split that into a first patch and stress the buildfarm to see what
> happens, then add these tests for SYSTEM_USER on top of the new test.

Makes fully sense, I've created a new thread [1] for this purpose, thanks!

For the moment I'm keeping the peer TAP test as it is in the current 
thread so that we can test the SYSTEM_USER behavior.

I just realized that the previous patch version contained useless change 
in name.c: attached a new version so that name.c now remains untouched.


[1]: 
https://www.postgresql.org/message-id/flat/aa60994b-1c66-ca7a-dab9-9a200dbac3d2%40amazon.com

Regards,

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