Обсуждение: SYSTEM_USER reserved word implementation

Hi hackers,

The SYSTEM_USER is a sql reserved word as mentioned in [1] and is currently not implemented.

Please find attached a patch proposal to make use of the SYSTEM_USER so that it returns the authenticated identity (if any) (aka authn_id in the Port struct).

Indeed in some circumstances, the authenticated identity is not the SESSION_USER and then the information is lost from the connection point of view (it could still be retrieved thanks to commit 9afffcb833 and log_connections set to on).

Example 1, using the gss authentification.

Say we have this entry in pg_hba.conf:

host all all 0.0.0.0/0 gss map=mygssmap

and the related mapping in pg_ident.conf

mygssmap   /^(.*@.*)\.LOCAL$    mary

Then, connecting with a valid Kerberos Ticket that contains “bertrand@BDTFOREST.LOCAL” as the default principal that way: psql -U mary -h myhostname -d postgres,

we will get:

postgres=> select current_user, session_user;
 current_user | session_user
--------------+--------------
 mary         | mary
(1 row)

While the SYSTEM_USER would produce the Kerberos principal:

postgres=> select system_user;
       system_user
--------------------------
 bertrand@BDTFOREST.LOCAL
(1 row)

Example 2, using the peer authentification.

Say we have this entry in pg_hba.conf:

local all john peer map=mypeermap

and the related mapping in pg_ident.conf

mypeermap postgres john

Then connected localy as the system user postgres and connecting to the database that way: psql -U john -d postgres, we will get:

postgres=> select current_user, session_user;
 current_user | session_user
--------------+--------------
 john         | john
(1 row)

While the SYSTEM_USER would produce the system user that requested the connection:

postgres=> select system_user;
 system_user
-------------
 postgres
(1 row)

Thanks to those examples we have seen some situations where the information related to the authenticated identity has been lost from the connection point of view (means not visible in the current_session or in the session_user).

The purpose of this patch is to make it visible through the SYSTEM_USER sql reserved word.

Remarks:

- In case port->authn_id is NULL then the patch is returning the SESSION_USER for the SYSTEM_USER. Perhaps it should return NULL instead.

- There is another thread [2] to expose port->authn_id to extensions and triggers thanks to a new API. This thread [2] leads to discussions about providing this information to the parallel workers too. While the new MyClientConnectionInfo being discussed in [2] could be useful to hold the client information that needs to be shared between the backend and any parallel workers, it does not seem to be needed in the case port->authn_id is exposed through SYSTEM_USER (like it is not for CURRENT_USER and SESSION_USER).

I will add this patch to the next commitfest.
I look forward to your feedback.

Bertrand

[1]: https://www.postgresql.org/docs/current/sql-keywords-appendix.html
[2]: https://www.postgresql.org/message-id/flat/793d990837ae5c06a558d58d62de9378ab525d83.camel%40vmware.com

"Drouvot, Bertrand" <bdrouvot@amazon.com> writes:
> Please find attached a patch proposal to make use of the SYSTEM_USER so 
> that it returns the authenticated identity (if any) (aka authn_id in the 
> Port struct).

On what grounds do you argue that that's the appropriate meaning of
SYSTEM_USER?

            regards, tom lane

On 6/22/22 09:49, Tom Lane wrote:
> "Drouvot, Bertrand" <bdrouvot@amazon.com> writes:
>> Please find attached a patch proposal to make use of the SYSTEM_USER so 
>> that it returns the authenticated identity (if any) (aka authn_id in the 
>> Port struct).
> 
> On what grounds do you argue that that's the appropriate meaning of
> SYSTEM_USER?


What else do you imagine it might mean?

Here is SQL Server interpretation for example:

https://docs.microsoft.com/en-us/sql/t-sql/functions/system-user-transact-sql?view=sql-server-ver16

And Oracle:
http://luna-ext.di.fc.ul.pt/oracle11g/timesten.112/e13070/ttsql257.htm#i1120532

   "SYSTEM_USER

   Returns the name of the current data store user
   as identified by the operating system."


Seems equivalent.


-- 
Joe Conway
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Joe Conway <mail@joeconway.com> writes:
> On 6/22/22 09:49, Tom Lane wrote:
>> On what grounds do you argue that that's the appropriate meaning of
>> SYSTEM_USER?

> What else do you imagine it might mean?

I was hoping for some citation of the SQL spec.

> Here is SQL Server interpretation for example:
>    "SYSTEM_USER
>    Returns the name of the current data store user
>    as identified by the operating system."

Meh.  That's as clear as mud.  (a) A big part of the question here
is what is meant by "current" user, in the face of operations like
SET ROLE.  (b) "as identified by the operating system" does more to
confuse me than anything else.  The operating system only deals in
OS user names; does that wording mean that what you get back is an OS
user name rather than a SQL role name?

My immediate guess would be that the SQL committee only intends
to deal in SQL role names and therefore SYSTEM_USER is defined
to return one of those, but I've not gone looking in the spec
to be sure.

I'm also not that clear on what we expect authn_id to be, but
a quick troll in the code makes it look like it's not necessarily
a SQL role name, but might be some external identifier such as a
Kerberos principal.  If that's the case I think it's going to be
inappropriate to use SQL-spec syntax to return it.  I don't object
to inventing some PG-specific function for the purpose, though.

BTW, are there any security concerns about exposing such identifiers?

            regards, tom lane

On 6/22/22 10:51, Tom Lane wrote:
> My immediate guess would be that the SQL committee only intends
> to deal in SQL role names and therefore SYSTEM_USER is defined
> to return one of those, but I've not gone looking in the spec
> to be sure.

