Feedback-ID: ia2694551:Fastmail
Date: Wed, 21 Aug 2024 13:55:06 -0400
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: "Andrey M. Borodin" <x4mmm@yandex-team.ru>, 
	vignesh C <vignesh21@gmail.com>, Andrew Borodin <amborodin86@gmail.com>, i.lazarev@postgrespro.ru, 
	Yura Sokolov <y.sokolov@postgrespro.ru>, Andres Freund <andres@anarazel.de>, 
	Thomas Munro <thomas.munro@gmail.com>, Gilles Darold <gilles@darold.net>, 
	Alexander Korotkov <aekorotkov@gmail.com>, Anastasia Lubennikova <a.lubennikova@postgrespro.ru>, 
	Daniel Gustafsson <daniel@yesql.se>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, 
	pgsql-hackers mailing list <pgsql-hackers@postgresql.org>
Subject: Re: MultiXact\SLRU buffers configuration
Message-ID: <202408211755.j5v7w2ez46no@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZsWH4HgIitscS6RE@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/202408211755.j5v7w2ez46no%40alvherre.pgsql>
Precedence: bulk

On 2024-Aug-21, Michael Paquier wrote:

> From fd8ab7b6845a2c56aa2c8d9c60f404f6b3407338 Mon Sep 17 00:00:00 2001
> From: Michael Paquier <michael@paquier.xyz>
> Date: Wed, 21 Aug 2024 15:16:06 +0900
> Subject: [PATCH 2/3] injection_point: Add injection_points.stats

> This GUC controls if statistics should be used or not in the module.
> Custom statistics require the module to be loaded with
> shared_preload_libraries, hence this GUC is made PGC_POSTMASTER.  By
> default, stats are disabled.
> 
> This will be used by an upcoming change in a test where stats should not
> be used, as the test has a dependency on a critical section.

I find it's strange that the information that stats cannot be used with
injection points that have dependency on critical sections (?), is only
in the commit message and not in the code.

Also, maybe it'd make sense for stats to be globally enabled, and that
only the tests that require it would disable them?  (It's probably easy
enough to have a value "injection_points.stats=auto" which means, if the
module is loaded in shared_preload_libraries them set stats on,
otherwise turn them off.)  TBH I don't understand why the issue that
stats require shared_preload_libraries only comes up now ...  Maybe
another approach is to say that if an injection point is loaded via
_LOAD() rather than the normal way, then stats are disabled for that one
rather than globally?  Or give the _LOAD() macro a boolean argument to
indicate whether to collect stats for that injection point or not.

Lastly, it's not clear to me what does it mean that the test has a
"dependency" on a critical section.  Do you mean to say that the
injection point runs inside a critical section?

> From e5329d080b9d8436af8f65aac118745cf1f81ca2 Mon Sep 17 00:00:00 2001
> From: Michael Paquier <michael@paquier.xyz>
> Date: Wed, 21 Aug 2024 15:09:06 +0900
> Subject: [PATCH 3/3] Rework new SLRU test with injection points
> 
> Rather than the SQL injection_points_load, this commit makes the test
> rely on the two macros to load and run an injection point from the
> cache, acting as an example of how to use them.

No issues with this, feel free to go ahead.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Cada quien es cada cual y baja las escaleras como quiere" (JMSerrat)