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
