On Thu, Jun 2, 2016 at 12:25 PM, Thomas Munro <thomas.mu...@enterprisedb.com> wrote: > This patch allows identifiers to be specified by the WaitLatch and > WaitLatchOrSocket calls, but not for WaitEventSetWait, which is the > more general waiting primitive. I think it should be done by > WaitEventSetWait, and merely passed down by those wrappers functions.
The advantage of having WaitEventSetWait track is that we could track the events of secure_read and secure_write. One potential problem by doing so is if those routines are called during authentication. I don't recall that's the case, but this needs a double-check. > These are actually names for *wait points*, not names for latches. OK. > Some of the language in this patch makes it sound like they are latch > names/latch identifiers, which is inaccurate (the latches themselves > wouldn't be very interesting eg MyLatch). In some cases the main > thing of interest is actually a socket or timer anyway, not a latch, > so maybe it should appear as wait_event_type = WaitEventSet? Hm. A latch could wait for multiple types things it is waiting for, so don't you think we'd need to add more details in what is reported to pg_stat_activity? There are two fields now, and in the context of this patch: - wait_event_type, which I'd like to think is associated to a latch, so I named it so. - wait_event, which is the code path that we are waiting at, like SyncRep, the WAL writer main routine, etc. Now if you would like to get a list of all the things that are being waited for, shouldn't we add a third column to the set that has text[] as return type? Let's name it wait_event_details, and for a latch we have the following: - WL_LATCH_SET - WL_POSTMASTER_DEATH - WL_SOCKET_READABLE - WL_SOCKET_WRITEABLE Compared to all the other existing wait types, that's a bit new and that's exclusive to latches because we need a higher level of details. Don't you think so? But actually I don't think that's necessary to go into this level of details. We already know what a latch is waiting for by looking at the code... > Is there a reason you chose names like 'WALWriterMain'? That > particular wait point is in the function WalWriterMain (note different > case). It seems odd to use the containing function names to guide > naming, but not use them exactly. Since this namespace needs to be > able to name wait points anywhere in the postgres source tree (and > maybe outside it too, for extensions), maybe it should be made > hierarchical, like 'walwriter.WalWriterMain' (or some other > organisational scheme). Yeah, possibly this could be improved. I have put some thoughts in having clear names for each one of them, so I'd rather keep them simple. > I personally think it would be very useful for extensions to be able > to name their wait points. For example, I'd rather see > 'postgres_fdw.pgfdw_get_result' or similar than a vague 'Extension' > string which appears not only for all wait points in an extension but > also for all extensions. I hope we can figure out a good way to do > that. Clearly that would involve some shmem registry machinery to > make the names visible across backends (a similar problem exists with > lock tranche names). This patch is shaped this way intentionally based on the feedback I received at PGCon (Robert and others). We could provide a routine that extensions call in _PG_init to register a new latch event name in shared memory, but I didn't see much use in doing so, take for example the case of background worker, it is possible to register a custom string for pg_stat_activity via pgstat_report_activity. One could take advantage of having custom latch wait names in shared memory if an extension has wait points with latches though... But I am still not sure if that's worth the complexity. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