I only have a draft copy, but in SQL 2016 I find relatively thin 
documentation for what SYSTEM_USER is supposed to represent:

   The value specified by SYSTEM_USER is equal to an
   implementation-defined string that represents the
   operating system user who executed the SQL-client
   module that contains the externally-invoked procedure
   whose execution caused the SYSTEM_USER <general value
   specification> to be evaluated.

> I'm also not that clear on what we expect authn_id to be, but
> a quick troll in the code makes it look like it's not necessarily
> a SQL role name, but might be some external identifier such as a
> Kerberos principal.  If that's the case I think it's going to be
> inappropriate to use SQL-spec syntax to return it.  I don't object
> to inventing some PG-specific function for the purpose, though.

To me the Kerberos principal makes perfect sense given the definition above.

> BTW, are there any security concerns about exposing such identifiers?

On the contrary, I would argue that not having the identifier for the 
external "user" available is a security concern. Ideally you want to be 
able to trace actions inside Postgres to the actual user that invoked them.

-- 
Joe Conway
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Joe Conway <mail@joeconway.com> writes:
> On 6/22/22 10:51, Tom Lane wrote:
>> My immediate guess would be that the SQL committee only intends
>> to deal in SQL role names and therefore SYSTEM_USER is defined
>> to return one of those, but I've not gone looking in the spec
>> to be sure.

> I only have a draft copy, but in SQL 2016 I find relatively thin 
> documentation for what SYSTEM_USER is supposed to represent:

>    The value specified by SYSTEM_USER is equal to an
>    implementation-defined string that represents the
>    operating system user who executed the SQL-client
>    module that contains the externally-invoked procedure
>    whose execution caused the SYSTEM_USER <general value
>    specification> to be evaluated.

Huh.  Okay, if it's implementation-defined then we can define it
as "whatever auth.c put into authn_id".  Objection withdrawn.

            regards, tom lane

On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
> On the contrary, I would argue that not having the identifier for the
> external "user" available is a security concern. Ideally you want to be
> able to trace actions inside Postgres to the actual user that invoked them.

If auditing is also the use case for SYSTEM_USER, you'll probably want
to review the arguments for making it available to parallel workers
that were made in the other thread [1].

Initial comments on the patch:

> In case port->authn_id is NULL then the patch is returning the SESSION_USER for the SYSTEM_USER. Perhaps it should
returnNULL instead.
 

If the spec says that SYSTEM_USER "represents the operating system
user", but we don't actually know who that user was (authn_id is
NULL), then I think SYSTEM_USER should also be NULL so as not to
mislead auditors.

> --- a/src/backend/utils/init/miscinit.c
> +++ b/src/backend/utils/init/miscinit.c
> @@ -473,6 +473,7 @@ static Oid    AuthenticatedUserId = InvalidOid;
>  static Oid    SessionUserId = InvalidOid;
>  static Oid    OuterUserId = InvalidOid;
>  static Oid    CurrentUserId = InvalidOid;
> +static const char *SystemUser = NULL;
>
>  /* We also have to remember the superuser state of some of these levels */
>  static bool AuthenticatedUserIsSuperuser = false;

What's the rationale for introducing a new global for this? A downside
is that now there are two sources of truth, for a security-critical
attribute of the connection.

--Jacob

[1] https://www.postgresql.org/message-id/flat/793d990837ae5c06a558d58d62de9378ab525d83.camel%40vmware.com

Jacob Champion <jchampion@timescale.com> writes:
> On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
>> In case port->authn_id is NULL then the patch is returning the SESSION_USER for the SYSTEM_USER. Perhaps it should
returnNULL instead. 

> If the spec says that SYSTEM_USER "represents the operating system
> user", but we don't actually know who that user was (authn_id is
> NULL), then I think SYSTEM_USER should also be NULL so as not to
> mislead auditors.

Yeah, that seems like a fundamental type mismatch.  If we don't know
the OS user identifier, substituting a SQL role name is surely not
the right thing.

I think a case could be made for ONLY returning non-null when authn_id
represents some externally-verified identifier (OS user ID gotten via
peer identification, Kerberos principal, etc).

            regards, tom lane

On 6/22/22 11:52, Tom Lane wrote:
> Jacob Champion <jchampion@timescale.com> writes:
>> On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
>>> In case port->authn_id is NULL then the patch is returning the SESSION_USER for the SYSTEM_USER. Perhaps it should
returnNULL instead.
 
> 
>> If the spec says that SYSTEM_USER "represents the operating system
>> user", but we don't actually know who that user was (authn_id is
>> NULL), then I think SYSTEM_USER should also be NULL so as not to
>> mislead auditors.
> 
> Yeah, that seems like a fundamental type mismatch.  If we don't know
> the OS user identifier, substituting a SQL role name is surely not
> the right thing.

+1 agreed

> I think a case could be made for ONLY returning non-null when authn_id
> represents some externally-verified identifier (OS user ID gotten via
> peer identification, Kerberos principal, etc).

But -1 on that.

I think any time we have a non-null authn_id we should expose it. Are 
there examples of cases when we have authn_id but for some reason don't 
trust the value of it?


-- 
Joe Conway
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On 6/22/22 11:35, Jacob Champion wrote:
> On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
>> --- a/src/backend/utils/init/miscinit.c
>> +++ b/src/backend/utils/init/miscinit.c
>> @@ -473,6 +473,7 @@ static Oid    AuthenticatedUserId = InvalidOid;
>>  static Oid    SessionUserId = InvalidOid;
>>  static Oid    OuterUserId = InvalidOid;
>>  static Oid    CurrentUserId = InvalidOid;
>> +static const char *SystemUser = NULL;
>>
>>  /* We also have to remember the superuser state of some of these levels */
>>  static bool AuthenticatedUserIsSuperuser = false;
> 
> What's the rationale for introducing a new global for this? A downside
> is that now there are two sources of truth, for a security-critical
> attribute of the connection.

