On Mon, Jul 31, 2023 at 6:40 AM Michael Paquier <mich...@paquier.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
