Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1shImV-00G49M-Fy for pgsql-hackers@arkaria.postgresql.org; Fri, 23 Aug 2024 01:07:59 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1shImT-007kFZ-5Z for pgsql-hackers@arkaria.postgresql.org; Fri, 23 Aug 2024 01:07:57 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1shImS-007kFR-QG for pgsql-hackers@lists.postgresql.org; Fri, 23 Aug 2024 01:07:57 +0000 Received: from mail-yb1-xb2a.google.com ([2607:f8b0:4864:20::b2a]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1shImL-0012Ix-Oa for pgsql-hackers@postgresql.org; Fri, 23 Aug 2024 01:07:56 +0000 Received: by mail-yb1-xb2a.google.com with SMTP id 3f1490d57ef6-e115eb44752so1562253276.0 for ; Thu, 22 Aug 2024 18:07:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1724375268; x=1724980068; darn=postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=4zSJvMWJe6rKhCvrp5q5CIs4rv9Es3Ud05UIxkrvsUA=; b=a3sQKmwslEEtTECVWEUzLyEAyn0SBiwsRtoLekbf+NOapUGvlDdS6ySzu3dq67oX05 NpeDMByfQLxeFg+cYFvmFGKJshu/jo7b43UNysBjF0GlV13nXS4Q8/WWfOll4zujRLDK x6ckJUB+kBJNdkK9g/SU+XUwCstDd99lyV6HnP8UaTtch/qH7kaexsn3tL6pjv5PhEOX M3mIPIGIeyVCvIHn2CakbMD6nwCjMnQwSvokItlHloXh1D9SbgIspPY3IvwPbHpBZiAM ERdSgiUH5Qw3eRqmOv8F4yRx/FOvgJAQOJ4DoubaHESUYWNcxqDu/y2yVjY5MNKwjd4L NJ7Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724375268; x=1724980068; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=4zSJvMWJe6rKhCvrp5q5CIs4rv9Es3Ud05UIxkrvsUA=; b=ngnGzDnR+K5qTQlZIdZtAigw1J5Gr+HoA6cGQ5MmQqLtx3adu6m6r+oAVMJFViyehq 1GCDjoqJWS6Lurq8532NQ2hxqKNlO4teO+iEb2Bb+513NxxlB1a7qBQQ3TTTwyVLMiPE Hmo7HCFtam7C7Bo9j+pZPEaTP/TRIkrps4PFIlKZSQA4q3MVGudTZ3YN+y3oFhSHWi6V zeSgIT1v21edc5wl4RK7BjCQDcDSK8xfc9hwW+DAFvwCMU4C4nsmAc/h2lGLHPU+i4/q ZI68lvj0LUkZqgz8Q8AMLW8mPEASRkK1BwGBwixOpvzmsSQWmBDhxC0qxjzJolXR8q5D UQVA== X-Forwarded-Encrypted: i=1; AJvYcCV2IJdaYvCT0/2F9rrvzKIS3eFeHdluQS4g/R/qe8pwJMBIYyijb3SOAA3Ppwe1agLu93pUlNJ/V9n4616W@postgresql.org X-Gm-Message-State: AOJu0YyQHo48bAg0A0j3KF+6Dl+wr3vdfuz+vbYzjZfNA9zoOSDW3ObS WoNTrlj51mF80NS8bjCIiB225p4Prb+THLH6XpQ6uZhA2auCcVmkLADVB6kg+dfiR15HkBMSW5K BMSeqY32/rXfznJFy0d3hUGZVJEc= X-Google-Smtp-Source: AGHT+IGMrIAaaWwGd6kh4LUQo7cY9Vkh7vkdPD4R0pBlBcPqCOSN1/aB+wMcGLT/p8qide1zmybFN6YPPzkQyxhongk= X-Received: by 2002:a05:6902:2581:b0:e16:4e65:9678 with SMTP id 3f1490d57ef6-e17a8af65d2mr817761276.14.1724375268359; Thu, 22 Aug 2024 18:07:48 -0700 (PDT) MIME-Version: 1.0 References: <53c47c2d-72a5-44f2-900c-9973b2af1808@tantorlabs.com> <4a902cea-54fb-41b5-b208-b84731a5f577@postgrespro.ru> <092adec6-4eae-4bd4-bd0d-473a9df1282b@tantorlabs.com> <3deae1bd-ad84-4459-a26e-04c9136b84e9@postgrespro.ru> In-Reply-To: <3deae1bd-ad84-4459-a26e-04c9136b84e9@postgrespro.ru> From: Alexander Korotkov Date: Fri, 23 Aug 2024 04:07:37 +0300 Message-ID: Subject: Re: Vacuum statistics To: Alena Rybakina Cc: Ilia Evdokimov , Andrei Zubkov , Alena Rybakina , pgsql-hackers , a.lepikhov@postgrespro.ru, jian he Content-Type: multipart/alternative; boundary="000000000000548ec206204f671a" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000548ec206204f671a Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Aug 21, 2024 at 1:39=E2=80=AFAM Alena Rybakina wrote: > > I think you've counted the above system tables from the database, but > I'll double-check it. Thank you for your review! > > On 19.08.2024 19:28, Ilia Evdokimov wrote: > > Are you certain that all tables are included in > > `pg_stat_vacuum_tables`? I'm asking because of the following: > > > > > > SELECT count(*) FROM pg_stat_all_tables ; > > count > > ------- > > 108 > > (1 row) > > > > SELECT count(*) FROM pg_stat_vacuum_tables ; > > count > > ------- > > 20 > > (1 row) > > I'd like to do some review a well. + MyDatabaseId =3D dbid; + + PG_TRY(); + { + tabentry =3D pgstat_fetch_stat_tabentry(relid); + MyDatabaseId =3D storedMyDatabaseId; + } + PG_CATCH(); + { + MyDatabaseId =3D storedMyDatabaseId; + } + PG_END_TRY(); I think this is generally wrong to change MyDatabaseId, especially if you have to wrap it with PG_TRY()/PG_CATCH(). I think, instead we need proper API changes, i.e. make pgstat_fetch_stat_tabentry() and others take dboid as an argument. +/* + * Get the vacuum statistics for the heap tables. + */ +Datum +pg_stat_vacuum_tables(PG_FUNCTION_ARGS) +{ + return pg_stats_vacuum(fcinfo, PGSTAT_EXTVAC_HEAP, EXTVACHEAPSTAT_COLUMNS); + + PG_RETURN_NULL(); +} The PG_RETURN_NULL() is unneeded after another return statement. However, does pg_stats_vacuum() need to return anything? What about making its return type void? @@ -874,4 +874,38 @@ pgstat_get_custom_snapshot_data(PgStat_Kind kind) return pgStatLocal.snapshot.custom_data[idx]; } +/* hash table for statistics snapshots entry */ +typedef struct PgStat_SnapshotEntry +{ + PgStat_HashKey key; + char status; /* for simplehash use */ + void *data; /* the stats data itself */ +} PgStat_SnapshotEntry; It would be nice to preserve encapsulation and don't expose pgstat_snapshot hash in the headers. I see there is only one usage of it outside of pgstat.c: pg_stats_vacuum(). + Oid storedMyDatabaseId =3D MyDatabaseId; + + pgstat_update_snapshot(PGSTAT_KIND_RELATION); + MyDatabaseId =3D storedMyDatabaseId; This manipulation with storedMyDatabaseId looks pretty useless. It seems to be intended to change MyDatabaseId, while I'm not fan of this as I mentioned above. +static PgStat_StatTabEntry * +fetch_dbstat_tabentry(Oid dbid, Oid relid) +{ + Oid storedMyDatabaseId =3D MyDatabaseId; + PgStat_StatTabEntry *tabentry =3D NULL; + + if (OidIsValid(CurrentDatabaseId) && CurrentDatabaseId =3D=3D dbid) + /* Quick path when we read data from the same database */ + return pgstat_fetch_stat_tabentry(relid); + + pgstat_clear_snapshot(); It looks scary to reset the whole snapshot each time we access another database. Need to also mention that the CurrentDatabaseId machinery isn't implemented. New functions pg_stat_vacuum_tables(), pg_stat_vacuum_indexes(), pg_stat_vacuum_database(= ) are SRFs. When zero Oid is passed they report all the objects. However, it seems they aren't intended to be used directly. Instead, there are views with the same names. These views always call them with particular Oids, therefore SRFs always return one row. Then why bother with SRF? They could return plain records instead. Also, as I mentioned above patchset makes a lot of trouble accessing statistics of relations of another database. But that seems to be useless given corresponding views allow to see only relations of the current database. Even if you call functions directly, what is the value of this information given that you don't know the relation oids in another database? So, I think if we will give up and limit access to the relations of the current database patch will become simpler and clearer. ------ Regards, Alexander Korotkov Supabase --000000000000548ec206204f671a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Wed, Aug 21, 2024 at 1:39=E2=80=AFAM Alena Rybakina <= ;a.rybakina@= postgrespro.ru> wrote:
>
> I think you've counted th= e above system tables from the database, but
> I'll double-check = it. Thank you for your review!
>
> On 19.08.2024 19:28, Ilia Ev= dokimov wrote:
> > Are you certain that all tables are included in=
> > `pg_stat_vacuum_tables`? I'm asking because of the follow= ing:
> >
> >
> > SELECT count(*) FROM pg_stat_al= l_tables ;
> > =C2=A0count
> > -------
> > =C2= =A0 =C2=A0108
> > (1 row)
> >
> > SELECT count(*= ) FROM pg_stat_vacuum_tables ;
> > =C2=A0count
> > ------= -
> > =C2=A0 =C2=A0 20
> > (1 row)
> >

I&= #39;d like to do some review a well.

+ =C2= =A0 MyDatabaseId =3D dbid;
+
+ =C2=A0 PG_TRY();
+ =C2=A0 {
+ = =C2=A0 =C2=A0 =C2=A0 tabentry =3D pgstat_fetch_stat_tabentry(relid);
+ = =C2=A0 =C2=A0 =C2=A0 MyDatabaseId =3D storedMyDatabaseId;
+ =C2=A0 }
= + =C2=A0 PG_CATCH();
+ =C2=A0 {
+ =C2=A0 =C2=A0 =C2=A0 MyDatabaseId = =3D storedMyDatabaseId;
+ =C2=A0 }
+ =C2=A0 PG_END_TRY();
<= br>I think this is generally wrong to change MyDatabaseId, especially if yo= u have to wrap it with PG_TRY()/PG_CATCH().=C2=A0 I think, instead we need = proper API changes, i.e. make pgstat_fetch_stat_tabentry() and others take = dboid as an argument.

+/*
+ * Get the va= cuum statistics for the heap tables.
+ */
+Datum
+pg_stat_vacuum_t= ables(PG_FUNCTION_ARGS)
+{
+ =C2=A0 return pg_stats_vacuum(fcinfo, PG= STAT_EXTVAC_HEAP, EXTVACHEAPSTAT_COLUMNS);
+
+ =C2=A0 PG_RETURN_NULL(= );
+}

The PG_RETURN_NULL() is unneeded after another retur= n statement.=C2=A0 However, does pg_stats_vacuum() need to return anything?= =C2=A0 What about making its return type void?

@@ -874,4 +874,38 @@ pgstat_get_custom_snapshot_data(PgStat_Kind kind)<= br>=C2=A0 =C2=A0return pgStatLocal.snapshot.custom_data[idx];
=C2=A0}=C2=A0
+/* hash table for statistics snapshots entry */
+typedef str= uct PgStat_SnapshotEntry
+{
+ =C2=A0PgStat_HashKey key;
+ =C2=A0ch= ar =C2=A0 =C2=A0 status; =C2=A0 =C2=A0 =C2=A0 =C2=A0/* for simplehash use *= /
+ =C2=A0void =C2=A0 =C2=A0 *data; =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* the s= tats data itself */
+} PgStat_SnapshotEntry;

