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