public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Michael Paquier <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Fix pgstat_database.c to honor passed database OIDs
Date: Fri, 10 Apr 2026 16:44:35 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
> On Apr 10, 2026, at 15:56, Michael Paquier <[email protected]> wrote:
>
> On Fri, Apr 10, 2026 at 03:12:41PM +0900, Michael Paquier wrote:
>> If we decide to expand pgstat_reset() in other contexts in the
>> back-branches, we'd be silently trapped as well.
>>
>> The connect and disconnect calls are less critical, perhaps we could
>> remove the argument altogether, but I cannot get excited about that
>> either as some extensions may rely on these as currently designed.
>>
>> I cannot look at that today, will do so later..
>
> - dbref = pgstat_get_entry_ref_locked(PGSTAT_KIND_DATABASE, MyDatabaseId, InvalidOid,
> + if (!OidIsValid(dboid))
> + return;
> +
> + dbref = pgstat_get_entry_ref_locked(PGSTAT_KIND_DATABASE, dboid, InvalidOid,
> false);
>
> This bypass of an invalid database OID is actually incorrect in the
> patch. There is a stats entry with a database OID of 0, documented as
> such in [1] for shared objects. There is one test in the main
> regression test suite that triggers this case:
> SELECT pg_stat_reset_single_table_counters('pg_shdescription'::regclass);
>
> The short answer is to remove this check based on OidIsValid(), and
> allow the timestamp be reset for this entry of 0 rather than ignore
> the update.
>
> [1]: https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-DATABASE-VIEW
> --
> Michael
Thanks for pointing out the test. I badly excluded *.sql and *.out in my vscode search last time.
Then the question becomes why the test didn't fail. I looked into it, and it seems the existing test does not check the stats_reset timestamp. I have now added checks for the stats_reset of both database 0 and the current database.
With those checks in place, the test fails without this patch, and also fails with the incorrect OidIsValid(dboid) check in v1. With the v2 patch, the test passes.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
[application/octet-stream] v2-0001-Fix-pgstat_database.c-to-honor-passed-database-OI.patch (4.6K, 2-v2-0001-Fix-pgstat_database.c-to-honor-passed-database-OI.patch)
download | inline diff:
From 39e558a01d6cea521fb76172fbda7fefb3bd5f09 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Fri, 10 Apr 2026 11:21:20 +0800
Subject: [PATCH v2] Fix pgstat_database.c to honor passed database OIDs
Several functions in pgstat_database.c take a database OID argument
but then ignore it and use MyDatabaseId instead. That is confusing,
and in pgstat_reset_database_timestamp() it is plainly wrong, because
the function's contract is to reset the timestamp for the specified
database.
Use the passed dboid in pgstat_report_connect(),
pgstat_report_disconnect(), and pgstat_reset_database_timestamp().
Author: Chao Li <[email protected]>
Reviewed-by: Michael Paquier <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/utils/activity/pgstat_database.c | 6 +++---
src/test/regress/expected/stats.out | 18 ++++++++++++++++++
src/test/regress/sql/stats.sql | 8 ++++++++
3 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/activity/pgstat_database.c b/src/backend/utils/activity/pgstat_database.c
index f1846d3236c..7f3bc016593 100644
--- a/src/backend/utils/activity/pgstat_database.c
+++ b/src/backend/utils/activity/pgstat_database.c
@@ -243,7 +243,7 @@ pgstat_report_connect(Oid dboid)
pgLastSessionReportTime = MyStartTimestamp;
- dbentry = pgstat_prep_database_pending(MyDatabaseId);
+ dbentry = pgstat_prep_database_pending(dboid);
dbentry->sessions++;
}
@@ -258,7 +258,7 @@ pgstat_report_disconnect(Oid dboid)
if (!pgstat_should_report_connstat())
return;
- dbentry = pgstat_prep_database_pending(MyDatabaseId);
+ dbentry = pgstat_prep_database_pending(dboid);
switch (pgStatSessionEndCause)
{
@@ -419,7 +419,7 @@ pgstat_reset_database_timestamp(Oid dboid, TimestampTz ts)
PgStat_EntryRef *dbref;
PgStatShared_Database *dbentry;
- dbref = pgstat_get_entry_ref_locked(PGSTAT_KIND_DATABASE, MyDatabaseId, InvalidOid,
+ dbref = pgstat_get_entry_ref_locked(PGSTAT_KIND_DATABASE, dboid, InvalidOid,
false);
dbentry = (PgStatShared_Database *) dbref->shared_stats;
diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out
index 55009cfcc7d..be2ee02bfa0 100644
--- a/src/test/regress/expected/stats.out
+++ b/src/test/regress/expected/stats.out
@@ -924,6 +924,10 @@ SELECT (n_tup_ins + n_tup_upd) > 0 AS has_data FROM pg_stat_all_tables
t
(1 row)
+SELECT coalesce(stats_reset::text, 'NULL') AS shared_db_reset_before
+ FROM pg_stat_database WHERE datid = 0 \gset
+SELECT coalesce(stats_reset::text, 'NULL') AS current_db_reset_before
+ FROM pg_stat_database WHERE datname = current_database() \gset
SELECT pg_stat_reset_single_table_counters('pg_shdescription'::regclass);
pg_stat_reset_single_table_counters
-------------------------------------
@@ -937,6 +941,20 @@ SELECT (n_tup_ins + n_tup_upd) > 0 AS has_data FROM pg_stat_all_tables
f
(1 row)
+SELECT coalesce(stats_reset::text, 'NULL') <> :'shared_db_reset_before' AS shared_db_reset_changed
+ FROM pg_stat_database WHERE datid = 0;
+ shared_db_reset_changed
+-------------------------
+ t
+(1 row)
+
+SELECT coalesce(stats_reset::text, 'NULL') = :'current_db_reset_before' AS current_db_reset_not_changed
+ FROM pg_stat_database WHERE datname = current_database();
+ current_db_reset_not_changed
+------------------------------
+ t
+(1 row)
+
-- set back comment
\if :{?description_before}
COMMENT ON DATABASE :"datname" IS :'description_before';
diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql
index 158f3ca6ebe..d4d371f6efa 100644
--- a/src/test/regress/sql/stats.sql
+++ b/src/test/regress/sql/stats.sql
@@ -416,9 +416,17 @@ COMMIT;
-- check that the stats are reset.
SELECT (n_tup_ins + n_tup_upd) > 0 AS has_data FROM pg_stat_all_tables
WHERE relid = 'pg_shdescription'::regclass;
+SELECT coalesce(stats_reset::text, 'NULL') AS shared_db_reset_before
+ FROM pg_stat_database WHERE datid = 0 \gset
+SELECT coalesce(stats_reset::text, 'NULL') AS current_db_reset_before
+ FROM pg_stat_database WHERE datname = current_database() \gset
SELECT pg_stat_reset_single_table_counters('pg_shdescription'::regclass);
SELECT (n_tup_ins + n_tup_upd) > 0 AS has_data FROM pg_stat_all_tables
WHERE relid = 'pg_shdescription'::regclass;
+SELECT coalesce(stats_reset::text, 'NULL') <> :'shared_db_reset_before' AS shared_db_reset_changed
+ FROM pg_stat_database WHERE datid = 0;
+SELECT coalesce(stats_reset::text, 'NULL') = :'current_db_reset_before' AS current_db_reset_not_changed
+ FROM pg_stat_database WHERE datname = current_database();
-- set back comment
\if :{?description_before}
--
2.50.1 (Apple Git-155)
view thread (7+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: Fix pgstat_database.c to honor passed database OIDs
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox