Re: is_superuser versus set_config_option's parallelism check

Nathan Bossart <nathandbossart(at)gmail(dot)com> writes:
> On Fri, Aug 09, 2024 at 06:50:14PM -0400, Tom Lane wrote:
>> Also, now that the error depends on which parameter we're talking
>> about, I thought it best to include the parameter name in the error
>> and to re-word it to be more like all the other can't-set-this-now
>> errors just below it. I'm half tempted to change the errcode and
>> set_config_option return value to match the others too, ie
>> ERRCODE_CANT_CHANGE_RUNTIME_PARAM and "return 0" not -1.

> This comment for set_config_option() leads me to think we should be
> returning -1 instead of 0 in many more places in set_config_with_handle():

> * Return value:
> * +1: the value is valid and was successfully applied.
> * 0: the name or value is invalid (but see below).
> * -1: the value was not applied because of context, priority, or changeVal.

> But I haven't thought through it, either. At this point, maybe the comment
> is wrong...

I poked through all the call sites. The only one that makes a
distinction between 0 and -1 is ProcessConfigFileInternal(),
and what it thinks is:

else if (scres == 0)
{
error = true;
item->errmsg = pstrdup("setting could not be applied");
ConfFileWithError = item->filename;
}
else
{
/* no error, but variable's active value was not changed */
item->applied = true;
}

Now, I don't believe that ProcessConfigFileInternal is ever executed
while IsInParallelMode, so it appears that no caller really cares
about which return code this case would return. However, if you
look through set_config_with_handle the general pattern is that
we "return 0" after any ereport call (either one directly in that
function, or one in a called function). Getting to those of course
implies that elevel is too low to throw an error; but we did think
there was an error condition. We "return -1" in cases where we didn't
ereport anything. So I am still of the opinion that the -1 usage here
is inconsistent, even if it happens to not make a difference today.

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.

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.

regards, tom lane