MIME-Version: 1.0
References: <bee34abe-fe87-49b3-8523-8ae19e641ccf@yandex.ru> <70e1cca9-ff89-4e76-a611-d38bcc0e14ad@yandex.ru>
In-Reply-To: <70e1cca9-ff89-4e76-a611-d38bcc0e14ad@yandex.ru>
From: Dilip Kumar <dilipbalaut@gmail.com>
Date: Fri, 7 Jun 2024 12:16:30 +0530
Message-ID: <CAFiTN-s3PG_v6RuozMHtsOeaWuQueGOQeZ3x-ZfPE1jt8hzYrA@mail.gmail.com>
Subject: Re: Vacuum statistics
To: Alena Rybakina <lena.ribackina@yandex.ru>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>, 
	Andrey Lepikhov <a.lepikhov@postgrespro.ru>, Andrei Zubkov <a.zubkov@postgrespro.ru>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAFiTN-s3PG_v6RuozMHtsOeaWuQueGOQeZ3x-ZfPE1jt8hzYrA%40mail.gmail.com>
Precedence: bulk

On Thu, May 30, 2024 at 11:57=E2=80=AFPM Alena Rybakina
<lena.ribackina@yandex.ru> wrote:
>
> On 30.05.2024 10:33, Alena Rybakina wrote:
> >
> > I suggest gathering information about vacuum resource consumption for
> > processing indexes and tables and storing it in the table and index
> > relationships (for example, PgStat_StatTabEntry structure like it has
> > realized for usual statistics). It will allow us to determine how well
> > the vacuum is configured and evaluate the effect of overhead on the
> > system at the strategic level, the vacuum has gathered this
> > information already, but this valuable information doesn't store it.
> >
> My colleagues and I have prepared a patch that can help to solve this
> problem.
>
> We are open to feedback.

I was reading through the patch here are some initial comments.

--
+typedef struct LVExtStatCounters
+{
+ TimestampTz time;
+ PGRUsage ru;
+ WalUsage walusage;
+ BufferUsage bufusage;
+ int64 VacuumPageMiss;
+ int64 VacuumPageHit;
+ int64 VacuumPageDirty;
+ double VacuumDelayTime;
+ PgStat_Counter blocks_fetched;
+ PgStat_Counter blocks_hit;
+} LVExtStatCounters;


I noticed that you are storing both pgBufferUsage and
VacuumPage(Hit/Miss/Dirty) stats. Aren't these essentially the same?
It seems they both exist in the system because some code, like
heap_vacuum_rel(), uses pgBufferUsage, while do_analyze_rel() still
relies on the old counters. And there is already a patch to remove
those old counters.


--
+static Datum
+pg_stats_vacuum(FunctionCallInfo fcinfo, ExtVacReportType type, int ncolum=
ns)
+{

I don't think you need this last parameter (ncolumns) we can anyway
fetch that from tupledesc, so adding an additional parameter
just for checking doesn't look good to me.

--
+ /* Tricky turn here: enforce pgstat to think that our database us dbid */
+
+ MyDatabaseId =3D dbid;

typo
/think that our database us dbid/think that our database has dbid

Also, remove the blank line between the comment and the next code
block that is related to that comment.


--
  VacuumPageDirty =3D 0;
+ VacuumDelayTime =3D 0.;

There is an extra "." after 0


--=20
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com