Обсуждение: Fix pg_stat_reset_single_table_counters function
Hi, My colleague, Mitsuru Hinata (in CC), found the following issue. The documentation of pg_stat_reset_single_table_counters() says > pg_stat_reset_single_table_counters ( oid ) → void > Resets statistics for a single table or index in the current database > or shared across all databases in the cluster to zero. > This function is restricted to superusers by default, but other users > can be granted EXECUTE to run the function. https://www.postgresql.org/docs/devel/monitoring-stats.html But, the stats will not be reset if the table shared across all databases is specified. IIUC, 5891c7a8e seemed to have mistakenly removed the feature implemented in e04267844. What do you think? * reproduce procedure SELECT COUNT(*) FROM pg_stat_database; SELECT pg_stat_reset_single_table_counters('pg_database'::regclass); SELECT seq_scan FROM pg_stat_all_tables WHERE relid = 'pg_database'::regclass; * unexpected result * Rename OverrideSearchPath to SearchPathMatcher (current HEAD: d3a38318a) * pgstat: store statistics in shared memory (5891c7a8e) psql=# SELECT seq_scan FROM pg_stat_all_tables WHERE relid = 'pg_database'::regclass; seq_scan ---------- 11 (1 row) * expected result * Enhance pg_stat_reset_single_table_counters function (e04267844) * pgstat: normalize function naming (be902e2651), which is previous commit of 5891c7a8e. psql=# SELECT seq_scan FROM pg_stat_all_tables WHERE relid = 'pg_database'::regclass; seq_scan ---------- 0 (1 row) Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Вложения
On 2023-08-01 15:23, Masahiro Ikeda wrote: > Hi, > > My colleague, Mitsuru Hinata (in CC), found the following issue. > > The documentation of pg_stat_reset_single_table_counters() says >> pg_stat_reset_single_table_counters ( oid ) → void >> Resets statistics for a single table or index in the current database >> or shared across all databases in the cluster to zero. >> This function is restricted to superusers by default, but other users >> can be granted EXECUTE to run the function. > https://www.postgresql.org/docs/devel/monitoring-stats.html > > But, the stats will not be reset if the table shared across all > databases is specified. IIUC, 5891c7a8e seemed to have mistakenly > removed the feature implemented in e04267844. What do you think? I fix an issue with the v1 patch reported by cfbot. The test case didn't assume the number of databases will change in the test of pg_upgrade. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Вложения
On Thu, Aug 10, 2023 at 2:10 PM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote: > > On 2023-08-01 15:23, Masahiro Ikeda wrote: > > Hi, > > > > My colleague, Mitsuru Hinata (in CC), found the following issue. > > > > The documentation of pg_stat_reset_single_table_counters() says > >> pg_stat_reset_single_table_counters ( oid ) → void > >> Resets statistics for a single table or index in the current database > >> or shared across all databases in the cluster to zero. > >> This function is restricted to superusers by default, but other users > >> can be granted EXECUTE to run the function. > > https://www.postgresql.org/docs/devel/monitoring-stats.html > > > > But, the stats will not be reset if the table shared across all > > databases is specified. IIUC, 5891c7a8e seemed to have mistakenly > > removed the feature implemented in e04267844. What do you think? Good catch! I've confirmed that the issue has been fixed by your patch. However, I'm not sure the added regression tests are stable since autovacuum workers may scan the pg_database and increment the statistics after resetting the stats. Also, the patch bumps the CATALOG_VERSION_NO but I don't think it's necessary. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Hi, On 2023-08-10 17:48:10 +0900, Masahiko Sawada wrote: > Good catch! I've confirmed that the issue has been fixed by your patch. Indeed. > However, I'm not sure the added regression tests are stable since > autovacuum workers may scan the pg_database and increment the > statistics after resetting the stats. What about updating the table and checking the update count is reset? That'd not be reset by autovacuum. Greetings, Andres Freund
Hi, On 2023-08-13 04:12, Andres Freund wrote: > On 2023-08-10 17:48:10 +0900, Masahiko Sawada wrote: >> Good catch! I've confirmed that the issue has been fixed by your >> patch. > > Indeed. Thanks for your responses! >> However, I'm not sure the added regression tests are stable since >> autovacuum workers may scan the pg_database and increment the >> statistics after resetting the stats. > > What about updating the table and checking the update count is reset? > That'd > not be reset by autovacuum. Yes. I confirmed that the stats are incremented by autovacuum as you said. I updated the patch to v3. * remove the code to bump the CATALOG_VERSION_NO because I misunderstood * change the test logic to check the update count instead of scan count I changed the table to check the stats from pg_database to pg_shdescription because the stats can update via the SQL interface COMMENT command. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Вложения
Hi, On Mon, Aug 14, 2023 at 5:12 PM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote: > > Hi, > > On 2023-08-13 04:12, Andres Freund wrote: > > On 2023-08-10 17:48:10 +0900, Masahiko Sawada wrote: > >> Good catch! I've confirmed that the issue has been fixed by your > >> patch. > > > > Indeed. > > Thanks for your responses! > > >> However, I'm not sure the added regression tests are stable since > >> autovacuum workers may scan the pg_database and increment the > >> statistics after resetting the stats. > > > > What about updating the table and checking the update count is reset? > > That'd > > not be reset by autovacuum. > > Yes. I confirmed that the stats are incremented by autovacuum as you > said. > > I updated the patch to v3. > * remove the code to bump the CATALOG_VERSION_NO because I misunderstood > * change the test logic to check the update count instead of scan count Thank you for updating the patch! > > I changed the table to check the stats from pg_database to > pg_shdescription > because the stats can update via the SQL interface COMMENT command. It seems to work well. +COMMENT ON DATABASE :current_database IS 'This is a test comment'; -- insert or update in 'pg_shdescription' I think the current_database should be quoted (see other examples where using current_database(), e.g. collate.linux.utf8.sql). Also it would be better to reset the comment after the test. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
On 2023-08-15 11:48, Masahiko Sawada wrote: > On Mon, Aug 14, 2023 at 5:12 PM Masahiro Ikeda > <ikedamsh@oss.nttdata.com> wrote: >> I changed the table to check the stats from pg_database to >> pg_shdescription >> because the stats can update via the SQL interface COMMENT command. > > It seems to work well. > > +COMMENT ON DATABASE :current_database IS 'This is a test comment'; > -- insert or update in 'pg_shdescription' > > I think the current_database should be quoted (see other examples > where using current_database(), e.g. collate.linux.utf8.sql). Also it > would be better to reset the comment after the test. Thanks! I fixed the issues in the v4 patch. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Вложения
Apologies. In the previous mail, I mistakenly addressed it to the wrong recipients. Reposted. On 2023/08/15 14:13, Masahiro Ikeda wrote: > On 2023-08-15 11:48, Masahiko Sawada wrote: >> +COMMENT ON DATABASE :current_database IS 'This is a test comment'; >> -- insert or update in 'pg_shdescription' >> >> I think the current_database should be quoted (see other examples >> where using current_database(), e.g. collate.linux.utf8.sql). Also it >> would be better to reset the comment after the test. > > Thanks! I fixed the issues in the v4 patch. I'm not sure about others, but I would avoid using the name "current_database" for the variable. I would just use "database" or "db" instead. -- Kyotaro Horiguchi NTT Open Source Software Center
On 2023-08-21 11:33, Kyotaro Horiguchi wrote: > On 2023/08/15 14:13, Masahiro Ikeda wrote: >> On 2023-08-15 11:48, Masahiko Sawada wrote: >>> +COMMENT ON DATABASE :current_database IS 'This is a test comment'; >>> -- insert or update in 'pg_shdescription' >>> >>> I think the current_database should be quoted (see other examples >>> where using current_database(), e.g. collate.linux.utf8.sql). Also it >>> would be better to reset the comment after the test. >> > > I'm not sure about others, but I would avoid using the name > "current_database" for the variable. > > I would just use "database" or "db" instead. Thanks! I agree your comment. I fixed in v5 patch. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Вложения
On Mon, Aug 21, 2023 at 11:33:28AM +0900, Kyotaro Horiguchi wrote: > I'm not sure about others, but I would avoid using the name > "current_database" for the variable. Agreed. Looking at the surroundings, we have for example "datname" in the collation tests so I have just used that, and applied the patch down to 15. -- Michael
Вложения
On 2023-08-21 13:34, Michael Paquier wrote: > On Mon, Aug 21, 2023 at 11:33:28AM +0900, Kyotaro Horiguchi wrote: >> I'm not sure about others, but I would avoid using the name >> "current_database" for the variable. > > Agreed. Looking at the surroundings, we have for example "datname" in > the collation tests so I have just used that, and applied the patch > down to 15. Thanks! Regards, -- Masahiro Ikeda NTT DATA CORPORATION