Why would you want to do it differently than 
SessionUserId/OuterUserId/CurrentUserId? It is analogous, no?

-- 
Joe Conway
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Joe Conway <mail@joeconway.com> writes:
> On 6/22/22 11:52, Tom Lane wrote:
>> I think a case could be made for ONLY returning non-null when authn_id
>> represents some externally-verified identifier (OS user ID gotten via
>> peer identification, Kerberos principal, etc).

> But -1 on that.

> I think any time we have a non-null authn_id we should expose it. Are 
> there examples of cases when we have authn_id but for some reason don't 
> trust the value of it?

I'm more concerned about whether we have a consistent story about what
SYSTEM_USER means (another way of saying "what type is it").  If it's
just the same as SESSION_USER it doesn't seem like we've added much.

Maybe, instead of just being the raw user identifier, it should be
something like "auth_method:user_identifier" so that one can tell
what the identifier actually is and how it was verified.

            regards, tom lane

On 6/22/22 12:28, Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>> On 6/22/22 11:52, Tom Lane wrote:
>>> I think a case could be made for ONLY returning non-null when authn_id
>>> represents some externally-verified identifier (OS user ID gotten via
>>> peer identification, Kerberos principal, etc).
> 
>> But -1 on that.
> 
>> I think any time we have a non-null authn_id we should expose it. Are 
>> there examples of cases when we have authn_id but for some reason don't 
>> trust the value of it?
> 
> I'm more concerned about whether we have a consistent story about what
> SYSTEM_USER means (another way of saying "what type is it").  If it's
> just the same as SESSION_USER it doesn't seem like we've added much.
> 
> Maybe, instead of just being the raw user identifier, it should be
> something like "auth_method:user_identifier" so that one can tell
> what the identifier actually is and how it was verified.

Oh, that's an interesting thought -- I like that.

-- 
Joe Conway
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Jun 22, 2022 at 9:26 AM Joe Conway <mail@joeconway.com> wrote:
> On 6/22/22 11:35, Jacob Champion wrote:
> > On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
> Why would you want to do it differently than
> SessionUserId/OuterUserId/CurrentUserId? It is analogous, no?

Like I said, now there are two different sources of truth, and
additional code to sync the two, and two different APIs to set what
should be a single write-once attribute. But if SystemUser is instead
derived from authn_id, like what's just been proposed with
`method:authn_id`, I think there's a better argument for separating
the two.

--Jacob

Hi,

On 6/22/22 6:32 PM, Joe Conway wrote:
> CAUTION: This email originated from outside of the organization. Do 
> not click links or open attachments unless you can confirm the sender 
> and know the content is safe.
>
>
>
> On 6/22/22 12:28, Tom Lane wrote:
>> Joe Conway <mail@joeconway.com> writes:
>>> On 6/22/22 11:52, Tom Lane wrote:
>>>> I think a case could be made for ONLY returning non-null when authn_id
>>>> represents some externally-verified identifier (OS user ID gotten via
>>>> peer identification, Kerberos principal, etc).
>>
>>> But -1 on that.
>>
>>> I think any time we have a non-null authn_id we should expose it. Are
>>> there examples of cases when we have authn_id but for some reason don't
>>> trust the value of it?
>>
>> I'm more concerned about whether we have a consistent story about what
>> SYSTEM_USER means (another way of saying "what type is it").  If it's
>> just the same as SESSION_USER it doesn't seem like we've added much.
>>
>> Maybe, instead of just being the raw user identifier, it should be
>> something like "auth_method:user_identifier" so that one can tell
>> what the identifier actually is and how it was verified.
>
> Oh, that's an interesting thought -- I like that.
>
Thanks Joe and Tom for your feedback.

I like this idea too and that's also more aligned with what 
log_connections set to on would report (aka the auth method).

Baring any objections, I'll work on that idea.

Bertrand

Hi,

On 6/22/22 5:35 PM, Jacob Champion wrote:
> On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
>> On the contrary, I would argue that not having the identifier for the
>> external "user" available is a security concern. Ideally you want to be
>> able to trace actions inside Postgres to the actual user that invoked them.
> If auditing is also the use case for SYSTEM_USER, you'll probably want
> to review the arguments for making it available to parallel workers
> that were made in the other thread [1].

Thanks Jacob for your feedback.

I did some testing initially around the parallel workers and did not see 
any issues at that time.

I just had another look and I agree that the parallel workers case needs 
to be addressed.

I'll have a closer look to what you have done in [1].

Thanks

Bertrand

[1]https://www.postgresql.org/message-id/flat/793d990837ae5c06a558d58d62de9378ab525d83.camel%40vmware.com

Hi,

On 6/23/22 10:06 AM, Drouvot, Bertrand wrote:
> Hi,
>
> On 6/22/22 5:35 PM, Jacob Champion wrote:
>> On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
>>> On the contrary, I would argue that not having the identifier for the
>>> external "user" available is a security concern. Ideally you want to be
>>> able to trace actions inside Postgres to the actual user that 
>>> invoked them.
>> If auditing is also the use case for SYSTEM_USER, you'll probably want
>> to review the arguments for making it available to parallel workers
>> that were made in the other thread [1].
>
> Thanks Jacob for your feedback.
>
> I did some testing initially around the parallel workers and did not 
> see any issues at that time.
>
> I just had another look and I agree that the parallel workers case 
> needs to be addressed.
>
> I'll have a closer look to what you have done in [1].
>
> Thanks
>
> Bertrand
>
Please find attached patch version 2.

It does contain:

- Tom's idea implementation (aka presenting the system_user as 
auth_method:authn_id)

- A fix for the parallel workers issue mentioned by Jacob. The patch now 
propagates the SYSTEM_USER to the parallel workers.

