On Mon, Jul 24, 2023 at 08:31:01AM +0530, Bharath Rupireddy wrote:
> I also added a note atop worker_spi.c that the module also
> demonstrates how to write core (SQL) tests and extended (TAP) tests.
The value of the SQL tests comes down to the DO blocks that emulate
what the TAP tests could equally be able to do. While we already have
some places that do something similar (slot.sql or postgres_fdw.sql),
the SQL tests of worker_spi count for a total of five queries, which
is not much with one cluster initialized:
- One pg_reload_conf() to work a loop to happen in the worker.
- Two sanity checks.
- Two wait emulations.
Anyway, most people that do serious hacking on this list care about
the runtime of the tests all the time, and I am not on board in making
things slower for the sake of showing a test example here
particularly if there are ways to make them faster (long-term, we
should be able to do the init step only once for most cases), and
because we *have to* switch to TAP to have more advanced scenarios for
the custom wait events or just dynamic work launches based on what we
set on shared_preload_libraries. On top of that, we have other
examples in the tree that emulate waits for plain SQL tests to satisfy
assumptions with some follow-up query.
So, I don't really agree with the value gained here compared to the
execution cost of initializing two clusters for this module. I have
taken the time to check how the runtime changes when switching to TAP
for all the scenarios discussed here, and from my laptop, I can see
that:
- HEAD takes 4.4s, for only the sql/ test.
- Your latest patch is at 5.6s.
- My version attached to this message is at 3.7s.
In terms of runtime the benefits are here for me. Note that with the
first part of the test (previously in sql/), we don't lose coverage
with the loop of the workers so I agree that only checking that these
are launched is OK once worker_spi is in shared_preload_libraries.
However, I think that we should make sure that they are connected to
the correct database 'mydb'. I have updated the test to do that.
So, what do you think about the attached?
--
Michael