Re: per backend WAL statistics

Hi,

On Wed, Jan 15, 2025 at 03:11:32PM +0900, Michael Paquier wrote:
> On Fri, Jan 10, 2025 at 09:40:38AM +0000, Bertrand Drouvot wrote:
> > Please find attached v4 taking into account 2c14037bb5.
> 
> +} PgStat_WalCounters;
> +
> +typedef struct PgStat_WalStats
> +{
> +	PgStat_WalCounters wal_counters;
> 
> I know that's a nit, but perhaps that could be a patch of its own,
> pushing that to pg_stat_wal_build_tuple() to reduce the diffs in the
> main patch.

Done in 0003 attached.

> - * Find or create a local PgStat_BackendPending entry for proc number.
> + * Find or create a local PgStat_BackendPendingIO entry for proc number.
> 
> Seems like you are undoing a change here.

Arf, nice catch, hopefully that's only the comment.. Fixed in the attached.

> + * WAL pending statistics are incremented inside a critical section
> + * (see XLogWrite()), so we can't use pgstat_prep_pending_entry() and we rely on
> + * PendingBackendWalStats instead.
> + */
> +extern PGDLLIMPORT PgStat_PendingWalStats PendingBackendWalStats;
> 
> Hmm.  This makes me wonder if we should rethink a bit the way pending
> entries are retrieved and if we should do it beforehand for the WAL
> paths to avoid allocations in some critical sections.  Isn't that also
> because we avoid calling pgstat_prep_backend_pending() for the I/O
> case as only backends are supported now, discarding cases like the
> checkpointer where I/O could happen in a critical path?  As a whole,
> the approach taken by the patch is not really consistent with the
> rest.

I agree that's better to have a generic solution and to be consistent with
the other variable-numbered stats. 

The attached is implementing in 0001 the proposition done in [1], i.e:

1. It adds a new allow_critical_section to PgStat_KindInfo for pgstats kinds
2. It ensures to set temporarly allowInCritSection to true when needed

Note that for safety reason 0001 does set allowInCritSection back to false
unconditionally (means not checking again for allow_critical_section).

While it avoids the failed assertion mentioned above and in [1] (on
the MemoryContextAllocZero() call), the TAP tests are still failing with a new
failed assertion.

If you apply the whole patch series attached, you'll see that:

make -C src/bin/pg_rewind check PROVE_TESTS=t/001_basic.pl

is failing with something like:

TRAP: failed Assert("CritSectionCount == 0"), File: "mcxt.c", Line: 1107, PID: 3295726
pg18/bin/postgres(ExceptionalCondition+0xbb)[0x59668bee1f6d]
pg18/bin/postgres(MemoryContextCreate+0x46)[0x59668bf2a8fe]
pg18/bin/postgres(AllocSetContextCreateInternal+0x1df)[0x59668bf1bb11]
pg18/bin/postgres(pgstat_prep_pending_entry+0x86)[0x59668bcff8cc]
pg18/bin/postgres(pgstat_prep_backend_pending+0x2b)[0x59668bd024a9]

This one is more problematic because we are in MemoryContextCreate() so that the
"workaround" above does not help. I need to put more thoughts on it but already
sharing the issue here (as also discussed in [1]).

[1]: https://www.postgresql.org/message-id/Z4d_eggsxtBEdJAG%40paquier.xyz

Regards,

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