Fix pg_stat_reset_single_table_counters function

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