Re: Should vacuum process config file reload more often

> On 4 Apr 2023, at 00:35, Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:
>
> On Mon, Apr 3, 2023 at 3:08 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>> On 2023-04-03 14:43:14 -0400, Tom Lane wrote:
>>> Melanie Plageman <melanieplageman(at)gmail(dot)com> writes:
>>>> v13 attached with requested updates.
>>>
>>> I'm afraid I'd not been paying any attention to this discussion,
>>> but better late than never. I'm okay with letting autovacuum
>>> processes reload config files more often than now. However,
>>> I object to allowing ProcessConfigFile to be called from within
>>> commands in a normal user backend. The existing semantics are
>>> that user backends respond to SIGHUP only at the start of processing
>>> a user command, and I'm uncomfortable with suddenly deciding that
>>> that can work differently if the command happens to be VACUUM.
>>> It seems unprincipled and perhaps actively unsafe.
>>
>> I think it should be ok in commands like VACUUM that already internally start
>> their own transactions, and thus require to be run outside of a transaction
>> and at the toplevel. I share your concerns about allowing config reload in
>> arbitrary places. While we might want to go there, it would require a lot more
>> analysis.

Thinking more on this I'm leaning towards going with allowing more frequent
reloads in autovacuum, and saving the same for VACUUM for more careful study.
The general case is probably fine but I'm not convinced that there aren't error
cases which can present unpleasant scenarios.

Regarding the autovacuum part of this patch I think we are down to the final
details and I think it's doable to finish this in time for 16.

> As an alternative for your consideration, attached v14 set implements
> the config file reload for autovacuum only (in 0003) and then enables it
> for VACUUM and ANALYZE not in a nested transaction command (in 0004).
>
> Previously I had the commits in the reverse order for ease of review (to
> separate changes to worker limit balancing logic from config reload
> code).

A few comments on top of already submitted reviews, will do another pass over
this later today.

+ * VacuumFailsafeActive is a defined as a global so that we can determine
+ * whether or not to re-enable cost-based vacuum delay when vacuuming a table.

This comment should be expanded to document who we expect to inspect this
variable in order to decide on cost-based vacuum.

Moving the failsafe switch into a global context means we face the risk of an
extension changing it independently of the GUCs that control it (or the code
relying on it) such that these are out of sync. External code messing up
internal state is not new and of course outside of our control, but it's worth
at least considering. There isn't too much we can do here, but perhaps expand
this comment to include a "do not change this" note?

+extern bool VacuumFailsafeActive;

While I agree with upthread review comments that extensions shoulnd't poke at
this, not decorating it with PGDLLEXPORT adds little protection and only cause
inconsistencies in symbol exports across platforms. We only explicitly hide
symbols in shared libraries IIRC.

+extern int VacuumCostLimit;
+extern double VacuumCostDelay;
...
-extern PGDLLIMPORT int VacuumCostLimit;
-extern PGDLLIMPORT double VacuumCostDelay;

Same with these, I don't think this is according to our default visibility.
Moreover, I'm not sure it's a good idea to perform this rename. This will keep
VacuumCostLimit and VacuumCostDelay exported, but change their meaning. Any
external code referring to these thinking they are backing the GUCs will still
compile, but may be broken in subtle ways. Is there a reason for not keeping
the current GUC variables and instead add net new ones?

+ * TODO: should VacuumCostLimit and VacuumCostDelay be initialized to valid or
+ * invalid values?
+ */
+int VacuumCostLimit = 0;
+double VacuumCostDelay = -1;

I think the important part is to make sure they are never accessed without
VacuumUpdateCosts having been called first. I think that's the case here, but
it's not entirely clear. Do you see a codepath where that could happen? If
they are initialized to a sentinel value we also need to check for that, so
initializing to the defaults from the corresponding GUCs seems better.

+* Update VacuumCostLimit with the correct value for an autovacuum worker, given

Trivial whitespace error in function comment.

+static double av_relopt_cost_delay = -1;
+static int av_relopt_cost_limit = 0;

These need a comment IMO, ideally one that explain why they are initialized to
those values.

+ /* There is at least 1 autovac worker (this worker). */
+ Assert(nworkers_for_balance > 0);

Is there a scenario where this is expected to fail? If so I think this should
be handled and not just an Assert.

--
Daniel Gustafsson