- Doc updates

- Tap tests (some of them are coming from [1])

Looking forward to your feedback,

Thanks

Bertrand

[1] 
https://www.postgresql.org/message-id/flat/793d990837ae5c06a558d58d62de9378ab525d83.camel%40vmware.com

On 6/24/22 11:49 AM, Drouvot, Bertrand wrote:
> Hi,
>
> On 6/23/22 10:06 AM, Drouvot, Bertrand wrote:
>> Hi,
>>
>> On 6/22/22 5:35 PM, Jacob Champion wrote:
>>> On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
>>>> On the contrary, I would argue that not having the identifier for the
>>>> external "user" available is a security concern. Ideally you want 
>>>> to be
>>>> able to trace actions inside Postgres to the actual user that 
>>>> invoked them.
>>> If auditing is also the use case for SYSTEM_USER, you'll probably want
>>> to review the arguments for making it available to parallel workers
>>> that were made in the other thread [1].
>>
>> Thanks Jacob for your feedback.
>>
>> I did some testing initially around the parallel workers and did not 
>> see any issues at that time.
>>
>> I just had another look and I agree that the parallel workers case 
>> needs to be addressed.
>>
>> I'll have a closer look to what you have done in [1].
>>
>> Thanks
>>
>> Bertrand
>>
> Please find attached patch version 2.
>
> It does contain:
>
> - Tom's idea implementation (aka presenting the system_user as 
> auth_method:authn_id)
>
> - A fix for the parallel workers issue mentioned by Jacob. The patch 
> now propagates the SYSTEM_USER to the parallel workers.
>
> - Doc updates
>
> - Tap tests (some of them are coming from [1])
>
> Looking forward to your feedback,
>
> Thanks
>
> Bertrand

FWIW here is a link to the commitfest entry: 
https://commitfest.postgresql.org/38/3703/

Bertrand

Hi,

On 6/24/22 2:47 PM, Drouvot, Bertrand wrote:
>
> On 6/24/22 11:49 AM, Drouvot, Bertrand wrote:
>> Hi,
>>
>> On 6/23/22 10:06 AM, Drouvot, Bertrand wrote:
>>> Hi,
>>>
>>> On 6/22/22 5:35 PM, Jacob Champion wrote:
>>>> On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
>>>>> On the contrary, I would argue that not having the identifier for the
>>>>> external "user" available is a security concern. Ideally you want 
>>>>> to be
>>>>> able to trace actions inside Postgres to the actual user that 
>>>>> invoked them.
>>>> If auditing is also the use case for SYSTEM_USER, you'll probably want
>>>> to review the arguments for making it available to parallel workers
>>>> that were made in the other thread [1].
>>>
>>> Thanks Jacob for your feedback.
>>>
>>> I did some testing initially around the parallel workers and did not 
>>> see any issues at that time.
>>>
>>> I just had another look and I agree that the parallel workers case 
>>> needs to be addressed.
>>>
>>> I'll have a closer look to what you have done in [1].
>>>
>>> Thanks
>>>
>>> Bertrand
>>>
>> Please find attached patch version 2.
>>
>> It does contain:
>>
>> - Tom's idea implementation (aka presenting the system_user as 
>> auth_method:authn_id)
>>
>> - A fix for the parallel workers issue mentioned by Jacob. The patch 
>> now propagates the SYSTEM_USER to the parallel workers.
>>
>> - Doc updates
>>
>> - Tap tests (some of them are coming from [1])
>>
>> Looking forward to your feedback,
>>
>> Thanks
>>
>> Bertrand
>
> FWIW here is a link to the commitfest entry: 
> https://commitfest.postgresql.org/38/3703/
>
> Bertrand
>
Attached a tiny rebase to make the CF bot CompilerWarnings happy.

Bertrand

On 2022-Jun-25, Drouvot, Bertrand wrote:

> diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
> index 0af130fbc5..8d761512fd 100644
> --- a/src/include/miscadmin.h
> +++ b/src/include/miscadmin.h
> @@ -364,6 +364,10 @@ extern void InitializeSessionUserIdStandalone(void);
>  extern void SetSessionAuthorization(Oid userid, bool is_superuser);
>  extern Oid    GetCurrentRoleId(void);
>  extern void SetCurrentRoleId(Oid roleid, bool is_superuser);
> +/* kluge to avoid including libpq/libpq-be.h here */
> +typedef struct Port MyPort;
> +extern void InitializeSystemUser(const MyPort *port);
> +extern const char* GetSystemUser(void);

This typedef here looks quite suspicious.  I think this should suffice:

+/* kluge to avoid including libpq/libpq-be.h here */
+struct Port;
+extern void InitializeSystemUser(struct Port *port);

I suspect that having a typedef called MyPort is going to wreak serious
havoc for pgindent.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Hi,

On 6/27/22 7:32 PM, Alvaro Herrera wrote:
> On 2022-Jun-25, Drouvot, Bertrand wrote:
>
>> diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
>> index 0af130fbc5..8d761512fd 100644
>> --- a/src/include/miscadmin.h
>> +++ b/src/include/miscadmin.h
>> @@ -364,6 +364,10 @@ extern void InitializeSessionUserIdStandalone(void);
>>   extern void SetSessionAuthorization(Oid userid, bool is_superuser);
>>   extern Oid   GetCurrentRoleId(void);
>>   extern void SetCurrentRoleId(Oid roleid, bool is_superuser);
>> +/* kluge to avoid including libpq/libpq-be.h here */
>> +typedef struct Port MyPort;
>> +extern void InitializeSystemUser(const MyPort *port);
>> +extern const char* GetSystemUser(void);
> This typedef here looks quite suspicious.  I think this should suffice:
>
> +/* kluge to avoid including libpq/libpq-be.h here */
> +struct Port;
> +extern void InitializeSystemUser(struct Port *port);
>
> I suspect that having a typedef called MyPort is going to wreak serious
> havoc for pgindent.

