Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.2\))
Subject: Re: Should vacuum process config file reload more often
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <CAD21AoBS7o6Ljt_vfqPQPf67AhzKu3fR0iqk8B=vVYczMugKMQ@mail.gmail.com>
Date: Fri, 7 Apr 2023 13:28:54 +0200
Cc: Melanie Plageman <melanieplageman@gmail.com>,
 Robert Haas <robertmhaas@gmail.com>,
 Andres Freund <andres@anarazel.de>,
 Tom Lane <tgl@sss.pgh.pa.us>,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 Amit Kapila <amit.kapila16@gmail.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <1195917C-F076-407C-83B8-7CC8BE7A269A@yesql.se>
References: <20230329.132155.629765142788133576.horikyota.ntt@gmail.com>
 <20230329.173456.1185961934810139447.horikyota.ntt@gmail.com>
 <CAAKRu_Yg4-kWH_EN7wtpm3z9B+H27UTmOxzzYv1Vg=YdZ6v_rw@mail.gmail.com>
 <CAD21AoCGdxYAJHmE_nXw_MFw1+Qtn3vcThRu5FiadukgQnpWeQ@mail.gmail.com>
 <935191FB-083B-4060-89FC-466F61FAA391@yesql.se>
 <CAAKRu_aoa0YT36q2n3xa9MZYdepeH4Jyf2EVJj=7YdC_1c974Q@mail.gmail.com>
 <CAAKRu_bSsChKsyEy1ZV2XJQ22FoDdA56efjNr9==m2TO94557w@mail.gmail.com>
 <CAD21AoDi4i4tj1Cf9BG0q2iyVhHisQ2xxVBZajEqExX6t6_b3g@mail.gmail.com>
 <CAAKRu_bkg+0WAGTxsy0HP75RzFbPwqz3ctYq1kThjFhSL69upw@mail.gmail.com>
 <1252075.1680547394@sss.pgh.pa.us>
 <20230403190837.qubpnwugfe2k2g46@awork3.anarazel.de>
 <CAAKRu_ap7nhmqtKQ1TRo3LzHG3FK=YpGO=O0019M3BbN7ysm3A@mail.gmail.com>
 <7E06F879-7E20-4A6A-862F-CA72CDC9A323@yesql.se>
 <CAAKRu_b1HjGCTsFpUnmwLNS8NeXJ+JnrDLhT1osP+Gq9HCU+Rw@mail.gmail.com>
 <CBF0083E-5893-4B35-9498-142A835D03DC@yesql.se>
 <CAAKRu_bJ2DJxYoMkyMo9UpoX74LY5d2Jir86QWFUftrGKWP5Bw@mail.gmail.com>
 <CA+TgmoYDYHht9zsHye3tywx7oWGMP4s_6AGefHkDsMuGHLzT7w@mail.gmail.com>
 <CAAKRu_auVL_uE794zwiX6t18LJvKtJ5mrmV9tkbpWzS8AYeChQ@mail.gmail.com>
 <CAAKRu_aX+MLn32kPnAQ30WChbiLTkKKmY9e8+B1ZY1BokpQZYA@mail.gmail.com>
 <CAAKRu_aH-UbvNCQq-EouNUTjtGexQTRVo7ik_vYR4WUocVny7w@mail.gmail.com>
 <CA+TgmoY3EaCz3_7F3+Sb3FUHonf4nxdC-_UWnn6fWEPtXU7t5g@mail.gmail.com>
 <D0A88FB1-56A6-4CD5-B886-8539B7BA7F58@yesql.se>
 <CAAKRu_YmQg_i=0DBfPzy1MMc2GjfuJFo=0rFv4fCzRQStLhjvw@mail.gmail.com>
 <CAAKRu_beQECOV4WfU6nG0hhEVobCnH88Y8MQi1LrTwMfRyxuCw@mail.gmail.com>
 <5028847C-937A-4CC2-B127-AFF5F9FB79C5@yesql.se>
 <CAAKRu_Znk6WCCQqEw8hv3+X1Fm8gG0x4KKRAkKmO2JOcA-77-g@mail.gmail.com>
 <2B703533-102A-47E5-A4F7-C6A68C09BB27@yesql.se>
 <CAD21AoBS7o6Ljt_vfqPQPf67AhzKu3fR0iqk8B=vVYczMugKMQ@mail.gmail.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/1195917C-F076-407C-83B8-7CC8BE7A269A%40yesql.se>
Precedence: bulk

> On 7 Apr 2023, at 08:52, Masahiko Sawada <sawada.mshk@gmail.com> =
wrote:
> On Fri, Apr 7, 2023 at 8:08=E2=80=AFAM Daniel Gustafsson =
<daniel@yesql.se> wrote:

>> I had another read-through and test-through of this version, and have =
applied
>> it with some minor changes to comments and whitespace.  Thanks for =
the quick
>> turnaround times on reviews in this thread!
>=20
> Cool!
>=20
> Regarding the commit 7d71d3dd08, I have one comment:
>=20
> +       /* Only log updates to cost-related variables */
> +       if (vacuum_cost_delay =3D=3D original_cost_delay &&
> +           vacuum_cost_limit =3D=3D original_cost_limit)
> +           return;
>=20
> IIUC by default, we log not only before starting the vacuum but also
> when changing cost-related variables. Which is good, I think, because
> logging the initial values would also be helpful for investigation.
> However, I think that we don't log the initial vacuum cost values
> depending on the values. For example, if the
> autovacuum_vacuum_cost_delay storage option is set to 0, we don't log
> the initial values. I think that instead of comparing old and new
> values, we can write the log only if
> message_level_is_interesting(DEBUG2) is true. That way, we don't need
> to acquire the lwlock unnecessarily. And the code looks cleaner to me.
> I've attached the patch (use_message_level_is_interesting.patch)

That's a good idea, unless Melanie has conflicting opinions I think we =
should
go ahead with this.  Avoiding taking a lock here is a good save.

> Also, while testing the autovacuum delay with relopt
> autovacuum_vacuum_cost_delay =3D 0, I realized that even if we set
> autovacuum_vacuum_cost_delay =3D 0 to a table, wi_dobalance is set to
> true. wi_dobalance comes from the following expression:
>=20
>        /*
>         * If any of the cost delay parameters has been set =
individually for
>         * this table, disable the balancing algorithm.
>         */
>        tab->at_dobalance =3D
>            !(avopts && (avopts->vacuum_cost_limit > 0 ||
>                         avopts->vacuum_cost_delay > 0));
>=20
> The initial values of both avopts->vacuum_cost_limit and
> avopts->vacuum_cost_delay are -1. I think we should use ">=3D 0" =
instead
> of "> 0". Otherwise, we include the autovacuum worker working on a
> table whose autovacuum_vacuum_cost_delay is 0 to the balancing
> algorithm. Probably this behavior has existed also on back branches
> but I haven't checked it yet.

Interesting, good find.  Looking quickly at the back branches I think =
there is
a variant of this for vacuum_cost_limit even there but needs more =
investigation.

--
Daniel Gustafsson