Re: is_superuser is not documented - Mailing list pgsql-hackers

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