Обсуждение: is_superuser is not documented

Hi!

is_superuser function checks whether a user is a superuser or not, and 
is commonly used. However, is_superuser is not documented and is set to 
UNGROUPED in guc.c. I think is_superuser should be added to the 
documentation and set to PRESET OPTIONS.What are you thought on this?

Regards,
Kotaro Kawmaoto

is_superuser function checks whether a user is a superuser or not, and 

is commonly used. However, is_superuser is not documented and is set to 

UNGROUPED in guc.c. I think is_superuser should be added to the 

documentation and set to PRESET OPTIONS.What are you thought on this?

"Euler Taveira" <euler@eulerto.com> writes:
> On Fri, Sep 9, 2022, at 2:28 AM, bt22kawamotok wrote:
>> is_superuser function checks whether a user is a superuser or not, and
>> is commonly used. However, is_superuser is not documented and is set to
>> UNGROUPED in guc.c. I think is_superuser should be added to the
>> documentation and set to PRESET OPTIONS.What are you thought on this?

> There is no such function. Are you referring to the GUC? I agree that it should
> be added to the documentation.

If you look at guc.c, it kind of seems intentional that it's undocumented:

        /* Not for general use --- used by SET SESSION AUTHORIZATION */
        {"is_superuser", PGC_INTERNAL, UNGROUPED,
            gettext_noop("Shows whether the current user is a superuser."),
            NULL,
            GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
        },
        &session_auth_is_superuser,
        false,
        NULL, NULL, NULL

On the other hand, it seems pretty silly that it's GUC_REPORT if
we want to consider it private.  I've not checked the git history,
but I bet that flag was added later with no thought about context.

If we are going to document this then we should at least remove
the GUC_NO_SHOW_ALL flag and rewrite the comment.  I wonder whether
the GUC_NO_RESET_ALL flag is needed either --- seems like the
PGC_INTERNAL context protects it sufficiently.

            regards, tom lane

I wrote:
> On the other hand, it seems pretty silly that it's GUC_REPORT if
> we want to consider it private.  I've not checked the git history,
> but I bet that flag was added later with no thought about context.
>
> If we are going to document this then we should at least remove
> the GUC_NO_SHOW_ALL flag and rewrite the comment.  I wonder whether
> the GUC_NO_RESET_ALL flag is needed either --- seems like the
> PGC_INTERNAL context protects it sufficiently.

BTW, "session_authorization" has a subset of these same issues:

        /* Not for general use --- used by SET SESSION AUTHORIZATION */
        {"session_authorization", PGC_USERSET, UNGROUPED,
            gettext_noop("Sets the session user name."),
            NULL,
            GUC_IS_NAME | GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE |
GUC_NOT_WHILE_SEC_REST
        },
        &session_authorization_string,
        NULL,
        check_session_authorization, assign_session_authorization, NULL

I wonder why this one is marked USERSET where the other is not.
You'd think both of them need similar special-casing about how
to handle SET.

            regards, tom lane

> On the other hand, it seems pretty silly that it's GUC_REPORT if
> we want to consider it private.  I've not checked the git history,
> but I bet that flag was added later with no thought about context.
> 
> If we are going to document this then we should at least remove
> the GUC_NO_SHOW_ALL flag and rewrite the comment.  I wonder whether
> the GUC_NO_RESET_ALL flag is needed either --- seems like the
> PGC_INTERNAL context protects it sufficiently.

> I wonder why this one is marked USERSET where the other is not.
> You'd think both of them need similar special-casing about how
> to handle SET.

Thanks for your review.

I have created a patch in response to your suggestion.
I wasn't sure about USERSET, so I only created documentation for 
is_superuser.

Regards,
Kotaro Kawamoto.

On 2022/09/12 17:13, bt22kawamotok wrote:
>> On the other hand, it seems pretty silly that it's GUC_REPORT if
>> we want to consider it private.  I've not checked the git history,
>> but I bet that flag was added later with no thought about context.
>>
>> If we are going to document this then we should at least remove
>> the GUC_NO_SHOW_ALL flag and rewrite the comment.  I wonder whether
>> the GUC_NO_RESET_ALL flag is needed either --- seems like the
>> PGC_INTERNAL context protects it sufficiently.
> 
>> I wonder why this one is marked USERSET where the other is not.
>> You'd think both of them need similar special-casing about how
>> to handle SET.

