public inbox for [email protected]
help / color / mirror / Atom feedFix pgstat_database.c to honor passed database OIDs
7+ messages / 3 participants
[nested] [flat]
* Fix pgstat_database.c to honor passed database OIDs
@ 2026-04-10 05:53 Chao Li <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Chao Li @ 2026-04-10 05:53 UTC (permalink / raw)
To: pgsql-hackers
Hi,
In pgstat_database.c, pgstat_report_connect(), pgstat_report_disconnect(), and pgstat_reset_database_timestamp() all take a dboid parameter, but currently ignore it and use MyDatabaseId instead. While that does not seem to break anything today, it at least hurts readability.
This patch changes those three functions to use the passed dboid.
For pgstat_report_connect() and pgstat_report_disconnect(), there is only one caller, and it passes MyDatabaseId, so this change should be safe.
For pgstat_reset_database_timestamp(), in most paths dboid is also just MyDatabaseId. However, there is one path where dboid can be InvalidOid:
1 - pg_stat_reset_single_table_counters may pass InvalidOid to pgstat_reset for a shared relation.
```
Datum
pg_stat_reset_single_table_counters(PG_FUNCTION_ARGS)
{
Oid taboid = PG_GETARG_OID(0);
Oid dboid = (IsSharedRelation(taboid) ? InvalidOid : MyDatabaseId);
pgstat_reset(PGSTAT_KIND_RELATION, dboid, taboid);
PG_RETURN_VOID();
}
```
2 - pgstat_reset only calls pgstat_reset_database_timestamp when kind_info->accessed_across_databases is false.
```
void
pgstat_reset(PgStat_Kind kind, Oid dboid, uint64 objid)
{
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
TimestampTz ts = GetCurrentTimestamp();
/* not needed atm, and doesn't make sense with the current signature */
Assert(!pgstat_get_kind_info(kind)->fixed_amount);
/* reset the "single counter" */
pgstat_reset_entry(kind, dboid, objid, ts);
if (!kind_info->accessed_across_databases)
pgstat_reset_database_timestamp(dboid, ts);
}
```
3 - In this path, kind is PGSTAT_KIND_RELATION, and accessed_across_databases is false:
```
[PGSTAT_KIND_RELATION] = {
.name = "relation",
.fixed_amount = false,
.write_to_file = true,
.shared_size = sizeof(PgStatShared_Relation),
.shared_data_off = offsetof(PgStatShared_Relation, stats),
.shared_data_len = sizeof(((PgStatShared_Relation *) 0)->stats),
.pending_size = sizeof(PgStat_TableStatus),
.flush_pending_cb = pgstat_relation_flush_cb,
.delete_pending_cb = pgstat_relation_delete_pending_cb,
.reset_timestamp_cb = pgstat_relation_reset_timestamp_cb,
},
```
So in this case, pgstat_reset_database_timestamp() can receive InvalidOid as dboid. In the current code, because that function ignores dboid and uses MyDatabaseId, calling pg_stat_reset_single_table_counters() on a shared relation can incorrectly reset the current database's stat_reset_timestamp. That behavior seems unintended, so this patch makes pgstat_reset_database_timestamp() return immediately when dboid is InvalidOid.
Please see the attached patch. “Make check-world” passed from my side.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
[application/octet-stream] v1-0001-Fix-pgstat_database.c-to-honor-passed-database-OI.patch (2.1K, 2-v1-0001-Fix-pgstat_database.c-to-honor-passed-database-OI.patch)
download | inline diff:
From 8e965bb55562a675d17e37de8b20fe17fe4ec452 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Fri, 10 Apr 2026 11:21:20 +0800
Subject: [PATCH v1] 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().
Also make pgstat_reset_database_timestamp() return immediately when
called with an invalid OID, avoiding an unnecessary stats lookup.
Author: Chao Li <[email protected]>
Reviewed-by:
Discussion: https://postgr.es/m/
---
src/backend/utils/activity/pgstat_database.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/activity/pgstat_database.c b/src/backend/utils/activity/pgstat_database.c
index f1846d3236c..36a84d1f0a4 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,10 @@ 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,
+ if (!OidIsValid(dboid))
+ return;
+
+ dbref = pgstat_get_entry_ref_locked(PGSTAT_KIND_DATABASE, dboid, InvalidOid,
false);
dbentry = (PgStatShared_Database *) dbref->shared_stats;
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix pgstat_database.c to honor passed database OIDs
@ 2026-04-10 06:12 Michael Paquier <[email protected]>
parent: Chao Li <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Michael Paquier @ 2026-04-10 06:12 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: pgsql-hackers
On Fri, Apr 10, 2026 at 01:53:15PM +0800, Chao Li wrote:
> For pgstat_reset_database_timestamp(), in most paths dboid is also
> just MyDatabaseId. However, there is one path where dboid can be
> InvalidOid:
The call of pgstat_reset_database_timestamp() in pgstat_reset() is a
bug that has to be backpatched down to v15. It does not make sense to
let a caller of pgstat_reset() pass down a custom dboid and then
decide to reset the timestamp of MyDatabaseId instead. The call of
pgstat_reset() in pgstat_create_transactional() is the only fishy one,
the other callers are OK.
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..
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix pgstat_database.c to honor passed database OIDs
@ 2026-04-10 07:56 Michael Paquier <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Michael Paquier @ 2026-04-10 07:56 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: pgsql-hackers
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
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix pgstat_database.c to honor passed database OIDs
@ 2026-04-10 08:44 Chao Li <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Chao Li @ 2026-04-10 08:44 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers
> 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)
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix pgstat_database.c to honor passed database OIDs
@ 2026-04-10 13:01 Dapeng Wang <[email protected]>
parent: Chao Li <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Dapeng Wang @ 2026-04-10 13:01 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers
Chao Li <[email protected]> 于2026年4月10日周五 16:45写道：
>
>
> > 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/
>
>
Hi Evan,
I tested the v2 patch on HEAD. It applies cleanly, compiles
without warnings, and all 247 regression tests pass.
I also manually verified the bug fix by comparing behavior
before and after the patch:
Without the patch, calling
pg_stat_reset_single_table_counters('pg_shdescription'::regclass)
incorrectly updates the current database's stats_reset timestamp
while leaving the shared db entry (datid=0) unchanged.
With the patch, the shared db entry's stats_reset is correctly
updated, and the current database's timestamp is not affected.
Regards,
Dapeng Wang
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix pgstat_database.c to honor passed database OIDs
@ 2026-04-13 00:16 Michael Paquier <[email protected]>
parent: Dapeng Wang <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Michael Paquier @ 2026-04-13 00:16 UTC (permalink / raw)
To: Dapeng Wang <[email protected]>; +Cc: Chao Li <[email protected]>; pgsql-hackers
On Fri, Apr 10, 2026 at 09:01:32PM +0800, Dapeng Wang wrote:
> Without the patch, calling
> pg_stat_reset_single_table_counters('pg_shdescription'::regclass)
> incorrectly updates the current database's stats_reset timestamp
> while leaving the shared db entry (datid=0) unchanged.
>
> With the patch, the shared db entry's stats_reset is correctly
> updated, and the current database's timestamp is not affected.
The coalesce() trick to bypass the fact that the reset timestamp may
not be reset was a bit ugly, so I have used instead a second reset.
I have limited the test to check for datid=0, not MyDatabaseId.
There is a bit down in stats.sql an extra portion of the test where we
use twice pg_stats_reset(). One reset could be removed, but I have
left it as-is in case someone decides to shuffle or split things in
this test script, to avoid problems in the future.
And fixed that down to v15.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix pgstat_database.c to honor passed database OIDs
@ 2026-04-13 00:43 Chao Li <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Chao Li @ 2026-04-13 00:43 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Dapeng Wang <[email protected]>; pgsql-hackers
> On Apr 13, 2026, at 08:16, Michael Paquier <[email protected]> wrote:
>
> On Fri, Apr 10, 2026 at 09:01:32PM +0800, Dapeng Wang wrote:
>> Without the patch, calling
>> pg_stat_reset_single_table_counters('pg_shdescription'::regclass)
>> incorrectly updates the current database's stats_reset timestamp
>> while leaving the shared db entry (datid=0) unchanged.
>>
>> With the patch, the shared db entry's stats_reset is correctly
>> updated, and the current database's timestamp is not affected.
>
> The coalesce() trick to bypass the fact that the reset timestamp may
> not be reset was a bit ugly, so I have used instead a second reset.
> I have limited the test to check for datid=0, not MyDatabaseId.
>
> There is a bit down in stats.sql an extra portion of the test where we
> use twice pg_stats_reset(). One reset could be removed, but I have
> left it as-is in case someone decides to shuffle or split things in
> this test script, to avoid problems in the future.
>
> And fixed that down to v15.
> --
> Michael
Thank you very much for fixing the test and pushing the patch.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-04-13 00:43 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-04-10 05:53 Fix pgstat_database.c to honor passed database OIDs Chao Li <[email protected]>
2026-04-10 06:12 ` Michael Paquier <[email protected]>
2026-04-10 07:56 ` Michael Paquier <[email protected]>
2026-04-10 08:44 ` Chao Li <[email protected]>
2026-04-10 13:01 ` Dapeng Wang <[email protected]>
2026-04-13 00:16 ` Michael Paquier <[email protected]>
2026-04-13 00:43 ` Chao Li <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox