Re: Vacuum statistics

Hi! Thank you for your review!

On 05.09.2024 15:47, jian he wrote:
> On Thu, Sep 5, 2024 at 1:23 AM Alena Rybakina<a.rybakina@postgrespro.ru>  wrote:
>> Hi, all!
>>
>> I have attached the new version of the code and the diff files
>> (minor-vacuum.no-cbot).
>>
> hi.
>
> still have white space issue when using "git apply",
> you may need to use "git diff --check" to find out where.
>
>
>   /* ----------
> diff --git a/src/test/regress/expected/opr_sanity.out
> b/src/test/regress/expected/opr_sanity.out
> index 5d72b970b03..7026de157e4 100644
> --- a/src/test/regress/expected/opr_sanity.out
> +++ b/src/test/regress/expected/opr_sanity.out
> @@ -32,11 +32,12 @@ WHERE p1.prolang = 0 OR p1.prorettype = 0 OR
>          prokind NOT IN ('f', 'a', 'w', 'p') OR
>          provolatile NOT IN ('i', 's', 'v') OR
>          proparallel NOT IN ('s', 'r', 'u');
> - oid  |        proname
> -------+------------------------
> + oid  |         proname
> +------+-------------------------
>    8001 | pg_stat_vacuum_tables
>    8002 | pg_stat_vacuum_indexes
> -(2 rows)
> + 8003 | pg_stat_vacuum_database
> +(3 rows)
>
>
> looking at src/test/regress/sql/opr_sanity.sql:
>
> -- **************** pg_proc ****************
> -- Look for illegal values in pg_proc fields.
>
> SELECT p1.oid, p1.proname
> FROM pg_proc as p1
> WHERE p1.prolang = 0 OR p1.prorettype = 0 OR
>         p1.pronargs < 0 OR
>         p1.pronargdefaults < 0 OR
>         p1.pronargdefaults > p1.pronargs OR
>         array_lower(p1.proargtypes, 1) != 0 OR
>         array_upper(p1.proargtypes, 1) != p1.pronargs-1 OR
>         0::oid = ANY (p1.proargtypes) OR
>         procost <= 0 OR
>         CASE WHEN proretset THEN prorows <= 0 ELSE prorows != 0 END OR
>         prokind NOT IN ('f', 'a', 'w', 'p') OR
>         provolatile NOT IN ('i', 's', 'v') OR
>         proparallel NOT IN ('s', 'r', 'u');
>
> that means
>   oid  |         proname
> ------+-------------------------
>   8001 | pg_stat_vacuum_tables
>   8002 | pg_stat_vacuum_indexes
>   8003 | pg_stat_vacuum_database
>
>
> These above functions, pg_proc.prorows should > 0 when
> pg_proc.proretset is true.
> I think that's the opr_sanity test's intention.
> so you may need to change pg_proc.dat.
>
> BTW the doc says:
> prorows float4, Estimated number of result rows (zero if not proretset)
>
I agree with you and have fixed it.
> segmentation fault cases:
> select * from pg_stat_vacuum_indexes(0);
> select * from pg_stat_vacuum_tables(0);
>
>
> + else if (type == PGSTAT_EXTVAC_DB)
> + {
> + PgStatShared_Database   *dbentry;
> + PgStat_EntryRef   *entry_ref;
> + Oid dbid = PG_GETARG_OID(0);
> +
> + if (OidIsValid(dbid))
> + {
> + entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_DATABASE,
> + dbid, InvalidOid, false);
> + dbentry = (PgStatShared_Database *) entry_ref->shared_stats;
> +
> + if (dbentry == NULL)
> + /* Table doesn't exist or isn't a heap relation */
> + return;
> +
> + tuplestore_put_for_database(dbid, rsinfo, dbentry);
> + pgstat_unlock_entry(entry_ref);
> + }
> + }
> didn't error out when dbid is invalid?
>
It is caused by the empty statistic snapshot. I have fixed that by 
updating the snapshot (pgstat_update_snapshot(PGSTAT_KIND_RELATION) 
function).

I also added the test to check it.

> pg_stat_vacuum_tables
> pg_stat_vacuum_indexes
> pg_stat_vacuum_database
> these functions didn't verify the only input argument oid's kind.
> for example:
>
> create table s(a int primary key) with (autovacuum_enabled = off);
> create view sv as select * from s;
> vacuum s;
> select * from pg_stat_vacuum_tables('sv'::regclass::oid);
> select * from pg_stat_vacuum_indexes('sv'::regclass::oid);
> select * from pg_stat_vacuum_database('sv'::regclass::oid);
>
> above all these 3 examples should error out? because  sv is view.

I don't think so. I noticed that if we try to find the object from the 
system table with the different type the Postgres returns empty rows. I 
think we should do the same.

> in src/backend/catalog/system_views.sql
> for view creation of pg_stat_vacuum_indexes
> you can change to
>
> WHERE
>    db.datname = current_database() AND
>    rel.oid = stats.relid AND
>    ns.oid = rel.relnamespace
> AND rel.relkind = 'i':
>
>
>
> pg_stat_vacuum_tables  in in src/backend/catalog/system_views.sql
> you can change to
>
> WHERE
>    db.datname = current_database() AND
>    rel.oid = stats.relid AND
>    ns.oid = rel.relnamespace
> AND rel.relkind = 'r':
>
I agree with your proposal and fixed it like that.