SET SESSION AUTHORIZATION command changes the setting of session_authorization
by calling set_config_option() with PGC_SUSET/USERSET and PGC_S_SESSION in
ExecSetVariableStmt(). So SET SESSION AUTHORIZATION command may fail
unless session_authorization uses PGC_USERSET.

OTOH, SET SESSION AUTHORIZATION causes to call the assign hook for
session_authorization and it changes is_superuser by using PGC_INTERNAL and
PGC_S_DYNAMIC_DEFAULT. So is_superuser doesn't need to use PGC_USERSET.

I think that session_authorization also can use PGC_INTERNAL if we add
the special-handling in SET SESSION AUTHORIZATION command. But it seems a bit
overkill to me.

> Thanks for your review.
> 
> I have created a patch in response to your suggestion.
> I wasn't sure about USERSET, so I only created documentation for is_superuser.

Thanks for the patch!


+     <varlistentry id="guc-is-superuser" xreflabel="is_superuser">
+      <term><varname>is_superuser</varname> (<type>boolean</type>)

You need to add this entry just after that of "in_hot_standby" because
the descriptions of preset parameters should be placed in alphabetical
order in the docs.


+       <para>
+        Reports whether the user is superuser or not.

Isn't it better to add "current" before "user", e.g.,
"Reports whether the current user is a superuser"?


          /* Not for general use --- used by SET SESSION AUTHORIZATION */
