public inbox for [email protected]  

help / color / mirror / Atom feed

From: Ryoga Yoshida <[email protected]>
To: Michael Paquier <[email protected]>
Cc: [email protected]
Subject: Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly
Date: Mon, 25 Sep 2023 11:27:27 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>

On 2023-09-25 09:56, Michael Paquier wrote:
> 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.

Thank you for the review. Certainly, adding a comments is a good idea. I 
added a comment.

Ryoga Yoshida

Attachments:

  [text/x-diff] v1-0002-bug-fix-in-foce-within-pgstat_report_wal.patch (804B, ../[email protected]/2-v1-0002-bug-fix-in-foce-within-pgstat_report_wal.patch)
  download | inline diff:
diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index bcaed14d02..6f3bac5d0f 100644
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -38,13 +38,18 @@ static WalUsage prevWalUsage;
  *
  * Must be called by processes that generate WAL, that do not call
  * pgstat_report_stat(), like walwriter.
+ *
+ * force=false is the same as nowait=true, and the statistics will
+ * not be flushed if the lock cannot be acquired.
+ * force=true is the same as nowait=false, and waits until the lock
+ * is acquired and ensures that the statistics are flushed.
  */
 void
 pgstat_report_wal(bool force)
 {
-	pgstat_flush_wal(force);
+	pgstat_flush_wal(!force);
 
-	pgstat_flush_io(force);
+	pgstat_flush_io(!force);
 }
 
 /*

view thread (8+ 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], [email protected]
  Subject: Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly
  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