Good catch, thanks!

Attached new version to fix it as suggested.

Regards,

Bertrand

Hi,

On 6/28/22 9:18 AM, Drouvot, Bertrand wrote:
>
> Attached new version to fix it as suggested.
>
Just to update current and new readers (if any) of this thread.

It has been agreed that the work on this patch is on hold until the 
ClientConnectionInfo related work is finished (see the discussion in [1]).

Having said that I'm attaching a new patch 
"v2-0004-system_user-implementation.patch" for the SYSTEM_USER.

This new patch currently does not apply on master (so the CF bot will 
fail and this is expected) but does currently apply on top of 
"v2-0001-Allow-parallel-workers-to-read-authn_id.patch" provided in [1].

The reason of it, is that it helps the testing for [1].


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

Regards,

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

On Fri, Aug 12, 2022 at 6:32 AM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
> It has been agreed that the work on this patch is on hold until the
> ClientConnectionInfo related work is finished (see the discussion in [1]).
>
> Having said that I'm attaching a new patch
> "v2-0004-system_user-implementation.patch" for the SYSTEM_USER.

(Not a full review.) Now that the implementation has increased in
complexity, the original tests for the parallel workers have become
underpowered. As a concrete example, I forgot to serialize auth_method
during my most recent rewrite, and the tests still passed.

I think it'd be better to check the contents of SYSTEM_USER, where we
can, rather than only testing for existence. Something like the
attached, maybe? And it would also be good to add a similar test to
the authentication suite, so that you don't have to have Kerberos
enabled to fully test SYSTEM_USER.

--Jacob

Hi,

On 8/16/22 6:52 PM, Jacob Champion wrote:
> On Fri, Aug 12, 2022 at 6:32 AM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>> It has been agreed that the work on this patch is on hold until the
>> ClientConnectionInfo related work is finished (see the discussion in [1]).
>>
>> Having said that I'm attaching a new patch
>> "v2-0004-system_user-implementation.patch" for the SYSTEM_USER.
> (Not a full review.) Now that the implementation has increased in
> complexity, the original tests for the parallel workers have become
> underpowered. As a concrete example, I forgot to serialize auth_method
> during my most recent rewrite, and the tests still passed.
>
> I think it'd be better to check the contents of SYSTEM_USER, where we
> can, rather than only testing for existence. Something like the
> attached, maybe?

Yeah, fully agree, thanks for pointing out!

I've included your suggestion in v2-0005 attached (it's expected to see 
the CF bot failing for the same reason as mentioned up-thread).

> And it would also be good to add a similar test to
> the authentication suite, so that you don't have to have Kerberos
> enabled to fully test SYSTEM_USER.

Agree, I'll look at what can be done here.

Regards,

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

Hi,

On 8/17/22 9:51 AM, Drouvot, Bertrand wrote:
> On 8/16/22 6:52 PM, Jacob Champion wrote:
>
>> And it would also be good to add a similar test to
>> the authentication suite, so that you don't have to have Kerberos
>> enabled to fully test SYSTEM_USER.
>
> Agree, I'll look at what can be done here.
>
I added authentication/t/003_peer.pl in 
v2-0006-system_user-implementation.patch attached.

It does the peer authentication and SYSTEM_USER testing with and without 
a user name map.

$ make -C src/test/authentication check PROVE_TESTS=t/003_peer.pl 
PROVE_FLAGS=-v

ok 1 - users with peer authentication have the correct SYSTEM_USER
ok 2 - parallel workers return the correct SYSTEM_USER when peer 
authentication is used
ok 3 - user name map is well defined and working
ok 4 - users with peer authentication and user name map have the correct 
SYSTEM_USER
ok 5 - parallel workers return the correct SYSTEM_USER when peer 
authentication and user name map is used
1..5
ok
All tests successful.

That way one could test the SYSTEM_USER behavior without the need to 
have kerberos enabled.

Regards,

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

On Wed, Aug 17, 2022 at 04:48:42PM +0200, Drouvot, Bertrand wrote:
> That way one could test the SYSTEM_USER behavior without the need to have
> kerberos enabled.

I was looking at this patch and noticed that SYSTEM_USER returns a
"name", meaning that the value would be automatically truncated at 63
characters.  We shouldn't imply that as authn_ids can be longer than
that, and this issue gets a bit worse once with the auth_method
appended to the string.

+if (!$use_unix_sockets)
+{
+   plan skip_all =>
+     "authentication tests cannot run without Unix-domain sockets";
+}

Are you sure that !$use_unix_sockets is safe here?  Could we have
platforms where we use our port's getpeereid() with $use_unix_sockets
works?  That would cause the test to fail with ENOSYS.  Hmm.  Without
being able to rely on HAVE_GETPEEREID, we could check for the error
generated when the fallback implementation does not work, and skip the
rest of the test.
--
Michael

Hi,

On 8/24/22 6:27 AM, Michael Paquier wrote:
> On Wed, Aug 17, 2022 at 04:48:42PM +0200, Drouvot, Bertrand wrote:
>> That way one could test the SYSTEM_USER behavior without the need to have
>> kerberos enabled.
> I was looking at this patch

Thanks for looking at it!

> and noticed that SYSTEM_USER returns a
> "name", meaning that the value would be automatically truncated at 63
> characters.  We shouldn't imply that as authn_ids can be longer than
> that, and this issue gets a bit worse once with the auth_method
> appended to the string.

Good catch! I'll fix that in the next version.

Hmm, I think it would make sense to keep system_user() with his friends 
current_user() and session_user().

But now that system_user() will not return a name anymore (but a text), 
I think name.c is no longer the right place, what do you think? (If so, 
where would you suggest?)

>
> +if (!$use_unix_sockets)
> +{
> +   plan skip_all =>
> +     "authentication tests cannot run without Unix-domain sockets";
> +}
>
> Are you sure that !$use_unix_sockets is safe here?  Could we have
> platforms where we use our port's getpeereid() with $use_unix_sockets
> works?  That would cause the test to fail with ENOSYS.  Hmm.  Without
> being able to rely on HAVE_GETPEEREID, we could check for the error
> generated when the fallback implementation does not work, and skip the
> rest of the test.

Oh right, I did not think about that, thanks for the suggestion.

I'll change this in the next version and simply skip the rest of the 
test in case we get "peer authentication is not supported on this platform".

Regards,

-- 

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

Hi,

On 8/24/22 8:26 PM, Drouvot, Bertrand wrote:
> Hi,
>
> On 8/24/22 6:27 AM, Michael Paquier wrote:
>> On Wed, Aug 17, 2022 at 04:48:42PM +0200, Drouvot, Bertrand wrote:
>>> That way one could test the SYSTEM_USER behavior without the need to 
>>> have
>>> kerberos enabled.
>> I was looking at this patch
>
> Thanks for looking at it!
>
>> and noticed that SYSTEM_USER returns a
>> "name", meaning that the value would be automatically truncated at 63
>> characters.  We shouldn't imply that as authn_ids can be longer than
>> that, and this issue gets a bit worse once with the auth_method
>> appended to the string.
>
> Good catch! I'll fix that in the next version.
>
> Hmm, I think it would make sense to keep system_user() with his 
> friends current_user() and session_user().
>
> But now that system_user() will not return a name anymore (but a 
> text), I think name.c is no longer the right place, what do you think? 
> (If so, where would you suggest?)

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).

>
>>
>> +if (!$use_unix_sockets)
>> +{
>> +   plan skip_all =>
>> +     "authentication tests cannot run without Unix-domain sockets";
>> +}
>>
>> Are you sure that !$use_unix_sockets is safe here?  Could we have
>> platforms where we use our port's getpeereid() with $use_unix_sockets
>> works?  That would cause the test to fail with ENOSYS.  Hmm. Without
>> being able to rely on HAVE_GETPEEREID, we could check for the error
>> generated when the fallback implementation does not work, and skip the
>> rest of the test.
>
> Oh right, I did not think about that, thanks for the suggestion.
>
> I'll change this in the next version and simply skip the rest of the 
> test in case we get "peer authentication is not supported on this 
> platform".
>
New version attached is also addressing Michael's remark regarding the 
peer authentication TAP test.

Regards,

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

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.  True that this approach saves some extra palloc() calls each time
the function is called.

> 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.
--
Michael

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

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, 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.
- The OID of the new function  should be in the range 8000-9999, as
taught by unused_oids.
- 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/.
- Docs have been reworded, and I have applied an indentation.
- No need to use 200k rows in the table used to force the parallel
scan, as long as the costs are set.

It is a bit late here, so I may have missed something.  For now, how
does the attached look to you?
--
Michael

Hi,

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.

- No need to use 200k rows in the table used to force the parallel
scan, as long as the costs are set.

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

On 9/7/22 07:46, Drouvot, Bertrand wrote:
> Except the Nit above, that looks all good to me.

A few additional comments:

> +        assigned a database role. It is represented as
> +        <literal>auth_method:identity</literal> or
> +        <literal>NULL</literal> if the user has not been authenticated (for
> +        example if <xref linkend="auth-trust"/> has been used).
> +       </para></entry>

This is rendered as

    ... (for example if Section 21.4 has been used).

which IMO isn't too helpful. Maybe a <link> would read better than an
<xref>?

Also, this function's placement in the docs (with the System Catalog
Information Functions) seems wrong to me. I think it should go up above
in the Session Information Functions, with current_user et al.

> +               /* Build system user as auth_method:authn_id */
> +               char       *system_user;
> +               Size            authname_len = strlen(auth_method);
> +               Size            authn_id_len = strlen(authn_id);
> +
> +               system_user = palloc0(authname_len + authn_id_len + 2);
> +               strcat(system_user, auth_method);
> +               strcat(system_user, ":");
> +               strcat(system_user, authn_id);

If we're palloc'ing anyway, can this be replaced with a single psprintf()?

> +       /* Initialize SystemUser now that MyClientConnectionInfo is restored. */
> +       InitializeSystemUser(MyClientConnectionInfo.authn_id,
> +                                                hba_authname(MyClientConnectionInfo.auth_method));

It makes me a little nervous to call hba_authname(auth_method) without
checking to see that auth_method is actually valid (which is only true
if authn_id is not NULL).

We could pass the bare auth_method index, or update the documentation
for auth_method to state that it's guaranteed to be zero if authn_id is
NULL (and then enforce that).

>         case SVFOP_CURRENT_USER:
>         case SVFOP_USER:
>         case SVFOP_SESSION_USER:
> +       case SVFOP_SYSTEM_USER:
>         case SVFOP_CURRENT_CATALOG:
>         case SVFOP_CURRENT_SCHEMA:
>             svf->type = NAMEOID;

Should this be moved to use TEXTOID instead?

Thanks,
--Jacob

On Wed, Sep 07, 2022 at 08:48:43AM -0700, Jacob Champion wrote:
> Also, this function's placement in the docs (with the System Catalog
> Information Functions) seems wrong to me. I think it should go up above
> in the Session Information Functions, with current_user et al.

Yeah, this had better use a <link>.