-        {"is_superuser", PGC_INTERNAL, UNGROUPED,
+        {"is_superuser", PGC_INTERNAL, PRESET_OPTIONS,

This comment should be rewritten or removed because "Not for general
use" part is not true.


-            GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+            GUC_REPORT | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE

As Tom commented upthread, GUC_NO_RESET_ALL flag should be removed
because it's not necessary when PGC_INTERNAL context (i.e., in this context,
RESET ALL is prohibit by defaulted) is used.


With the patch, make check failed. You need to update src/test/regress/expected/guc.out.


        <varlistentry>
         <term><literal>IS_SUPERUSER</literal></term>
         <listitem>
          <para>
           True if the current role has superuser privileges.
          </para>

I found that the docs of SET command has the above description about is_superuser.
This description seems useless if we document the is_superuser GUC itself. So isn't
it better to remove this description?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

> Thanks for the patch!
> 
> 
> +     <varlistentry id="guc-is-superuser" xreflabel="is_superuser">
> +      <term><varname>is_superuser</varname> (<type>boolean</type>)
> 
> You need to add this entry just after that of "in_hot_standby" because
> the descriptions of preset parameters should be placed in alphabetical
> order in the docs.
> 
> 
> +       <para>
> +        Reports whether the user is superuser or not.
> 
> Isn't it better to add "current" before "user", e.g.,
> "Reports whether the current user is a superuser"?
> 
> 
>          /* Not for general use --- used by SET SESSION AUTHORIZATION */
> -        {"is_superuser", PGC_INTERNAL, UNGROUPED,
> +        {"is_superuser", PGC_INTERNAL, PRESET_OPTIONS,
> 
> This comment should be rewritten or removed because "Not for general
> use" part is not true.
> 
> 
> -            GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL |
> GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
> +            GUC_REPORT | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | 
> GUC_DISALLOW_IN_FILE
> 
> As Tom commented upthread, GUC_NO_RESET_ALL flag should be removed
> because it's not necessary when PGC_INTERNAL context (i.e., in this 
> context,
> RESET ALL is prohibit by defaulted) is used.
> 
> 
> With the patch, make check failed. You need to update
> src/test/regress/expected/guc.out.
> 
> 
>        <varlistentry>
>         <term><literal>IS_SUPERUSER</literal></term>
>         <listitem>
>          <para>
>           True if the current role has superuser privileges.
>          </para>
> 
> I found that the docs of SET command has the above description about
> is_superuser.
> This description seems useless if we document the is_superuser GUC
> itself. So isn't
> it better to remove this description?

Thank you for your review.
I create new patch add_document_is_superuser_v2.
please check it.

Regards,
Kotaro Kawamoto

On 2022/09/13 17:25, bt22kawamotok wrote:
> 
>> Thanks for the patch!
>>
>>
>> +     <varlistentry id="guc-is-superuser" xreflabel="is_superuser">
>> +      <term><varname>is_superuser</varname> (<type>boolean</type>)
>>
>> You need to add this entry just after that of "in_hot_standby" because
>> the descriptions of preset parameters should be placed in alphabetical
>> order in the docs.
>>
>>
>> +       <para>
>> +        Reports whether the user is superuser or not.
>>
>> Isn't it better to add "current" before "user", e.g.,
>> "Reports whether the current user is a superuser"?
>>
>>
>>          /* Not for general use --- used by SET SESSION AUTHORIZATION */
>> -        {"is_superuser", PGC_INTERNAL, UNGROUPED,
>> +        {"is_superuser", PGC_INTERNAL, PRESET_OPTIONS,
>>
>> This comment should be rewritten or removed because "Not for general
>> use" part is not true.
>>
>>
>> -            GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL |
>> GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
>> +            GUC_REPORT | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
>>
>> As Tom commented upthread, GUC_NO_RESET_ALL flag should be removed
>> because it's not necessary when PGC_INTERNAL context (i.e., in this context,
>> RESET ALL is prohibit by defaulted) is used.
>>
>>
>> With the patch, make check failed. You need to update
>> src/test/regress/expected/guc.out.
>>
>>
>>        <varlistentry>
>>         <term><literal>IS_SUPERUSER</literal></term>
>>         <listitem>
>>          <para>
>>           True if the current role has superuser privileges.
>>          </para>
>>
>> I found that the docs of SET command has the above description about
>> is_superuser.
>> This description seems useless if we document the is_superuser GUC
>> itself. So isn't
>> it better to remove this description?
> 
> Thank you for your review.
> I create new patch add_document_is_superuser_v2.
> please check it.

Thanks for updating the patch!

The patch looks good to me.

-        /* Not for general use --- used by SET SESSION AUTHORIZATION */
          {"session_authorization", PGC_USERSET, UNGROUPED,

If we don't document session_authorization and do expect that
it's usually used by SET/SHOW SESSION AUTHORIZATION,
this comment doesn't need to be removed, I think.

Could you register this patch into the next Commit Fest
so that we don't forget it?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

> Thanks for updating the patch!
> 
> The patch looks good to me.
> 
> -        /* Not for general use --- used by SET SESSION AUTHORIZATION */
>          {"session_authorization", PGC_USERSET, UNGROUPED,
> 
> If we don't document session_authorization and do expect that
> it's usually used by SET/SHOW SESSION AUTHORIZATION,
> this comment doesn't need to be removed, I think.

Thanks for reviewing.

I update patch to reflect master update.

> Could you register this patch into the next Commit Fest
> so that we don't forget it?

OK. I will register next Commit Fest. Thank you.

Regards,

Kotaro Kawamoto

On 2022/09/14 14:27, bt22kawamotok wrote:
> I update patch to reflect master update.

Thanks for updating the patch!

+       <para>
+        Shows whether the current user is a superuser or not.
+       </para>

How about adding the note about when this parameter can change,
like we do for in_hot_standby docs?  I applied this change to the patch.
Attached is the updated version of the patch.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Thu, Mar  2, 2023 at 12:00:43PM -0500, Joseph Koshakow wrote:
> 
> 
> On Thu, Mar 2, 2023 at 11:53 AM Fujii Masao <masao.fujii@oss.nttdata.com>
> wrote:
> >
> >    On 2022/09/14 14:27, bt22kawamotok wrote:
> >    > I update patch to reflect master update.
> >
> >    Thanks for updating the patch!
> >
> >    +       <para>
> >    +        Shows whether the current user is a superuser or not.
> >    +       </para>
> >
> >    How about adding the note about when this parameter can change,
> >    like we do for in_hot_standby docs?  I applied this change to the patch.
> >    Attached is the updated version of the patch.
> >
> 
> I just came across this thread and noticed that the patch was never
> merged. There is some brief docs for is_superuser in the SHOW docs:
> https://www.postgresql.org/docs/current/sql-show.html, but the GUC
> fields were never updated.
> 
> Is there a reason that it never got merged or was it just forgotten
> about?

Uh, where are you looking?  I see it in the SGML, and in the PG 15 docs:

    https://www.postgresql.org/docs/current/sql-show.html

    IS_SUPERUSER
    
        True if the current role has superuser privileges.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Embrace your flaws.  They make you human, rather than perfect,
  which you will never be.

On 2023/04/01 22:34, Joseph Koshakow wrote:
> The patch updated the guc table for is_superuser in
> src/backend/utils/misc/guc_tables.c

Yes, this patch moves the descriptions of is_superuser to config.sgml
and changes its group to PRESET_OPTIONS.

> However, when I look at the code on master I don't see this update

Yes, the patch has not been committed yet because of lack of review comments.
Do you have any review comments on this patch?
Or you think it's ready for committer?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Sat, Apr 8, 2023 at 10:54 AM Joseph Koshakow <koshy44@gmail.com> wrote:
> is_superuser feels a little out of place in this file. All of
> the options here apply to the entire PostgreSQL server, while
> is_superuser only applies to the current session. The description of
> this file says :
>
> > These options report various aspects of PostgreSQL behavior that
> > might be of interest to certain applications, particularly
> > administrative front-ends. Most of them are determined when
> > PostgreSQL is compiled or when it is installed.
>
> Which doesn't seem to apply to is_superuser. It doesn't affect
> the behavior of PostgreSQL, only what the current session is allowed to
> do.

I'm not sure I agree with that. I mean, maybe the phrasing could be
improved somehow, but "PostgreSQL behavior" as a category seems to
include whether or not it lets you do certain things. And, for
example, psql will show a > or # in the prompt based on whether you're
a superuser. I find "administrative front-end" to be a somewhat odd
turn of phrase, but I guess it means general purpose frontends like
pgsql or pgAdmin or whatever that you might use to administer the
system, as opposed to applications.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2023/04/08 23:53, Joseph Koshakow wrote:
> 
> 
> On Mon, Apr 3, 2023 at 10:47 AM Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>>
wrote:
>  >    Yes, the patch has not been committed yet because of lack of review comments.
>  >    Do you have any review comments on this patch?
>  >    Or you think it's ready for committer?
> 
> I'm not very familiar with this code, so I'm not sure how much my
> review is worth, but maybe it will spark some discussion.

Thanks for the comments!


>  > Yes, this patch moves the descriptions of is_superuser to config.sgml
>  > and changes its group to PRESET_OPTIONS.
> 
> is_superuser feels a little out of place in this file. All of
> the options here apply to the entire PostgreSQL server, while
> is_superuser only applies to the current session.

Aren't other preset options like lc_collate, lc_ctype and server_encoding
similar to is_superuser? They seem to behave in a similar way as their
settings can be different for each connection depending on the connected database.


> I'm not familiar with the origins of is_superuser and it may be too
> late for this, but it seems like is_superuser would fit in much better
> as a system information function [0] rather than a GUC. Particularly
> in the Session Information Functions.

I understand your point, but I think it would be more confusing to document
is_superuser there because it's defined and behaves differently from
session information functions like current_user. For instance,
the value of is_superuser can be displayed using SHOW command,
while current_user cannot. Therefore, it's better to keep is_superuser
separate from the session information functions.


> As a side note server_version, server_encoding, lc_collate, and
> lc_ctype all appear in both the preset options section of config.sgml
> and in show.sgml. I'm not sure what the logic is for just including
> these three parameters in show.sgml, but I think we should either
> include all of the preset options or none of them.

Agreed. I think that it's better to just treat them as GUCs and
remove their descriptions from show.sgml.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2023/04/12 5:41, Joseph Koshakow wrote:
> Having said all that I actually think this is the best place for
> is_superuser since it doesn't seem to fit in anywhere else.

Yeah, I also could not find more appropriate place for is_superuser than there.


> I was implying that I thought it would have made more sense for
> is_superuser to be implemented as a function, behave as a function,
> and not be visible via SHOW. However, there may have been a good reason
> not to do this and it may already be too late for that.

The is_superuser parameter is currently marked as GUC_REPORT and
its value is automatically reported to a client. If we change
it to a function, we will need to add functionality to automatically
report the return value of the function to a client, which could
be overkill.


> In my opinion, this is ready to be committed.

Thanks! Given that we have already exceeded the feature freeze date,
I'm thinking to commit this change at the next CommitFest.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2023/06/07 23:15, Joseph Koshakow wrote:
> I think I may have discovered a reason why is_superuser is
> intentionally undocumented. is_superuser is not updated if a role's
> superuser attribute is changed by another session. Therefore,
> is_superuser may show you an incorrect stale value.
> 
> Perhaps this can be fixed with a show_hook? Otherwise it's probably
> best not to document a GUC that can show an incorrect value.

Or we can correct the description of is_superuser, for example,
"True if the current role had superuser privileges when it connected to
the database. Note that this parameter doesn't always indicate
the current superuser status of the role."?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION