Content-Type: multipart/alternative;
 boundary="------------WCtZjXIpzJpXYWv0PIJeg2cK"
Message-ID: <53c47c2d-72a5-44f2-900c-9973b2af1808@tantorlabs.com>
Date: Tue, 13 Aug 2024 16:18:48 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Vacuum statistics
To: Andrei Zubkov <zubkov@moonset.ru>,
 Alena Rybakina <lena.ribackina@yandex.ru>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>, a.lepikhov@postgrespro.ru
References: <bee34abe-fe87-49b3-8523-8ae19e641ccf@yandex.ru>
 <ff99283d-f9d4-4b37-b4fa-3a433fb408a4@tantorlabs.com>
 <e1f5b560691a4fe693625d529e19e13959210568.camel@moonset.ru>
Content-Language: en-US
From: Ilia Evdokimov <ilya.evdokimov@tantorlabs.com>
In-Reply-To: <e1f5b560691a4fe693625d529e19e13959210568.camel@moonset.ru>
Archived-At: <https://www.postgresql.org/message-id/53c47c2d-72a5-44f2-900c-9973b2af1808%40tantorlabs.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------WCtZjXIpzJpXYWv0PIJeg2cK
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 10.8.24 22:37, Andrei Zubkov wrote:

> Hi, Ilia!
>
>> Do you consider not to create new table in pg_catalog but to save
>> statistics in existing table? I mean pg_class or
>> pg_stat_progress_analyze, pg_stat_progress_vacuum?
>>
> Thank you for your interest on our patch!
>
> *_progress views is not our case. They hold online statistics while
> vacuum is in progress. Once work is done on a table the entry is gone
> from those views. Idea of this patch is the opposite - it doesn't
> provide online statistics but it accumulates statistics about rosources
> consumed by all vacuum passes over all relations. It's much closer to
> the pg_stat_all_tables than pg_stat_progress_vacuum.
>
> It seems pg_class is not the right place because it is not a statistic
> view - it holds the current relation state and haven't anything about
> the relation workload.
>
> Maybe the pg_stat_all_tables is the right place but I have several
> thoughts about why it is not:
> - Some statistics provided by this patch is really vacuum specific. I
> don't think we want them in the relation statistics view.
> - Postgres is extreamly extensible. I'm sure someday there will be
> table AMs that does not need the vacuum at all.
>
> Right now vacuum specific workload views seems optimal choice to me.
>
> Regards,


Agreed. They are not god places to store such statistics.


I have some suggestions:

 1. pgstatfuncs.c in functions tuplestore_put_for_database() and
    tuplestore_put_for_relation you can remove 'nulls' array if you're
    sure that columns cannot be NULL.
 2. These functions are almost the same and I would think of writing one
    function depending of type 'ExtVacReportType'

--------------WCtZjXIpzJpXYWv0PIJeg2cK
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 7bit

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p>On 10.8.24 22:37, Andrei Zubkov wrote:</p>
    <blockquote type="cite"
cite="mid:e1f5b560691a4fe693625d529e19e13959210568.camel@moonset.ru">
      <pre class="moz-quote-pre" wrap="">
Hi, Ilia!

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">Do you consider not to create new table in pg_catalog but to save 
statistics in existing table? I mean pg_class or 
pg_stat_progress_analyze, pg_stat_progress_vacuum?

</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">Thank you for your interest on our patch!

*_progress views is not our case. They hold online statistics while
vacuum is in progress. Once work is done on a table the entry is gone
from those views. Idea of this patch is the opposite - it doesn't
provide online statistics but it accumulates statistics about rosources
consumed by all vacuum passes over all relations. It's much closer to
the pg_stat_all_tables than pg_stat_progress_vacuum.

It seems pg_class is not the right place because it is not a statistic
view - it holds the current relation state and haven't anything about
the relation workload.

Maybe the pg_stat_all_tables is the right place but I have several
thoughts about why it is not:
- Some statistics provided by this patch is really vacuum specific. I
don't think we want them in the relation statistics view.
- Postgres is extreamly extensible. I'm sure someday there will be
table AMs that does not need the vacuum at all.

Right now vacuum specific workload views seems optimal choice to me.

Regards,
</pre>
    </blockquote>
    <p><br>
    </p>
    <p>Agreed. They are not god places to store such statistics.</p>
    <p><br>
    </p>
    <p>I have some suggestions:</p>
    <ol>
      <li>pgstatfuncs.c in functions tuplestore_put_for_database() and
        tuplestore_put_for_relation you can remove 'nulls' array if
        you're sure that columns cannot be NULL. <br>
      </li>
      <li>These functions are almost the same and I would think of
        writing one function depending of type 'ExtVacReportType'</li>
    </ol>
  </body>
</html>

--------------WCtZjXIpzJpXYWv0PIJeg2cK--