Re: Support to define custom wait events for extensions

On Mon, Jul 31, 2023 at 6:40 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> You are right that I am making things inconsistent here. Having a
> behavior close to the existing LWLock and use "extension" when the
> event cannot be found makes the most sense. I have been a bit
> confused with the wording though of this part of the docs, though, as
> LWLocks don't directly use the custom wait event APIs.

+ * calling WaitEventExtensionRegisterName() in the current process, in
+ * which case give up and return an unknown state.

We're not giving up and returning an unknown state in the v10 patch
unlike v9, no? This comment needs to change.

> > 4.
> > + Add-ins can define custom wait events under the wait event type
> >
> > I see a few instances of Add-ins/add-in in xfunc.sgml. Isn't it better
> > to use the word extension given that glossary defines what an
> > extension is https://www.postgresql.org/docs/current/glossary.html#GLOSSARY-EXTENSION?
>
> An extension is an Add-in, and may be loaded, but it is possible to
> have modules that just need to be LOAD'ed (with command line or just
> shared_preload_libraries) so an add-in may not always be an extension.
> I am not completely sure if add-ins is the best term, but it covers
> both, and that's consistent with the existing docs. Perhaps the same
> area of the docs should be refreshed, but that looks like a separate
> patch for me. For now, I'd rather use a consistent term, and this one
> sounds OK to me.
>
> [1]: https://www.postgresql.org/docs/devel/extend-extensions.html.

The "external module" seems the right wording here. Use of "add-ins"
is fine by me for this patch.

> Okay by me that it may be a better idea to use ereport(ERROR) in the
> long run, so changed this way. I have introduced a
> WAIT_EVENT_CLASS_MASK and a WAIT_EVENT_ID_MASK as we now use
> 0xFF000000 and 0x0000FFFF in three places of this file. This should
> just be a patch on its own.

Yeah, I don't mind these macros going along or before or after the
custom wait events feature.

> Yes, this was mentioned upthread. I am not completely sure yet how
> much we need to do for this interface, but surely it would be faster
> to have a Multiple() interface that returns an array made of N numbers
> requested (rather than a rank of them). For now, I'd rather just aim
> for simplicity for the basics.

+1 to be simple for now. If any such requests come in future, I'm sure
we can always get back to it.

> > 9.
> > # The expected result is a special pattern here with a newline coming from the
> > # first query where the shared memory state is set.
> > $result = $node->poll_query_until(
> > 'postgres',
> > qq[SELECT worker_spi_init(); SELECT wait_event FROM
> > pg_stat_activity WHERE backend_type ~ 'worker_spi';],
> > qq[
> > worker_spi_main]);
> >
> > This test doesn't have to be that complex with the result being a
> > special pattern, SELECT worker_spi_init(); can just be within a
> > separate safe_psql.
>
> No, it cannot because we need the custom wait event string to be
> loaded in the same connection as the one querying pg_stat_activity.
> A different thing that can be done here is to use background_psql()
> with a query_until(), though I am not sure that this is worth doing
> here.

-1 to go to the background_psql and query_until route. However,
enhancing the comment might help "we need the custom wait event string
to be loaded in the same connection as .....". Having said this, I
don't quite understand the point of having worker_spi_init() when we
say clearly how to use shmem hooks and custom wait events. If its
intention is to show loading of shared memory to a backend via a
function, do we really need it in worker_spi? I don't mind removing it
if it's not adding any significant value.

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