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
