> On 5 Apr 2023, at 17:29, Melanie Plageman <melanieplageman@gmail.com> wrote:
>>
>> I think I wasn't clear in my comment, sorry. I don't have a problem with
>> introducing a new variable to split the balanced value from the GUC value.
>> What I don't think we should do is repurpose an exported symbol into doing a
>> new thing. In the case at hand I think VacuumCostLimit and VacuumCostDelay
>> should remain the backing variables for the GUCs, with vacuum_cost_limit and
>> vacuum_cost_delay carrying the balanced values. So the inverse of what is in
>> the patch now.
>>
>> The risk of these symbols being used in extensions might be very low but on
>> principle it seems unwise to alter a symbol and risk subtle breakage.
>
> I totally see what you are saying. The only complication is that all of
> the other variables used in vacuum code are the camelcase and the gucs
> follow the snake case -- as pointed out in a previous review comment by
> Sawada-san:
Fair point.
>> @@ -83,6 +84,7 @@ int vacuum_cost_limit;
>> */
>> int VacuumCostLimit = 0;
>> double VacuumCostDelay = -1;
>> +static bool vacuum_can_reload_config = false;
>>
>> In vacuum.c, we use snake case for GUC parameters and camel case for
>> other global variables, so it seems better to rename it
>> VacuumCanReloadConfig. Sorry, that's my fault.
>
> This is less of a compelling argument than subtle breakage for extension
> code, though.
How about if we rename the variable into something which also acts at bit as
self documenting why there are two in the first place? Perhaps
BalancedVacuumCostLimit or something similar (I'm terrible with names)?
> I am, however, wondering if extensions expect to have access to the guc
> variable or the global variable -- or both?
Extensions have access to all exported symbols, and I think it's not uncommon
for extension authors to expect to have access to at least read GUC variables.
--
Daniel Gustafsson