Hi, On Thu, Jan 16, 2025 at 11:38:47AM -0500, Andres Freund wrote: > Hi, > > On 2025-01-16 15:59:31 +0000, Bertrand Drouvot wrote: > > 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: > > > + * 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). > > This is a preposterously bad idea. The restriction to not allocate memory in > critical sections exists for a reason,
Thanks for sharing your thoughts on it. In [1], you said: " My view is that for IO stats no memory allocation should be required - that used to be the case and should be the case again " So, do you think that the initial proposal that has been made here (See R1. in [2]) i.e make use of a new PendingBackendWalStats variable: " 0003 does not rely on pgstat_prep_backend_pending() for its pending statistics but on a new PendingBackendWalStats variable. The reason is that the pending wal statistics are incremented in a critical section (see XLogWrite(), and so a call to pgstat_prep_pending_entry() could trigger a failed assertion: MemoryContextAllocZero()->"CritSectionCount == 0 || (context)->allowInCritSection" " and implemented up to v4 is a viable approach? [1]: https://www.postgresql.org/message-id/66efowskppsns35v5u2m7k4sdnl7yoz5bo64tdjwq7r5lhplrz%40y7dme5xwh2r5 [2]: https://www.postgresql.org/message-id/Z3zqc4o09dM/Ezyz%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
