Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 25 Sep 2023 09:56:22 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Ryoga Yoshida <bt23yoshidar@oss.nttdata.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Doesn't pgstat_report_wal() handle the argument "force"
 incorrectly
Message-ID: <ZRDatnXJqlx46zV9@paquier.xyz>
References: <f87a4d7be70530606b864fd1df91718c@oss.nttdata.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Jp8p3Uy7gkuSfSUV"
Content-Disposition: inline
In-Reply-To: <f87a4d7be70530606b864fd1df91718c@oss.nttdata.com>
Archived-At: <https://www.postgresql.org/message-id/ZRDatnXJqlx46zV9%40paquier.xyz>
Precedence: bulk


--Jp8p3Uy7gkuSfSUV
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Sep 22, 2023 at 01:58:37PM +0900, Ryoga Yoshida wrote:
> pgstat_report_wal() calls pgstat_flush_wal() and pgstat_flush_io(). When
> calling them, pgstat_report_wal() specifies its argument "force" as the
> argument of them, as follows. But according to the code of
> pgstat_flush_wal() and pgstat_flush_io(), their argument is "nowait" and its
> meaning seems the opposite of "force". This means that, even when
> checkpointer etc calls pgstat_report_wal() with force=true to forcibly flush
> the statistics, pgstat_flush_wal() and pgstat_flush_io() skip flushing the
> statistics if they fail to acquire the lock immediately because they are
> called with nowait=true. This seems unexpected behavior and a bug.

It seems to me that you are right here.  It would make sense to me to
say that force=true is equivalent to nowait=false, as in "I'm OK to
wait on the lockas I want to make sure that the stats are flushed at
this point".  Currently force=true means nowait=true, as in "I'm OK to
not have the stats flushed if I cannot take the lock".

Seeing the three callers of pgstat_report_wal(), the checkpointer
wants to force its way twice, and the WAL writer does not care if they
are not flushed immediately at it loops forever in this path.

A comment at the top of pgstat_report_wal() would be nice to document
that a bit better, at least.
--
Michael

--Jp8p3Uy7gkuSfSUV
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmUQ2rYACgkQnvQgOdby
QH2YTw/+J5zagtF63uzPCQADdPkztCmcb6MU89ru/S0/HsJCD26y3RAISnFWAThh
UeNUzXCE/BcN5g4Yg+mcPOwyVCNZxrfvOgL5CNPsLkxumNWJ14bSoJILdSaprp5u
KXQ0JRxOBWXcB8OAOf/LESyu+tATor/wyZC+jqwxBoms8V4VXM3WuXEnFKsLiCKw
xdFr6a9tKLAef4NlKkkt1jigOvv10aNtUNgv3a0ExDuymqgeh7Vy2BQs1IH4/1pD
xNsJi14sQsRL2NPABDHMHPoilnKh+/+eR8FmjOIMJB3+cBybpAWX0JIgbv1/15nj
I6tpYY2AvLCz2OAzXJ5hL6Nkt9VWys9v0DKyc3UTBLvAa0yv8UMZUljCaM/y4hid
qeESFn3TeluZ3UAMT+dGWMnIQb2ruiFvSiJ66OE/CkgHseJ/SWZzUtI0zYEf0ntl
chZGjiKPDDtL5cW6dJZ6Rd04oVXxwl2XpJpbfUWDA44ffn8W+YtSBo1p6k+YI9Lh
WgRgsj61uuSsjgp3lzmvX8eK+zOIFwe8TNWbBgtAGsLqn0oUa8dcIfSgkNisAhkH
8Nfr6OXZ5CKsdIhUqV6QEs1VVSRTpebR3yu2vJaPepdqG29/tANA8OEpsAI+RS1B
yilyhi0BJbJy1J13U3sduS6K5HG0OdJZ6/irD+f/IV8W6S+28pA=
=xdzN
-----END PGP SIGNATURE-----

--Jp8p3Uy7gkuSfSUV--