Re: per backend WAL statistics - Mailing list pgsql-hackers

Hi,

On Mon, Mar 10, 2025 at 04:46:53PM +0900, Michael Paquier wrote:
> On Sat, Mar 08, 2025 at 07:53:04AM +0000, Bertrand Drouvot wrote:
> > That would not be an issue should we only access the struct
> > fields in the code, but that's not the case as we're making use of
> > pg_memory_is_all_zeros() on it.
> 
> It does not hurt to keep it as it is, honestly.

I believe that's worse than before actually. Before padding bytes would "probably"
be set to zeros while now it's certainly not always the case. I think that
we already removed this (see comments === 4 in [1]).

> I've reviewed the last patch of the series

Thanks!

> and noticed a couple of 
> inconsistent comments across it, and some indentation issue.

I think I ran pgindent though. Anyway, thanks for fixing those!

> @@ -199,7 +258,8 @@ pgstat_flush_backend(bool nowait, bits32 flags)
>          return false;
>  
>      if (pg_memory_is_all_zeros(&PendingBackendStats,
> -                               sizeof(struct PgStat_BackendPending)))
> +                               sizeof(struct PgStat_BackendPending))
> +        && !pgstat_backend_wal_have_pending())
>          return false;
> 
> I have one issue with pgstat_flush_backend() and the early exit check
> done here.  If for example we use FLUSH_WAL but there is some IO data
> pending, we would lock the stats entry for nothing.  We could also
> return true even if there is no pending WAL data if the lock could not
> be taken, which would be incorrect because there was no data to flush
> to begin with.  I think that this should be adjusted so as we limit
> the entry lock depending on the flags given in input, like in the
> attached.

Yeah I agree this needs to be improved, thanks!

+       /* Some IO data pending? */
+       if ((flags & PGSTAT_BACKEND_FLUSH_IO) &&
+               !pg_memory_is_all_zeros(&PendingBackendStats,
+                                                               sizeof(struct PgStat_BackendPending)))
+               has_pending_data = true;

if PgStat_BackendPending contains more than pending_io in the future, then
that would check for zeros in a too large memory region.

I think it's better to check for:

    if (pg_memory_is_all_zeros(&PendingBackendStats.pending_io,
                               sizeof(struct PgStat_PendingIO)))

like in the attached. Or check on "backend_has_iostats" (if 0002 in [2] goes in).

+       /* Some WAL data pending? */
+       if ((flags & PGSTAT_BACKEND_FLUSH_WAL) &&
+               pgstat_backend_wal_have_pending())
+               has_pending_data = true;

I think we can use "else if" here (done in the attached) as it's not needed if
has_pending_data is already set to true.

That's the only 2 changes done in the attached as compared to the previous
version.

Regards,

[1]: https://www.postgresql.org/message-id/Z44vMD/rALy8pfVE%40ip-10-97-1-34.eu-west-3.compute.internal
[2]: https://www.postgresql.org/message-id/Z8WYf1jyy4MwOveQ%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

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