Re: Should vacuum process config file reload more often - Mailing list pgsql-hackers

From Kyotaro Horiguchi
Subject Re: Should vacuum process config file reload more often
Date
Msg-id 20230329.173456.1185961934810139447.horikyota.ntt@gmail.com
Whole thread Raw
In response to Re: Should vacuum process config file reload more often  (Kyotaro Horiguchi <horikyota.ntt@gmail.com>)
Responses Re: Should vacuum process config file reload more often  (Melanie Plageman <melanieplageman@gmail.com>)
List pgsql-hackers
At Wed, 29 Mar 2023 13:21:55 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> So, sorry for the noise. I'll review it while this into cnosideration.

0003:

It's not this patche's fault, but I don't like the fact that the
variables used for GUC, VacuumCostDelay and VacuumCostLimit, are
updated outside the GUC mechanism. Also I don't like the incorrect
sorting of variables, where some working variables are referred to as
GUC parameters or vise versa.

Although it's somewhat unrelated to the goal of this patch, I think we
should clean up the code tidy before proceeding. Shouldn't we separate
the actual parameters from the GUC base variables, and sort out the
all related variaghble? (something like the attached, on top of your
patch.)


I have some comments on 0003 as-is.

+        tab->at_relopt_vac_cost_limit = avopts ?
+            avopts->vacuum_cost_limit : 0;
+        tab->at_relopt_vac_cost_delay = avopts ?
+            avopts->vacuum_cost_delay : -1;

The value is not used when do_balance is false, so I don't see a
specific reason for these variables to be different when avopts is
null.

+autovac_recalculate_workers_for_balance(void)
+{
+    dlist_iter    iter;
+    int            orig_nworkers_for_balance;
+    int            nworkers_for_balance = 0;
+
+    if (autovacuum_vac_cost_delay == 0 ||
+        (autovacuum_vac_cost_delay == -1 && VacuumCostDelay == 0))
         return;
+    if (autovacuum_vac_cost_limit <= 0 && VacuumCostLimit <= 0)
+        return;
+

I'm not quite sure how these conditions relate to the need to count
workers that shares the global I/O cost. (Though I still believe this
funtion might not be necessary.)

+    if (av_relopt_cost_limit > 0)
+        VacuumCostLimit = av_relopt_cost_limit;
+    else
+    {
+        av_base_cost_limit = autovacuum_vac_cost_limit > 0 ?
+            autovacuum_vac_cost_limit : VacuumCostLimit;
+
+        AutoVacuumBalanceLimit();

I think each worker should use MyWorkerInfo->wi_dobalance to identyify
whether the worker needs to use balanced cost values.


+void
+AutoVacuumBalanceLimit(void)

I'm not sure this function needs to be a separate function.

(Sorry, timed out..)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Attachment

pgsql-hackers by date:

Previous
From: Amit Langote
Date:
Subject: Re: "variable not found in subplan target list"
Next
From: Denis Laxalde
Date:
Subject: Re: [EXTERNAL] Re: Add non-blocking version of PQcancel