Re: is_superuser versus set_config_option's parallelism check

On Sat, Aug 10, 2024 at 12:57:36PM -0400, Tom Lane wrote:
> Yeah, the header comment could stand to be improved to make this
> clearer.  I think there are more conditions being checked now than
> existed when the comment was written.  But the para right below the
> bit you quoted is pretty clear that "return 0" is associated with
> an ereport.

Ah, sorry, ENOCOFFEE.

> Maybe
> 
>  * Return value:
>  *  +1: the value is valid and was successfully applied.
>  *  0:  the name or value is invalid, or it's invalid to try to set
>  *      this GUC now; but elevel was less than ERROR (see below).
>  *  -1: no error detected, but the value was not applied, either
>  *      because changeVal is false or there is some overriding value.

Nevertheless, I think this is an improvement.

Regarding returning 0 instead of -1 for the parallel case, I think that
follows.  While doing some additional research, I noticed this return value
was just added in December (commit 059de3c [0]).  Before that, it
apparently assumed that elevel >= ERROR.  With that and your analysis of
the call sites, it seems highly unlikely that changing it will cause any
problems.

For the errcode, I do see that we pretty consistently use
ERRCODE_INVALID_TRANSACTION_STATE for "can't do thing during a parallel
operation."  In fact, it looks like all but one use is for parallel errors.
I don't have any particular qualms about changing it to
ERRCODE_CANT_CHANGE_RUNTIME_PARAM in set_config_with_handle(), but I
thought that was interesting.

[0] https://postgr.es/m/2089235.1703617353%40sss.pgh.pa.us

-- 
nathan