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
