From: | Alena Rybakina <a(dot)rybakina(at)postgrespro(dot)ru> |
---|---|
To: | jian he <jian(dot)universality(at)gmail(dot)com> |
Cc: | Alexander Korotkov <aekorotkov(at)gmail(dot)com>, Ilia Evdokimov <ilya(dot)evdokimov(at)tantorlabs(dot)com>, Andrei Zubkov <zubkov(at)moonset(dot)ru>, Alena Rybakina <lena(dot)ribackina(at)yandex(dot)ru>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, a(dot)lepikhov(at)postgrespro(dot)ru |
Subject: | Re: Vacuum statistics |
Date: | 2024-09-05 21:00:27 |
Message-ID: | 9b10c6d3-52c4-4eef-b67c-c33442667729@postgrespro.ru |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(dot)rybakina(at)postgrespro(dot)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.
Attachment | Content-Type | Size |
---|---|---|
v8-0001-Machinery-for-grabbing-an-extended-vacuum-statistics.patch | text/x-patch | 63.6 KB |
v8-0002-Machinery-for-grabbing-an-extended-vacuum-statistics.patch | text/x-patch | 40.3 KB |
v8-0003-Machinery-for-grabbing-an-extended-vacuum-statistics.patch | text/x-patch | 18.0 KB |
v8-0004-Add-documentation-about-the-system-views-that-are-us.patch | text/x-patch | 24.2 KB |
minor-vacuum.no-cbot | text/plain | 7.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Ilia Evdokimov | 2024-09-05 21:11:08 | Re: Adding NOTICE for differences between estimated and actual rows |
Previous Message | Euler Taveira | 2024-09-05 20:57:44 | Re: Adding NOTICE for differences between estimated and actual rows |