Date: Thu, 13 Mar 2025 13:18:45 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Xuneng Zhou <xunengzhou@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org,
	Michael Paquier <michael@paquier.xyz>,
	Andres Freund <andres@anarazel.de>
Subject: Re: Fwd: [BUG]: the walsender does not update its IO statistics
 until it exits
Message-ID: <Z9LbNVAZrMinORG7@ip-10-97-1-34.eu-west-3.compute.internal>
References: <Z73IsKBceoVd4t55@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z762lusJXp_wNF_z@paquier.xyz>
 <Z77jgvhwOu9S0a5r@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z8FMjlyNpNicucGa@paquier.xyz>
 <Z8GSY8mk+0qVHzsW@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z8ULFzNY24_SMCdu@paquier.xyz>
 <Z8WYf1jyy4MwOveQ@ip-10-97-1-34.eu-west-3.compute.internal>
 <CABPTF7U5B1gnvvrbk+we+Zi3p0-KKAEXYCLj914T9Fm5Cjk1LA@mail.gmail.com>
 <CABPTF7VreDnD3YiWzx_=PpLRdgOQsH8Xp3fTGMc+r9rGpc3WLg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="RYJlY3SbA0pOku46"
Content-Disposition: inline
In-Reply-To: 
 <CABPTF7VreDnD3YiWzx_=PpLRdgOQsH8Xp3fTGMc+r9rGpc3WLg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/Z9LbNVAZrMinORG7%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk


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

Hi,

On Thu, Mar 13, 2025 at 07:33:24PM +0800, Xuneng Zhou wrote:
> Regarding patch 0001, the optimization in pgstat_backend_have_pending_cb
> looks good:

Thanks for looking at it!

> bool
> pgstat_backend_have_pending_cb(void)
> {
> - return (!pg_memory_is_all_zeros(&PendingBackendStats,
> - sizeof(struct PgStat_BackendPending)));
> + return backend_has_iostats;
> }
> 
> Additionally, the function pgstat_flush_backend includes the check:
> 
> + if (!pgstat_backend_have_pending_cb())
>     return false;
> 
> However, I think we might need to revise the comment (and possibly the
> function name) for clarity:
> 
> /*
>  * Check if there are any backend stats waiting to be flushed.
>  */

The comment is not exactly this one on the current HEAD, it looks like that you're
looking at a previous version of the core code.

> Originally, this function was intended to check multiple types of backend
> statistics, which made sense when PendingBackendStats was the centralized
> structure for various pending backend stats. However, since
> PgStat_PendingWalStats was removed from PendingBackendStats earlier, and
> now this patch introduces the backend_has_iostats variable, the scope of
> this function appears even narrower. This narrowed functionality no longer
> aligns closely with the original function name and its associated comment.

I don't think so, as since 76def4cdd7c, a pgstat_backend_wal_have_pending() check
has been added to pgstat_backend_have_pending_cb(). You're probably looking at
a version prior to 76def4cdd7c.

This particular sub-patch needs a rebase though, done in the attached. 0001
remains unchanged as compared to the v4 one just shared up-thread. If 0001 goes
in, merging 0002 would be less beneficial (as compared to v3).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

--RYJlY3SbA0pOku46
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0001-Flush-the-IO-statistics-of-active-walsenders.patch"