>> +       /* Initialize SystemUser now that MyClientConnectionInfo is restored. */
>> +       InitializeSystemUser(MyClientConnectionInfo.authn_id,
>> +                                                hba_authname(MyClientConnectionInfo.auth_method));
>
> It makes me a little nervous to call hba_authname(auth_method) without
> checking to see that auth_method is actually valid (which is only true
> if authn_id is not NULL).

You have mentioned that a couple of months ago if I recall correctly,
and we pass down an enum value.

> We could pass the bare auth_method index, or update the documentation
> for auth_method to state that it's guaranteed to be zero if authn_id is
> NULL (and then enforce that).
>
> >         case SVFOP_CURRENT_USER:
> >         case SVFOP_USER:
> >         case SVFOP_SESSION_USER:
> > +       case SVFOP_SYSTEM_USER:
> >         case SVFOP_CURRENT_CATALOG:
> >         case SVFOP_CURRENT_SCHEMA:
> >             svf->type = NAMEOID;
>
> Should this be moved to use TEXTOID instead?

Yeah, it should.  There is actually a second and much deeper issue
here, in the shape of a collation problem.  See the assertion failure
in exprSetCollation(), because we expect SQLValueFunction nodes to
return a name or a non-collatable type.  However, for this case, we'd
require a text to get rid of the 63-character limit, and that's
a collatable type.  This reminds me of the recent thread to work on
getting rid of this limit for the name type..
--
Michael

On Wed, Sep 7, 2022 at 6:17 PM Michael Paquier <michael@paquier.xyz> wrote:
> >> +       /* Initialize SystemUser now that MyClientConnectionInfo is restored. */
> >> +       InitializeSystemUser(MyClientConnectionInfo.authn_id,
> >> +                                                hba_authname(MyClientConnectionInfo.auth_method));
> >
> > It makes me a little nervous to call hba_authname(auth_method) without
> > checking to see that auth_method is actually valid (which is only true
> > if authn_id is not NULL).
>
> You have mentioned that a couple of months ago if I recall correctly,
> and we pass down an enum value.

Ah, sorry. Do you remember which thread?

I am probably misinterpreting you, but I don't see why auth_method's
being an enum helps. uaReject (and the "reject" string) is not a sane
value to be using in SYSTEM_USER, and the more call stacks away we get
from MyClientConnectionInfo, the easier it is to forget that that
value is junk. As long as the code doesn't get more complicated, I
suppose there's no real harm being done, but it'd be cleaner not to
access auth_method at all if authn_id is NULL. I won't die on that
hill, though.

> There is actually a second and much deeper issue
> here, in the shape of a collation problem.

Oh, none of that sounds fun. :/

--Jacob

Hi,

On 9/7/22 5:48 PM, Jacob Champion wrote:
> On 9/7/22 07:46, Drouvot, Bertrand wrote:
>> Except the Nit above, that looks all good to me.
> 
> A few additional comments:
> 
>> +        assigned a database role. It is represented as
>> +        <literal>auth_method:identity</literal> or
>> +        <literal>NULL</literal> if the user has not been authenticated (for
>> +        example if <xref linkend="auth-trust"/> has been used).
>> +       </para></entry>
> 
> This is rendered as
> 
>      ... (for example if Section 21.4 has been used).
> 
> which IMO isn't too helpful. Maybe a <link> would read better than an
> <xref>?

Thanks for looking at it!
Good catch, V4 coming soon will make use of <link> instead.

> 
> Also, this function's placement in the docs (with the System Catalog
> Information Functions) seems wrong to me. I think it should go up above
> in the Session Information Functions, with current_user et al.

Agree, will move it to the Session Information Functions in V4.

> 
>> +               /* Build system user as auth_method:authn_id */
>> +               char       *system_user;
>> +               Size            authname_len = strlen(auth_method);
>> +               Size            authn_id_len = strlen(authn_id);
>> +
>> +               system_user = palloc0(authname_len + authn_id_len + 2);
>> +               strcat(system_user, auth_method);
>> +               strcat(system_user, ":");
>> +               strcat(system_user, authn_id);
> 
> If we're palloc'ing anyway, can this be replaced with a single psprintf()?

Fair point, V4 will make use of psprintf().

> 
>> +       /* Initialize SystemUser now that MyClientConnectionInfo is restored. */
>> +       InitializeSystemUser(MyClientConnectionInfo.authn_id,
>> +                                                hba_authname(MyClientConnectionInfo.auth_method));
> 
> It makes me a little nervous to call hba_authname(auth_method) without
> checking to see that auth_method is actually valid (which is only true
> if authn_id is not NULL).

Will add additional check for safety in V4.


> 
> We could pass the bare auth_method index, or update the documentation
> for auth_method to state that it's guaranteed to be zero if authn_id is
> NULL (and then enforce that).
> 
>>          case SVFOP_CURRENT_USER:
>>          case SVFOP_USER:
>>          case SVFOP_SESSION_USER:
>> +       case SVFOP_SYSTEM_USER:
>>          case SVFOP_CURRENT_CATALOG:
>>          case SVFOP_CURRENT_SCHEMA:
>>              svf->type = NAMEOID;
> 
> Should this be moved to use TEXTOID instead?

Good catch, will do in V4.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On 9/8/22 3:17 AM, Michael Paquier wrote:
> On Wed, Sep 07, 2022 at 08:48:43AM -0700, Jacob Champion wrote:
> 
>> We could pass the bare auth_method index, or update the documentation
>> for auth_method to state that it's guaranteed to be zero if authn_id is
>> NULL (and then enforce that).
>>
>>>          case SVFOP_CURRENT_USER:
>>>          case SVFOP_USER:
>>>          case SVFOP_SESSION_USER:
>>> +       case SVFOP_SYSTEM_USER:
>>>          case SVFOP_CURRENT_CATALOG:
>>>          case SVFOP_CURRENT_SCHEMA:
>>>              svf->type = NAMEOID;
>>
>> Should this be moved to use TEXTOID instead?
> 
> Yeah, it should.  There is actually a second and much deeper issue
> here, in the shape of a collation problem.  See the assertion failure
> in exprSetCollation(), because we expect SQLValueFunction nodes to
> return a name or a non-collatable type.  However, for this case, we'd
> require a text to get rid of the 63-character limit, and that's
> a collatable type.  This reminds me of the recent thread to work on
> getting rid of this limit for the name type..

