On Thu, Jan 04, 2024 at 04:31:02PM -0600, Nathan Bossart wrote:
> On Thu, Jan 04, 2024 at 06:04:20PM +0530, Ashutosh Bapat wrote:
>> 0003 and 0004 are using the extension in this module for some serious
>> testing. The name of the extension test_injection_point indicates that
>> it's for testing injection points and not for some serious use of
>> injection callbacks it adds. Changes 0003 and 0004 suggest otherwise.
>
> Yeah, I think test_injection_point should be reserved for testing the
> injection point machinery.
Sure. FWIW, it makes sense to me to keep the SQL interface and the
callbacks in the module, per the reasons below.
>> I suggest we move test_injection_points from src/test/modules to
>> contrib/ and rename it as "injection_points". The test files may still
>> be named as test_injection_point. The TAP tests in 0003 and 0004 once
>> moved to their appropriate places, will load injection_point extension
>> and use it. That way predefined injection point callbacks will also be
>> available for others to use.
>
> Rather than defining a module somewhere that tests would need to load,
> should we just put the common callbacks in the core server? Unless there's
> a strong reason to define them elsewhere, that could be a nice way to save
> a step in the tests.
Nah, having some pre-existing callbacks existing in the backend is
against the original minimalistic design spirit. These would also
require an SQL interface, and the interface design also depends on the
functions registering them when pushing down custom conditions.
Pushing that down to extensions to do what they want will lead to less
noise, particularly if you consider that we will most likely want to
tweak the callback interfaces for backpatched bugs. That's also why I
think contrib/ is not a good idea, src/test/modules/ serving the
actual testing purpose here.
--
Michael