MIME-Version: 1.0
References: <ABBD5026-506F-4006-A569-28F72C188693@gmail.com>
 <adiU2YZ0auyO6jie@paquier.xyz> <aditMVNDceHtvYj4@paquier.xyz>
 <8CD3DF77-C4F0-4CA0-B329-2B62B5A85E3B@gmail.com>
In-Reply-To: <8CD3DF77-C4F0-4CA0-B329-2B62B5A85E3B@gmail.com>
From: Dapeng Wang <wangdp20191008@gmail.com>
Date: Fri, 10 Apr 2026 21:01:32 +0800
Message-ID: 
 <CAKx0YhzpuMkVwxa-fh36YOk11KZ6tjXnApAF=H6Sgso5a+P4Zg@mail.gmail.com>
Subject: Re: Fix pgstat_database.c to honor passed database OIDs
To: Chao Li <li.evan.chao@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary="0000000000002565e4064f1abc7c"
Archived-At: 
 <https://www.postgresql.org/message-id/CAKx0YhzpuMkVwxa-fh36YOk11KZ6tjXnApAF%3DH6Sgso5a%2BP4Zg%40mail.gmail.com>
Precedence: bulk

--0000000000002565e4064f1abc7c
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Chao Li <li.evan.chao@gmail.com> =E4=BA=8E2026=E5=B9=B44=E6=9C=8810=E6=97=
=A5=E5=91=A8=E4=BA=94 16:45=E5=86=99=E9=81=93=EF=BC=9A

>
>
> > On Apr 10, 2026, at 15:56, Michael Paquier <michael@paquier.xyz> 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 =3D pgstat_get_entry_ref_locked(PGSTAT_KIND_DATABASE,
> MyDatabaseId, InvalidOid,
> > +    if (!OidIsValid(dboid))
> > +        return;
> > +
> > +    dbref =3D 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 ha=
ve
> 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=3D0) 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

--0000000000002565e4064f1abc7c
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Chao Li &lt;<a href=3D"mailto:li.evan.cha=
o@gmail.com">li.evan.chao@gmail.com</a>&gt; =E4=BA=8E2026=E5=B9=B44=E6=9C=
=8810=E6=97=A5=E5=91=A8=E4=BA=94 16:45=E5=86=99=E9=81=93=EF=BC=9A</div><div=
 class=3D"gmail_quote gmail_quote_container"><blockquote class=3D"gmail_quo=
te" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204=
);padding-left:1ex"><br>
<br>
&gt; On Apr 10, 2026, at 15:56, Michael Paquier &lt;<a href=3D"mailto:micha=
el@paquier.xyz" target=3D"_blank">michael@paquier.xyz</a>&gt; wrote:<br>
&gt; <br>
&gt; On Fri, Apr 10, 2026 at 03:12:41PM +0900, Michael Paquier wrote:<br>
&gt;&gt; If we decide to expand pgstat_reset() in other contexts in the<br>
&gt;&gt; back-branches, we&#39;d be silently trapped as well.<br>
&gt;&gt; <br>
&gt;&gt; The connect and disconnect calls are less critical, perhaps we cou=
ld<br>
&gt;&gt; remove the argument altogether, but I cannot get excited about tha=
t<br>
&gt;&gt; either as some extensions may rely on these as currently designed.=
<br>
&gt;&gt; <br>
&gt;&gt; I cannot look at that today, will do so later..<br>
&gt; <br>
&gt; -=C2=A0 =C2=A0 dbref =3D pgstat_get_entry_ref_locked(PGSTAT_KIND_DATAB=
ASE, MyDatabaseId, InvalidOid,<br>
&gt; +=C2=A0 =C2=A0 if (!OidIsValid(dboid))<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return;<br>
&gt; +<br>
&gt; +=C2=A0 =C2=A0 dbref =3D pgstat_get_entry_ref_locked(PGSTAT_KIND_DATAB=
ASE, dboid, InvalidOid,<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0false);<br>
&gt; <br>
&gt; This bypass of an invalid database OID is actually incorrect in the<br=
>
&gt; patch.=C2=A0 There is a stats entry with a database OID of 0, document=
ed as<br>
&gt; such in [1] for shared objects.=C2=A0 There is one test in the main<br=
>
&gt; regression test suite that triggers this case:<br>
&gt; SELECT pg_stat_reset_single_table_counters(&#39;pg_shdescription&#39;:=
:regclass);<br>
&gt; <br>
&gt; The short answer is to remove this check based on OidIsValid(), and<br=
>
&gt; allow the timestamp be reset for this entry of 0 rather than ignore<br=
>
&gt; the update.<br>
&gt; <br>
&gt; [1]: <a href=3D"https://www.postgresql.org/docs/devel/monitoring-stats=
.html#MONITORING-PG-STAT-DATABASE-VIEW" rel=3D"noreferrer" target=3D"_blank=
">https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG=
-STAT-DATABASE-VIEW</a><br>
&gt; --<br>
&gt; Michael<br>
<br>
Thanks for pointing out the test. I badly excluded *.sql and *.out in my vs=
code search last time.<br>
<br>
Then the question becomes why the test didn&#39;t fail. I looked into it, a=
nd it seems the existing test does not check the stats_reset timestamp. I h=
ave now added checks for the stats_reset of both database 0 and the current=
 database.<br>
<br>
With those checks in place, the test fails without this patch, and also fai=
ls with the incorrect OidIsValid(dboid) check in v1. With the v2 patch, the=
 test passes.<br>
<br>
Best regards,<br>
--<br>
Chao Li (Evan)<br>
HighGo Software Co., Ltd.<br>
<a href=3D"https://www.highgo.com/" rel=3D"noreferrer" target=3D"_blank">ht=
tps://www.highgo.com/</a><br>
<br></blockquote><div><br></div>Hi Evan,<br><br>I tested the v2 patch on HE=
AD. It applies cleanly, compiles<br>without warnings, and all 247 regressio=
n tests pass.<br><br>I also manually verified the bug fix by comparing beha=
vior<br>before and after the patch:<br><br>Without the patch, calling<br>pg=
_stat_reset_single_table_counters(&#39;pg_shdescription&#39;::regclass)<br>=
incorrectly updates the current database&#39;s stats_reset timestamp<br>whi=
le leaving the shared db entry (datid=3D0) unchanged.<br><br>With the patch=
, the shared db entry&#39;s stats_reset is correctly<br>updated, and the cu=
rrent database&#39;s timestamp is not affected.<br><br>Regards,<br><div>Dap=
eng Wang=C2=A0</div></div></div>

--0000000000002565e4064f1abc7c--