MIME-Version: 1.0
References: <CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7+DXhrjDA0gw@mail.gmail.com>
 <CAD21AoC=b09pzWNyPYuti0M2DQwy57CGF9UWWb_+v=gdWeNG=A@mail.gmail.com>
 <CAAKRu_bR2VrTUskR24GkXV15SxPaCiHYD=soOuWZcT-rGDcA8Q@mail.gmail.com> <CAD21AoB=EC6wzjjqDXKmNEhu_BhGg0g_Gc50yh_zvXqiDC_OSg@mail.gmail.com>
In-Reply-To: <CAD21AoB=EC6wzjjqDXKmNEhu_BhGg0g_Gc50yh_zvXqiDC_OSg@mail.gmail.com>
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Thu, 2 Mar 2023 18:37:43 -0500
Message-ID: <CAAKRu_buP5wzsho3qNw5o9_R0pF69FRM5hgCmr-mvXmGXwdA7A@mail.gmail.com>
Subject: Re: Should vacuum process config file reload more often
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>, Andres Freund <andres@anarazel.de>, 
	Amit Kapila <amit.kapila16@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAAKRu_buP5wzsho3qNw5o9_R0pF69FRM5hgCmr-mvXmGXwdA7A%40mail.gmail.com>
Precedence: bulk

On Thu, Mar 2, 2023 at 2:36=E2=80=AFAM Masahiko Sawada <sawada.mshk@gmail.c=
om> wrote:
>
> On Thu, Mar 2, 2023 at 10:41 AM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > On another topic, I've just realized that when autovacuuming we only
> > update tab->at_vacuum_cost_delay/limit from
> > autovacuum_vacuum_cost_delay/limit for each table (in
> > table_recheck_autovac()) and then use that to update
> > MyWorkerInfo->wi_cost_delay/limit. MyWorkerInfo->wi_cost_delay/limit is
> > what is used to update VacuumCostDelay/Limit in AutoVacuumUpdateDelay()=
.
> > So, even if we reload the config file in vacuum_delay_point(), if we
> > don't use the new value of autovacuum_vacuum_cost_delay/limit it will
> > have no effect for autovacuum.
>
> Right, but IIUC wi_cost_limit (and VacuumCostDelayLimit) might be
> updated. After the autovacuum launcher reloads the config file, it
> calls autovac_balance_cost() that updates that value of active
> workers. I'm not sure why we don't update workers' wi_cost_delay,
> though.

Ah yes, I didn't realize this. Thanks. I went back and did more code
reading/analysis, and I see no reason why we shouldn't update
worker->wi_cost_delay to the new value of autovacuum_vac_cost_delay in
autovac_balance_cost(). Then, as you said, the autovac launcher will
call autovac_balance_cost() when it reloads the configuration file.
Then, the next time the autovac worker calls AutoVacuumUpdateDelay(), it
will update VacuumCostDelay.

> > I started writing a little helper that could be used to update these
> > workerinfo->wi_cost_delay/limit in vacuum_delay_point(),
>
> Since we set vacuum delay parameters for autovacuum workers so that we
> ration out I/O equally, I think we should keep the current mechanism
> that the autovacuum launcher sets workers' delay parameters and they
> update accordingly.

Yes, agreed, it should go in the same place as where we update
wi_cost_limit (autovac_balance_cost()). I think we should potentially
rename autovac_balance_cost() because its name and all its comments
point to its only purpose being to balance the total of the workers
wi_cost_limits to no more than autovacuum_vacuum_cost_limit. And the
autovacuum_vacuum_cost_delay doesn't need to be balanced in this way.

Though, since this change on its own would make autovacuum pick up new
values of autovacuum_vacuum_cost_limit (without having the worker reload
the config file), I wonder if it makes sense to try and have
vacuum_delay_point() only reload the config file if it is an explicit
vacuum or an analyze not being run in an outer transaction (to avoid
overhead of reloading config file)?

The lifecycle of this different vacuum delay-related gucs and how it
differs between autovacuum workers and explicit vacuum is quite tangled
already, though.

> >  but I notice
> > when they are first set, we consider the autovacuum table options. So,
> > I suppose I would need to consider these when updating
> > wi_cost_delay/limit later as well? (during vacuum_delay_point() or
> > in AutoVacuumUpdateDelay())
> >
> > I wasn't quite sure because I found these chained ternaries rather
> > difficult to interpret, but I think table_recheck_autovac() is saying
> > that the autovacuum table options override all other values for
> > vac_cost_delay?
> >
> >         vac_cost_delay =3D (avopts && avopts->vacuum_cost_delay >=3D 0)
> >             ? avopts->vacuum_cost_delay
> >             : (autovacuum_vac_cost_delay >=3D 0)
> >             ? autovacuum_vac_cost_delay
> >             : VacuumCostDelay;
> >
> > i.e. this?
> >
> >   if (avopts && avopts->vacuum_cost_delay >=3D 0)
> >     vac_cost_delay =3D avopts->vacuum_cost_delay;
> >   else if (autovacuum_vac_cost_delay >=3D 0)
> >     vac_cost_delay =3D autovacuum_vacuum_cost_delay;
> >   else
> >     vac_cost_delay =3D VacuumCostDelay
>
> Yes, if the table has autovacuum table options, we use these values
> and the table is excluded from the balancing algorithm I mentioned
> above. See the code from table_recheck_autovac(),
>
>        /*
>         * If any of the cost delay parameters has been set individually f=
or
>         * this table, disable the balancing algorithm.
>         */
>        tab->at_dobalance =3D
>            !(avopts && (avopts->vacuum_cost_limit > 0 ||
>                         avopts->vacuum_cost_delay > 0));
>
> So if the table has autovacuum table options, the vacuum delay
> parameters probably should be updated by ALTER TABLE, not by reloading
> the config file.

Yes, if the table has autovacuum table options, I think the user is
out-of-luck until the relation is done being vacuumed because the ALTER
TABLE will need to get a lock.

- Melanie