It would be n= ice to preserve encapsulation and don't expose=C2=A0pgstat_snapshot has= h in the headers.=C2=A0 I see there is only one usage of it outside of pgst= at.c:=C2=A0pg_stats_vacuum().=C2=A0=C2=A0

+= =C2=A0 =C2=A0 =C2=A0 =C2=A0Oid =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0storedMyDatabaseId =3D MyDatabaseId;
+
+ =C2=A0 = =C2=A0 =C2=A0 =C2=A0pgstat_update_snapshot(PGSTAT_KIND_RELATION);
+ =C2= =A0 =C2=A0 =C2=A0 =C2=A0MyDatabaseId =3D storedMyDatabaseId;

= This manipulation with storedMyDatabaseId looks pretty useless.=C2=A0 It se= ems to be intended to change=C2=A0MyDatabaseId, while I'm not fan of th= is as I mentioned above.

+stati= c PgStat_StatTabEntry *
+fetch_dbstat_tabentry(Oid dbid, Oid relid)
+= {
+ =C2=A0Oid =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0storedMyDatabaseId =3D MyDatabaseId;
+ =C2=A0PgStat_StatTabEntry = =C2=A0*tabentry =3D NULL;
+
+ =C2=A0if (OidIsValid(CurrentDatabaseId)= && CurrentDatabaseId =3D=3D dbid)
+ =C2=A0 =C2=A0 /* Quick path= when we read data from the same database */
+ =C2=A0 =C2=A0 return pgst= at_fetch_stat_tabentry(relid);
+
+ =C2=A0pgstat_clear_snapshot();
=
It looks scary to reset the whole snapshot each time we access a= nother database.=C2=A0 Need to also mention that the CurrentDatabaseId mach= inery isn't implemented.

New functions pg_stat= _vacuum_tables(),=C2=A0pg_stat_vacuum_indexes(),=C2=A0pg_stat_vacuum_databa= se() are SRFs.=C2=A0 When zero Oid is passed they report all the objects.= =C2=A0 However, it seems they aren't intended to be used directly.=C2= =A0 Instead, there are views with the same names.=C2=A0 These views always = call them with particular Oids, therefore SRFs always return one row.=C2=A0= Then why bother with SRF?=C2=A0 They could return plain records instead.

Also, as I mentioned above patchset makes a lot of = trouble accessing statistics of relations of another database.=C2=A0 But th= at seems to be useless given corresponding views allow to see only relation= s of the current database.=C2=A0 Even if you call functions directly, what = is the value of this information given that you don't know the relation= oids in another database?=C2=A0 So, I think if we will give up and limit a= ccess to the relations of the current database patch will become simpler an= d clearer.

------
Regards,
Alexander Korotkov
Supaba= se
--000000000000548ec206204f671a--