Re: is_superuser is not documented

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