Please find attached V4 taking care of Jacob's previous comments.

As far the assertion failure mentioned by Michael when moving the 
SVFOP_SYSTEM_USER from NAMEOID to TEXTOID: V4 is assuming that it is 
safe to force the collation to C_COLLATION_OID for SQLValueFunction 
having a TEXT type, but I would be happy to also hear your thoughts 
about it.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On 9/26/22 06:29, Drouvot, Bertrand wrote:
> Please find attached V4 taking care of Jacob's previous comments.

> +    /*
> +     * InitializeSystemUser should already be called once we are sure that
> +     * authn_id is not NULL (means auth_method is actually valid).
> +     * But keep the test here also for safety.
> +     */
> +    if (authn_id)

Since there are only internal clients to the API, I'd argue this makes
more sense as an Assert(authn_id != NULL), but I don't think it's a
dealbreaker.

> As far the assertion failure mentioned by Michael when moving the 
> SVFOP_SYSTEM_USER from NAMEOID to TEXTOID: V4 is assuming that it is 
> safe to force the collation to C_COLLATION_OID for SQLValueFunction 
> having a TEXT type, but I would be happy to also hear your thoughts 
> about it.

Unfortunately I don't have much to add here; I don't know enough about
the underlying problems.

Thanks,
--Jacob

On Tue, Sep 27, 2022 at 03:38:49PM -0700, Jacob Champion wrote:
> On 9/26/22 06:29, Drouvot, Bertrand wrote:
> Since there are only internal clients to the API, I'd argue this makes
> more sense as an Assert(authn_id != NULL), but I don't think it's a
> dealbreaker.

Using an assert() looks like a good idea from here.  If this is called
with a NULL authn, this could reflect a problem in the authentication
logic.

>> As far the assertion failure mentioned by Michael when moving the
>> SVFOP_SYSTEM_USER from NAMEOID to TEXTOID: V4 is assuming that it is
>> safe to force the collation to C_COLLATION_OID for SQLValueFunction
>> having a TEXT type, but I would be happy to also hear your thoughts
>> about it.
>
> Unfortunately I don't have much to add here; I don't know enough about
> the underlying problems.

I have been looking at that, and after putting my hands on that this
comes down to the facility introduced in 40c24bf.  So, I think that
we'd better use COERCE_SQL_SYNTAX so as there is no need to worry
about the shortcuts this patch is trying to use with the collation
setup.  And there are a few tests for get_func_sql_syntax() in
create_view.sql.  Note that this makes the patch slightly shorter, and
simpler.

The docs still mentioned "name", and not "text".

This brings in a second point.  40c24bf has refrained from removing
SQLValueFunction, but based the experience on this thread I see a
pretty good argument in doing the jump once and for all.  This
deserves a separate discussion, though.  I'll do that and create a new
thread.
--
Michael

Hi,

On 9/28/22 5:28 AM, Michael Paquier wrote:
> On Tue, Sep 27, 2022 at 03:38:49PM -0700, Jacob Champion wrote:
>> On 9/26/22 06:29, Drouvot, Bertrand wrote:
>> Since there are only internal clients to the API, I'd argue this makes
>> more sense as an Assert(authn_id != NULL), but I don't think it's a
>> dealbreaker.
> 
> Using an assert() looks like a good idea from here.  If this is called
> with a NULL authn, this could reflect a problem in the authentication
> logic.
> 

Agree, thanks for pointing out.

>>> As far the assertion failure mentioned by Michael when moving the
>>> SVFOP_SYSTEM_USER from NAMEOID to TEXTOID: V4 is assuming that it is
>>> safe to force the collation to C_COLLATION_OID for SQLValueFunction
>>> having a TEXT type, but I would be happy to also hear your thoughts
>>> about it.
>>
>> Unfortunately I don't have much to add here; I don't know enough about
>> the underlying problems.
> 
> I have been looking at that, and after putting my hands on that this
> comes down to the facility introduced in 40c24bf.  So, I think that
> we'd better use COERCE_SQL_SYNTAX so as there is no need to worry
> about the shortcuts this patch is trying to use with the collation
> setup.

Nice!

>  And there are a few tests for get_func_sql_syntax() in
> create_view.sql.  Note that this makes the patch slightly shorter, and
> simpler.
>

Agree that it does look simpler that way and that making use of 
COERCE_SQL_SYNTAX does looks like a better approach. Nice catch!


> The docs still mentioned "name", and not "text".
> 

Oups, thanks for pointing out.

I had a look at v5 and it does look good to me.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Sep 28, 2022 at 12:58:48PM +0200, Drouvot, Bertrand wrote:
> I had a look at v5 and it does look good to me.

Okay, cool.  I have spent some time today doing a last pass over it
and an extra round of tests.  Things looked fine, so applied.
--
Michael

Hi,

On 9/29/22 8:12 AM, Michael Paquier wrote:
> On Wed, Sep 28, 2022 at 12:58:48PM +0200, Drouvot, Bertrand wrote:
>> I had a look at v5 and it does look good to me.
> 
> Okay, cool.  I have spent some time today doing a last pass over it
> and an extra round of tests.  Things looked fine, so applied.
> --

Thanks for your precious help!

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com