Обсуждение: WIP: new system catalog pg_wait_event
Hi hackers, Now that fa88928470 generates automatically code and documentation related to wait events, why not exposing the wait events description through a system catalog relation? (the idea has been proposed on twitter by Yves Colin [1]). I think that could be useful to: - join this new relation with pg_stat_activity and have a quick understanding of what the sessions are waiting for (if any). - quickly compare the wait events across versions (what are the new ones if any,..) Please find attached a POC patch creating this new system catalog pg_wait_event. The patch: - updates the documentation - adds a new option to generate-wait_event_types.pl to generate the pg_wait_event.dat - creates the pg_wait_event.h - works with autoconf It currently does not: - works with meson (has to be done) - add tests (not sure it's needed) - create an index on the new system catalog (not sure it's needed as the data fits in 4 pages (8kB default size)). Outcome example: postgres=# select a.pid, a.application_name, a.wait_event,d.description from pg_stat_activity a, pg_wait_event d where a.wait_event= d.wait_event_name and state='active'; pid | application_name | wait_event | description ---------+------------------+-------------+------------------------------------------------------------------- 2937546 | pgbench | WALInitSync | Waiting for a newly initialized WAL file to reach durable storage (1 row) There is still some work to be done to generate the pg_wait_event.dat file, specially when the same wait event name can be found in multiple places (like for example "WALWrite" in IO and LWLock), leading to: postgres=# select * from pg_wait_event where wait_event_name = 'WALWrite'; wait_event_name | description -----------------+---------------------------------------------------------------------------------- WALWrite | Waiting for a write to a WAL file. Waiting for WAL buffers to be written to disk WALWrite | Waiting for WAL buffers to be written to disk (2 rows) which is obviously not right (we'll probably have to add the wait class name to the game). I'm sharing it now (even if it's still WIP) so that you can start sharing your thoughts about it. [1]: https://twitter.com/Ycolin/status/1676598065048743948 Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
"Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com> writes: > Now that fa88928470 generates automatically code and documentation > related to wait events, why not exposing the wait events description > through a system catalog relation? I think you'd be better off making this a view over a set-returning function. The nearby work to allow run-time extensibility of the set of wait events is not going to be happy with a static catalog. regards, tom lane
Hi, On 8/4/23 5:08 PM, Tom Lane wrote: > "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com> writes: >> Now that fa88928470 generates automatically code and documentation >> related to wait events, why not exposing the wait events description >> through a system catalog relation? > > I think you'd be better off making this a view over a set-returning > function. The nearby work to allow run-time extensibility of the > set of wait events is not going to be happy with a static catalog. Oh right, good point, thanks!: I'll come back with a new patch version to make use of SRF instead. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On 8/7/23 10:23 AM, Drouvot, Bertrand wrote: > Hi, > > On 8/4/23 5:08 PM, Tom Lane wrote: >> "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com> writes: >>> Now that fa88928470 generates automatically code and documentation >>> related to wait events, why not exposing the wait events description >>> through a system catalog relation? >> >> I think you'd be better off making this a view over a set-returning >> function. The nearby work to allow run-time extensibility of the >> set of wait events is not going to be happy with a static catalog. > > Oh right, good point, thanks!: I'll come back with a new patch version to make > use of SRF instead. Please find attached v2 making use of SRF. v2: - adds a new pg_wait_event.c that acts as the "template" for the SRF - generates pg_wait_event_insert.c (through generate-wait_event_types.pl) that is included into pg_wait_event.c That way I think it's flexible enough to add more code if needed in the SRF. The patch also: - updates the doc - works with autoconf and meson - adds a simple test I'm adding a new CF entry for it. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
At Mon, 7 Aug 2023 17:11:50 +0200, "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com> wrote in > That way I think it's flexible enough to add more code if needed in > the SRF. > > The patch also: > > - updates the doc > - works with autoconf and meson > - adds a simple test > > I'm adding a new CF entry for it. As I mentioned in another thread, I'm uncertain about our stance on the class id of the wait event. If a class acts as a namespace, we should include it in the view. Otherwise, if the class id is just an attribute of the wait event, we should make the event name unique. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Tue, Aug 08, 2023 at 11:53:32AM +0900, Kyotaro Horiguchi wrote: > As I mentioned in another thread, I'm uncertain about our stance on > the class id of the wait event. If a class acts as a namespace, we > should include it in the view. Otherwise, if the class id is just an > attribute of the wait event, we should make the event name unique. Including the class name in the view makes the most sense to me, FWIW, as it could be also possible that one reuses an event name in the existing in-core list, but for extensions. That's of course not something I would recommend. -- Michael
Вложения
Hi, On 8/8/23 5:05 AM, Michael Paquier wrote: > On Tue, Aug 08, 2023 at 11:53:32AM +0900, Kyotaro Horiguchi wrote: >> As I mentioned in another thread, I'm uncertain about our stance on >> the class id of the wait event. If a class acts as a namespace, we >> should include it in the view. Otherwise, if the class id is just an >> attribute of the wait event, we should make the event name unique. > > Including the class name in the view makes the most sense to me, FWIW, > as it could be also possible that one reuses an event name in the > existing in-core list, but for extensions. That's of course not > something I would recommend. Thanks Kyotaro-san and Michael for the feedback. I do agree and will add the class name. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On 8/8/23 7:37 AM, Drouvot, Bertrand wrote: > Hi, > > On 8/8/23 5:05 AM, Michael Paquier wrote: >> On Tue, Aug 08, 2023 at 11:53:32AM +0900, Kyotaro Horiguchi wrote: >>> As I mentioned in another thread, I'm uncertain about our stance on >>> the class id of the wait event. If a class acts as a namespace, we >>> should include it in the view. Otherwise, if the class id is just an >>> attribute of the wait event, we should make the event name unique. >> >> Including the class name in the view makes the most sense to me, FWIW, >> as it could be also possible that one reuses an event name in the >> existing in-core list, but for extensions. That's of course not >> something I would recommend. > > Thanks Kyotaro-san and Michael for the feedback. I do agree and will > add the class name. > Please find attached v3 adding the wait event types. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
On Tue, Aug 08, 2023 at 10:16:37AM +0200, Drouvot, Bertrand wrote: > Please find attached v3 adding the wait event types. +-- There will surely be at least 9 wait event types, 240 wait events and at +-- least 27 related to WAL +select count(distinct(wait_event_type)) > 8 as ok_type, + count(*) > 239 as ok, + count(*) FILTER (WHERE description like '%WAL%') > 26 AS ok_wal_desc +from pg_wait_event; The point is to check the execution of this function, so this could be simpler, like that or a GROUP BY clause with the event type: SELECT count(*) > 0 FROM pg_wait_event; SELECT wait_event_type, count(*) > 0 AS has_data FROM pg_wait_event GROUP BY wait_event_type ORDER BY wait_event_type; + printf $ic "\tmemset(values, 0, sizeof(values));\n"; + printf $ic "\tmemset(nulls, 0, sizeof(nulls));\n\n"; + printf $ic "\tvalues[0] = CStringGetTextDatum(\"%s\");\n", $last; + printf $ic "\tvalues[1] = CStringGetTextDatum(\"%s\");\n", $wev->[1]; + printf $ic "\tvalues[2] = CStringGetTextDatum(\"%s\");\n\n", $new_desc; That's overcomplicated for some code generated. Wouldn't it be simpler to generate a list of elements, with the code inserting the tuples materialized looping over it? + my $new_desc = substr $wev->[2], 1, -2; + $new_desc =~ s/'/\\'/g; + $new_desc =~ s/<.*>(.*?)<.*>/$1/g; + $new_desc =~ s/<xref linkend="guc-(.*?)"\/>/$1/g; + $new_desc =~ s/; see.*$//; Better to document what this does, the contents produced look good. + rename($ictmp, "$output_path/pg_wait_event_insert.c") + || die "rename: $ictmp to $output_path/pg_wait_event_insert.c: $!"; # seems nicer to not add that as an include path for the whole backend. waitevent_sources = files( 'wait_event.c', + 'pg_wait_event.c', ) This could use a name referring to SQL functions, say wait_event_funcs.c, with a wait_event_data.c or a wait_event_funcs_data.c? + # Don't generate .c (except pg_wait_event_insert.c) and .h files for + # Extension, LWLock and Lock, these are handled independently. + my $is_exception = $waitclass eq 'WaitEventExtension' || + $waitclass eq 'WaitEventLWLock' || + $waitclass eq 'WaitEventLock'; Perhaps it would be cleaner to use a separate loop? -- Michael
Вложения
Hi, On 8/9/23 9:56 AM, Michael Paquier wrote: > On Tue, Aug 08, 2023 at 10:16:37AM +0200, Drouvot, Bertrand wrote: >> Please find attached v3 adding the wait event types. > > +-- There will surely be at least 9 wait event types, 240 wait events and at > +-- least 27 related to WAL > +select count(distinct(wait_event_type)) > 8 as ok_type, > + count(*) > 239 as ok, > + count(*) FILTER (WHERE description like '%WAL%') > 26 AS ok_wal_desc > +from pg_wait_event; > The point is to check the execution of this function, so this could be > simpler, like that or a GROUP BY clause with the event type: > SELECT count(*) > 0 FROM pg_wait_event; > SELECT wait_event_type, count(*) > 0 AS has_data FROM pg_wait_event > GROUP BY wait_event_type ORDER BY wait_event_type; > Thanks for looking at it! Right, so v4 attached is just testing "SELECT count(*) > 0 FROM pg_wait_event;", that does look enough to test. > + printf $ic "\tmemset(values, 0, sizeof(values));\n"; > + printf $ic "\tmemset(nulls, 0, sizeof(nulls));\n\n"; > + printf $ic "\tvalues[0] = CStringGetTextDatum(\"%s\");\n", $last; > + printf $ic "\tvalues[1] = CStringGetTextDatum(\"%s\");\n", $wev->[1]; > + printf $ic "\tvalues[2] = CStringGetTextDatum(\"%s\");\n\n", $new_desc; > > That's overcomplicated for some code generated. Wouldn't it be > simpler to generate a list of elements, with the code inserting the > tuples materialized looping over it? > Yeah, agree thanks! In v4, the perl script now appends the wait events in a List that way: " printf $ic "\telement = (wait_event_element *) palloc(sizeof(wait_event_element));\n"; printf $ic "\telement->wait_event_type = \"%s\";\n", $last; printf $ic "\telement->wait_event_name = \"%s\";\n", $wev->[1]; printf $ic "\telement->wait_event_description = \"%s\";\n\n", $new_desc; printf $ic "\twait_event = lappend(wait_event, element);\n\n"; " And the C function pg_get_wait_events() now iterates over this List. > + my $new_desc = substr $wev->[2], 1, -2; > + $new_desc =~ s/'/\\'/g; > + $new_desc =~ s/<.*>(.*?)<.*>/$1/g; > + $new_desc =~ s/<xref linkend="guc-(.*?)"\/>/$1/g; > + $new_desc =~ s/; see.*$//; > Better to document what this does, good idea... I had to turn them "on" one by one to recall why they are there...;-) Done in v4. > the contents produced look good. yeap > > + rename($ictmp, "$output_path/pg_wait_event_insert.c") > + || die "rename: $ictmp to $output_path/pg_wait_event_insert.c: $!"; > > # seems nicer to not add that as an include path for the whole backend. > waitevent_sources = files( > 'wait_event.c', > + 'pg_wait_event.c', > ) > > This could use a name referring to SQL functions, say > wait_event_funcs.c, with a wait_event_data.c or a > wait_event_funcs_data.c? That sounds better indeed, thanks! v4 is using wait_event_funcs.c and wait_event_funcs_data.c. > > + # Don't generate .c (except pg_wait_event_insert.c) and .h files for > + # Extension, LWLock and Lock, these are handled independently. > + my $is_exception = $waitclass eq 'WaitEventExtension' || > + $waitclass eq 'WaitEventLWLock' || > + $waitclass eq 'WaitEventLock'; > Perhaps it would be cleaner to use a separate loop? Agree that's worth it given the fact that iterating one more time is not that costly here. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
On Thu, Aug 10, 2023 at 08:09:34PM +0200, Drouvot, Bertrand wrote: > Agree that's worth it given the fact that iterating one more time is not that > costly here. I have reviewed v4, and finished by putting my hands on it to see what I could do. + printf $wc "\telement = (wait_event_element *) palloc(sizeof(wait_event_element));\n"; + + printf $wc "\telement->wait_event_type = \"%s\";\n", $last; + printf $wc "\telement->wait_event_name = \"%s\";\n", $wev->[1]; + printf $wc "\telement->wait_event_description = \"%s\";\n\n", $new_desc; + + printf $wc "\twait_event = lappend(wait_event, element);\n\n"; + } This is simpler than the previous versions, still I am not much a fan of implying the use of a list and these pallocs. There are two things that we could do: - Hide that behind a macro defined in wait_event_funcs.c. - Feed the data generated here into a static structure, like: +static const struct +{ + const char *type; + const char *name; + const char *description; +} After experimenting with both, I've found the latter a tad cleaner, so the attached version does that. + <structfield>description</structfield> <type>texte</type> This one was difficult to see.. I am not sure that "pg_wait_event" is a good idea for the name if the new view. How about "pg_wait_events" instead, in plural form? There is more than one wait event listed. One log entry in Solution.pm has missed the addition of a reference to wait_event_funcs_data.c. And.. src/backend/Makefile missed two updates for maintainer-clean & co, no? One thing that the patch is still missing is the handling of custom wait events for extensions. So this still requires more code. I was thinking about listed these events as: - Type: "Extension" - Name: What a module has registered. - Description: "Custom wait event \"%Name%\" defined by extension". For now I am attaching a v5. -- Michael
Вложения
Hi, On 8/14/23 6:37 AM, Michael Paquier wrote: > On Thu, Aug 10, 2023 at 08:09:34PM +0200, Drouvot, Bertrand wrote: >> Agree that's worth it given the fact that iterating one more time is not that >> costly here. > > I have reviewed v4, and finished by putting my hands on it to see what > I could do. Thanks! > There are two things > that we could do: > - Hide that behind a macro defined in wait_event_funcs.c. > - Feed the data generated here into a static structure, like: > +static const struct > +{ > + const char *type; > + const char *name; > + const char *description; > +} > > After experimenting with both, I've found the latter a tad cleaner, so > the attached version does that. Yeah, looking at what you've done in v5, I also agree that's better that what has been done in v4 (I also think it will be easier to maintain). > I am not sure that "pg_wait_event" is a good idea for the name if the > new view. How about "pg_wait_events" instead, in plural form? There > is more than one wait event listed. > I'd prefer the singular form. There is a lot of places where it's already used (pg_database, pg_user, pg_namespace...to name a few) and it looks like that using the plural form are exceptions. > One log entry in Solution.pm has missed the addition of a reference to > wait_event_funcs_data.c. > Oh right, thanks for fixing it in v5. > And.. src/backend/Makefile missed two updates for maintainer-clean & co, > no? > Oh right, thanks for fixing it in v5. > One thing that the patch is still missing is the handling of custom > wait events for extensions. Yeah, now that af720b4c50 is done, I'll add the custom wait events handling in v6. > So this still requires more code. I was > thinking about listed these events as: > - Type: "Extension" > - Name: What a module has registered. > - Description: "Custom wait event \"%Name%\" defined by extension". That sounds good to me, I'll do that. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Wed, Aug 16, 2023 at 07:04:53AM +0200, Drouvot, Bertrand wrote: > I'd prefer the singular form. There is a lot of places where it's already used > (pg_database, pg_user, pg_namespace...to name a few) and it looks like that using > the plural form are exceptions. Okay, fine by me. Also, I would remove the "wait_event_" prefixes to the field names for the attribute names. > Yeah, now that af720b4c50 is done, I'll add the custom wait events handling > in v6. Thanks. I guess that the hash tables had better remain local to wait_event.c. -- Michael
Вложения
Hi, On 8/16/23 8:22 AM, Michael Paquier wrote: > On Wed, Aug 16, 2023 at 07:04:53AM +0200, Drouvot, Bertrand wrote: >> I'd prefer the singular form. There is a lot of places where it's already used >> (pg_database, pg_user, pg_namespace...to name a few) and it looks like that using >> the plural form are exceptions. > > Okay, fine by me. Great, singular form is being used in v6 attached. > Also, I would remove the "wait_event_" prefixes to > the field names for the attribute names. > It looks like it has already been done in v5. >> Yeah, now that af720b4c50 is done, I'll add the custom wait events handling >> in v6. > > Thanks. I guess that the hash tables had better remain local to > wait_event.c. Yeah, agree, done that way in v6 (also added a test in 001_worker_spi.pl to ensure that "worker_spi_main" is reported in pg_wait_event). I added a "COLLATE "C"" in the "order by" clause's test that we added in src/test/regress/sql/sysviews.sql (the check was failing on my environment). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
On Wed, Aug 16, 2023 at 01:43:35PM +0200, Drouvot, Bertrand wrote: > Yeah, agree, done that way in v6 (also added a test in 001_worker_spi.pl > to ensure that "worker_spi_main" is reported in pg_wait_event). -typedef struct WaitEventExtensionEntryByName -{ - char wait_event_name[NAMEDATALEN]; /* hash key */ - uint16 event_id; /* wait event ID */ -} WaitEventExtensionEntryByName; I'd rather keep all these structures local to wait_event.c, as these cover the internals of the hash tables. Could you switch GetWaitEventExtensionEntries() so as it returns a list of strings or an array of char*? We probably can live with the list for that. + char buf[NAMEDATALEN + 44]; //"Custom wait event \"%Name%\" defined by extension" + Incorrect comment. This would be simpler as a StringInfo. Thanks for the extra test in worker_spi looking at the contents of the catalog. -- Michael
Вложения
Hi, Thank you for creating the patch! I think it is a very useful view as a user. I will share some thoughts about the v6 patch. 1) The regular expression needs to be changed in generate-wait_event_types.pl. I have compared the documentation with the output of the pg_wait_event view and found the following differences. * For parameter names, the substitution for underscore is missing. -ArchiveCleanupCommand Waiting for archive_cleanup_command to complete -ArchiveCommand Waiting for archive_command to complete +ArchiveCleanupCommand Waiting for archive-cleanup-command to complete +ArchiveCommand Waiting for archive-command to complete -RecoveryEndCommand Waiting for recovery_end_command to complete +RecoveryEndCommand Waiting for recovery-end-command to complete -RestoreCommand Waiting for restore_command to complete +RestoreCommand Waiting for restore-command to complete * The HTML tag match is not shortest match. -frozenid Waiting to update pg_database.datfrozenxid and pg_database.datminmxid +frozenid Waiting to update datminmxid * There are two blanks before "about". Also " for heavy weight is removed (is it intended?) -LockManager Waiting to read or update information about "heavyweight" locks +LockManager Waiting to read or update information about heavyweight locks * Do we need "worker_spi_main" in the description? The name column shows the same one, so it could be omitted. > pg_wait_event > worker_spi_main Custom wait event "worker_spi_main" defined by > extension 2) Would it be better to add "extension" meaning unassigned? > Extensions can add Extension and LWLock types to the list shown in > Table 28.8 and Table 28.12. In some cases, the name of LWLock assigned > by an extension will not be available in all server processes; It might > be reported as just “extension” rather than the extension-assigned > name. 3) Would index == els be better for the following Assert? + Assert(index <= els); 4) There is a typo. alll -> all + /* Allocate enough space for alll entries */ 5) BTW, although I think this is outside the scope of this patch, it might be a good idea to be able to add a description to the API for custom wait events. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
On Thu, Aug 17, 2023 at 10:53:02AM +0900, Masahiro Ikeda wrote: > BTW, although I think this is outside the scope of this patch, > it might be a good idea to be able to add a description to the > API for custom wait events. Somebody on twitter has raised this point. I am not sure that we need to go down to that for the sake of this view, but I'm OK to.. Disagree and Commit to any consensus reached on this matter. -- Michael
Вложения
On 2023-08-17 10:57, Michael Paquier wrote: > On Thu, Aug 17, 2023 at 10:53:02AM +0900, Masahiro Ikeda wrote: >> BTW, although I think this is outside the scope of this patch, >> it might be a good idea to be able to add a description to the >> API for custom wait events. > > Somebody on twitter has raised this point. I am not sure that we need > to go down to that for the sake of this view, but I'm OK to.. > Disagree and Commit to any consensus reached on this matter. Oh, okay. Thanks for sharing the information. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Hi, On 8/16/23 2:08 PM, Michael Paquier wrote: > On Wed, Aug 16, 2023 at 01:43:35PM +0200, Drouvot, Bertrand wrote: >> Yeah, agree, done that way in v6 (also added a test in 001_worker_spi.pl >> to ensure that "worker_spi_main" is reported in pg_wait_event). > > -typedef struct WaitEventExtensionEntryByName > -{ > - char wait_event_name[NAMEDATALEN]; /* hash key */ > - uint16 event_id; /* wait event ID */ > -} WaitEventExtensionEntryByName; > > I'd rather keep all these structures local to wait_event.c, as these > cover the internals of the hash tables. > > Could you switch GetWaitEventExtensionEntries() so as it returns a > list of strings or an array of char*? We probably can live with the > list for that. > Yeah, I was not sure about this (returning a list of WaitEventExtensionEntryByName or a list of wait event names) while working on v6. That's true that the only need here is to get the names of the custom wait events. Returning only the names would allow us to move the WaitEventExtensionEntryByName definition back to the wait_event.c file. It makes sense to me, done in v7 attached and renamed the function to GetWaitEventExtensionNames(). > + char buf[NAMEDATALEN + 44]; //"Custom wait event \"%Name%\" defined by extension" + > Incorrect comment. This would be simpler as a StringInfo. Yeah and probably less error prone: done in v7. While at it, v7 is deliberately not calling "pfree(waiteventnames)" and "resetStringInfo(&buf)" in pg_get_wait_events(): reason is that they are palloc in a short-lived memory context while executing pg_get_wait_events(). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
Hi, On 8/17/23 3:53 AM, Masahiro Ikeda wrote: > Hi, > > Thank you for creating the patch! > I think it is a very useful view as a user. > > I will share some thoughts about the v6 patch. > Thanks for looking at it! > 1) > > The regular expression needs to be changed in generate-wait_event_types.pl. > I have compared the documentation with the output of the pg_wait_event > view and found the following differences. > > * For parameter names, the substitution for underscore is missing. > -ArchiveCleanupCommand Waiting for archive_cleanup_command to complete > -ArchiveCommand Waiting for archive_command to complete > +ArchiveCleanupCommand Waiting for archive-cleanup-command to complete > +ArchiveCommand Waiting for archive-command to complete > -RecoveryEndCommand Waiting for recovery_end_command to complete > +RecoveryEndCommand Waiting for recovery-end-command to complete > -RestoreCommand Waiting for restore_command to complete > +RestoreCommand Waiting for restore-command to complete > Yeah, nice catch. v7 just shared up-thread replace "-" by "_" for such a case. But I'm not sure the pg_wait_event description field needs to be 100% aligned with the documentation: wouldn't be better to replace "-" by " " in such cases in pg_wait_event? > * The HTML tag match is not shortest match. > -frozenid Waiting to update pg_database.datfrozenxid and pg_database.datminmxid > +frozenid Waiting to update datminmxid > Nice catch, fixed in v7. > * There are two blanks before "about". This is coming from wait_event_names.txt, thanks for pointing out. I just submitted a patch [1] to fix that. > Also " for heavy weight is > removed (is it intended?) > -LockManager Waiting to read or update information about "heavyweight" locks > +LockManager Waiting to read or update information about heavyweight locks > Not intended, fixed in v7. > * Do we need "worker_spi_main" in the description? The name column > shows the same one, so it could be omitted. >> pg_wait_event >> worker_spi_main Custom wait event "worker_spi_main" defined by extension > Do you mean remove the wait event name from the description in case of custom extension wait events? I'd prefer to keep it, if not the descriptions would be all the same for custom wait events. > 2) > > Would it be better to add "extension" meaning unassigned? > >> Extensions can add Extension and LWLock types to the list shown in Table 28.8 and Table 28.12. In some cases, the nameof LWLock assigned by an extension will not be available in all server processes; It might be reported as just “extension”rather than the extension-assigned name. > Yeah, could make sense but I think that should be a dedicated patch for the documentation. > 3) > > Would index == els be better for the following Assert? > + Assert(index <= els); > Agree, done in v7. > 4) > > There is a typo. alll -> all > + /* Allocate enough space for alll entries */ > Thanks! Fixed in v7. [1]: https://www.postgresql.org/message-id/dd836027-2e9e-4df9-9fd9-7527cd1757e1%40gmail.com Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On 8/17/23 3:57 AM, Michael Paquier wrote: > On Thu, Aug 17, 2023 at 10:53:02AM +0900, Masahiro Ikeda wrote: >> BTW, although I think this is outside the scope of this patch, >> it might be a good idea to be able to add a description to the >> API for custom wait events. > > Somebody on twitter has raised this point. I think I know him ;-) > I am not sure that we need > to go down to that for the sake of this view, but I'm OK to.. > Disagree and Commit to any consensus reached on this matter. Yeah, I changed my mind of this. I think it's better to keep the "control" about what is displayed in the description field for this new system view. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On 2023-08-17 14:53, Drouvot, Bertrand wrote: > On 8/17/23 3:53 AM, Masahiro Ikeda wrote: >> 1) >> >> The regular expression needs to be changed in >> generate-wait_event_types.pl. >> I have compared the documentation with the output of the pg_wait_event >> view and found the following differences. >> >> * For parameter names, the substitution for underscore is missing. >> -ArchiveCleanupCommand Waiting for archive_cleanup_command to complete >> -ArchiveCommand Waiting for archive_command to complete >> +ArchiveCleanupCommand Waiting for archive-cleanup-command to complete >> +ArchiveCommand Waiting for archive-command to complete >> -RecoveryEndCommand Waiting for recovery_end_command to complete >> +RecoveryEndCommand Waiting for recovery-end-command to complete >> -RestoreCommand Waiting for restore_command to complete >> +RestoreCommand Waiting for restore-command to complete >> > > Yeah, nice catch. v7 just shared up-thread replace "-" by "_" > for such a case. But I'm not sure the pg_wait_event description field > needs > to be 100% aligned with the documentation: wouldn't be better to > replace > "-" by " " in such cases in pg_wait_event? > >> * The HTML tag match is not shortest match. >> -frozenid Waiting to update pg_database.datfrozenxid and >> pg_database.datminmxid >> +frozenid Waiting to update datminmxid >> > > Nice catch, fixed in v7. Thanks, I confirmed. >> * There are two blanks before "about". > > This is coming from wait_event_names.txt, thanks for pointing out. > I just submitted a patch [1] to fix that. Thanks. I agree it's better to be treated as a separated patch. >> Also " for heavy weight is >> removed (is it intended?) >> -LockManager Waiting to read or update information about "heavyweight" >> locks >> +LockManager Waiting to read or update information about heavyweight >> locks >> > > Not intended, fixed in v7. OK, I confirmed it's fixed. >> * Do we need "worker_spi_main" in the description? The name column >> shows the same one, so it could be omitted. >>> pg_wait_event >>> worker_spi_main Custom wait event "worker_spi_main" defined by >>> extension >> > > Do you mean remove the wait event name from the description in case of > custom > extension wait events? I'd prefer to keep it, if not the descriptions > would be > all the same for custom wait events. OK, I thought it's redundant. BTW, is it better to start with "Waiting" like any other lines? For example, "Waiting for custom event \"worker_spi_main\" defined by an extension". >> 2) >> >> Would it be better to add "extension" meaning unassigned? >> >>> Extensions can add Extension and LWLock types to the list shown in >>> Table 28.8 and Table 28.12. In some cases, the name of LWLock >>> assigned by an extension will not be available in all server >>> processes; It might be reported as just “extension” rather than the >>> extension-assigned name. >> > > Yeah, could make sense but I think that should be a dedicated patch > for the documentation. OK, make sense. >> 3) >> >> Would index == els be better for the following Assert? >> + Assert(index <= els); >> > > Agree, done in v7. > >> 4) >> >> There is a typo. alll -> all >> + /* Allocate enough space for alll entries */ Thanks, I confirmed it's fixed. The followings are additional comments for v7. 1) >> I am not sure that "pg_wait_event" is a good idea for the name if the >> new view. How about "pg_wait_events" instead, in plural form? There >> is more than one wait event listed. > I'd prefer the singular form. There is a lot of places where it's > already used > (pg_database, pg_user, pg_namespace...to name a few) and it looks like > that using > the plural form are exceptions. Since I got the same feeling as Michael-san that "pg_wait_events" would be better, I check the names of all system views. I think the singular form seems to be exceptions for views though the singular form is used usually for system catalogs. https://www.postgresql.org/docs/devel/views.html https://www.postgresql.org/docs/devel/catalogs.html 2) $ctmp must be $wctmp? + rename($wctmp, "$output_path/wait_event_funcs_data.c") + || die "rename: $ctmp to $output_path/wait_event_funcs_data.c: $!"; 3) "List all the wait events type, name and descriptions" is enough? + * List all the wait events (type, name) and their descriptions. 4) Why don't you add the usage of the view in the monitoring.sgml? ``` <para> Here is an example of how wait events can be viewed: <programlisting> SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event is NOT NULL; pid | wait_event_type | wait_event ------+-----------------+------------ 2540 | Lock | relation 6644 | LWLock | ProcArray (2 rows) </programlisting> </para> ``` ex. postgres(3789417)=# SELECT a.pid, a.wait_event_type, a.wait_event, w.description FROM pg_stat_activity a JOIN pg_wait_event w ON (a.wait_event_type = w.type AND a.wait_event = w.name) WHERE wait_event is NOT NULL; pid | wait_event_type | wait_event | description ---------+-----------------+-------------------+-------------------------------------------------------- 3783759 | Activity | AutoVacuumMain | Waiting in main loop of autovacuum launcher process 3812866 | Extension | WorkerSpiMain | Custom wait event "WorkerSpiMain" defined by extension 3783756 | Activity | BgWriterHibernate | Waiting in background writer process, hibernating 3783760 | Activity | ArchiverMain | Waiting in main loop of archiver process 3783755 | Activity | CheckpointerMain | Waiting in main loop of checkpointer process 3783758 | Activity | WalWriterMain | Waiting in main loop of WAL writer process (6 rows) 5) Though I'm not familiar the value, do we need procost? +{ oid => '8403', descr => 'describe wait events', + proname => 'pg_get_wait_events', procost => '10', prorows => '100', + proretset => 't', provolatile => 's', prorettype => 'record', + proargtypes => '', proallargtypes => '{text,text,text}', + proargmodes => '{o,o,o}', proargnames => '{type,name,description}', + prosrc => 'pg_get_wait_events' }, Regards, -- Masahiro Ikeda NTT DATA CORPORATION
On Thu, Aug 17, 2023 at 04:37:22PM +0900, Masahiro Ikeda wrote: > On 2023-08-17 14:53, Drouvot, Bertrand wrote: > BTW, is it better to start with "Waiting" like any other lines? > For example, "Waiting for custom event \"worker_spi_main\" defined by an > extension". Using "Waiting for custom event" is a good term here. I am wondering if this should be s/extension/module/, as an event could be set by a loaded library, which may not be set with CREATE EXTENSION. > ex. > postgres(3789417)=# SELECT a.pid, a.wait_event_type, a.wait_event, > w.description FROM pg_stat_activity a JOIN pg_wait_event w ON > (a.wait_event_type = w.type AND a.wait_event = w.name) WHERE wait_event is > NOT NULL; > pid | wait_event_type | wait_event | > description > ---------+-----------------+-------------------+-------------------------------------------------------- > 3783759 | Activity | AutoVacuumMain | Waiting in main loop of > autovacuum launcher process > 3812866 | Extension | WorkerSpiMain | Custom wait event > "WorkerSpiMain" defined by extension > 3783756 | Activity | BgWriterHibernate | Waiting in background > writer process, hibernating > 3783760 | Activity | ArchiverMain | Waiting in main loop of > archiver process > 3783755 | Activity | CheckpointerMain | Waiting in main loop of > checkpointer process > 3783758 | Activity | WalWriterMain | Waiting in main loop of WAL > writer process > (6 rows) You need to think about the PDF being generated from the SGML docs, as well, so this should not be too large. I don't think we need to care about wait_event_type for this example, so it can be removed. If the tuple generated is still too large, showing one tuple under \x with a filter on backend_type would be enough. -- Michael
Вложения
Hi, On 8/17/23 9:37 AM, Masahiro Ikeda wrote: > Hi, > > On 2023-08-17 14:53, Drouvot, Bertrand wrote: > > The followings are additional comments for v7. > > 1) > >>> I am not sure that "pg_wait_event" is a good idea for the name if the >>> new view. How about "pg_wait_events" instead, in plural form? There >>> is more than one wait event listed. >> I'd prefer the singular form. There is a lot of places where it's already used >> (pg_database, pg_user, pg_namespace...to name a few) and it looks like that using >> the plural form are exceptions. > > Since I got the same feeling as Michael-san that "pg_wait_events" would be better, > I check the names of all system views. I think the singular form seems to be > exceptions for views though the singular form is used usually for system catalogs. > > https://www.postgresql.org/docs/devel/views.html > https://www.postgresql.org/docs/devel/catalogs.html Okay, using the plural form in v8 attached. > > 2) > > $ctmp must be $wctmp? > > + rename($wctmp, "$output_path/wait_event_funcs_data.c") > + || die "rename: $ctmp to $output_path/wait_event_funcs_data.c: $!"; > Nice catch, thanks, fixed in v8. > > 3) > > "List all the wait events type, name and descriptions" is enough? > > + * List all the wait events (type, name) and their descriptions. > Agree, used "List all the wait events type, name and description" in v8 (using description in singular as compared to your proposal) > 4) > > Why don't you add the usage of the view in the monitoring.sgml? > > ``` > <para> > Here is an example of how wait events can be viewed: > > <programlisting> > SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event is NOT NULL; > pid | wait_event_type | wait_event > ------+-----------------+------------ > 2540 | Lock | relation > 6644 | LWLock | ProcArray > (2 rows) > </programlisting> > </para> > ``` > > ex. > postgres(3789417)=# SELECT a.pid, a.wait_event_type, a.wait_event, w.description FROM pg_stat_activity a JOIN pg_wait_eventw ON (a.wait_event_type = w.type AND a.wait_event = w.name) WHERE wait_event is NOT NULL; Agree, I think that's a good idea. I'd prefer to add also a filtering on "state='active'" for this example (I think that would provide a "real" use case as the sessions are not waiting if they are not active). Done in v8. > 5) > > Though I'm not familiar the value, do we need procost? > I think it makes sense to add a "small" one as it's done. BTW, while looking at it, I changed prorows to a more "accurate" value. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
Hi, On 8/18/23 12:37 AM, Michael Paquier wrote: > On Thu, Aug 17, 2023 at 04:37:22PM +0900, Masahiro Ikeda wrote: >> On 2023-08-17 14:53, Drouvot, Bertrand wrote: >> BTW, is it better to start with "Waiting" like any other lines? >> For example, "Waiting for custom event \"worker_spi_main\" defined by an >> extension". > > Using "Waiting for custom event" is a good term here. Yeah, done in v8 just shared up-thread. > I am wondering > if this should be s/extension/module/, as an event could be set by a > loaded library, which may not be set with CREATE EXTENSION. > I think that removing "extension" sounds weird given the fact that the wait event type is "Extension" (that could lead to confusion). I did use "defined by an extension module" in v8 (also that's aligned with the wording used in wait_event.h). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Fri, Aug 18, 2023 at 10:57:28AM +0200, Drouvot, Bertrand wrote: > I did use "defined by an extension module" in v8 (also that's aligned with > the wording used in wait_event.h). WFM. -- Michael
Вложения
On Fri, Aug 18, 2023 at 10:56:55AM +0200, Drouvot, Bertrand wrote: > Okay, using the plural form in v8 attached. Noting in passing: - Here is an example of how wait events can be viewed: + Here is are examples of how wait events can be viewed: s/is are/are/. -- Michael
Вложения
Hi, On 8/18/23 12:31 PM, Michael Paquier wrote: > On Fri, Aug 18, 2023 at 10:56:55AM +0200, Drouvot, Bertrand wrote: >> Okay, using the plural form in v8 attached. > > Noting in passing: > - Here is an example of how wait events can be viewed: > + Here is are examples of how wait events can be viewed: > s/is are/are/. Thanks! Please find attached v9 fixing this typo. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
On Sat, Aug 19, 2023 at 11:35:01AM +0200, Drouvot, Bertrand wrote: > Hi, > > On 8/18/23 12:31 PM, Michael Paquier wrote: > Thanks! Please find attached v9 fixing this typo. I was looking at v8, and this looks pretty good to me. I have spotted a few minor things. + proretset => 't', provolatile => 's', prorettype => 'record', This function should be volatile. For example, a backend could add a new wait event while a different backend queries twice this view in the same transaction, resulting in a different set of rows sent back. By the way, shouldn't the results of GetWaitEventExtensionNames() in the SQL function be freed? I mean that: for (int idx = 0; idx < nbextwaitevents; idx++) pfree(waiteventnames[idx]); pfree(waiteventnames); + /* Handling extension custom wait events */ Nit warning: s/Handling/Handle/, or "Handling of". -- Michael
Вложения
Hi, On 8/19/23 12:00 PM, Michael Paquier wrote: > On Sat, Aug 19, 2023 at 11:35:01AM +0200, Drouvot, Bertrand wrote: >> Hi, >> >> On 8/18/23 12:31 PM, Michael Paquier wrote: >> Thanks! Please find attached v9 fixing this typo. > > I was looking at v8, and this looks pretty good to me. Great! > I have > spotted a few minor things. > > + proretset => 't', provolatile => 's', prorettype => 'record', > This function should be volatile. Oh right, I copied/pasted this and should have paid more attention to that part. Fixed in v10 attached. > By the way, shouldn't the results of GetWaitEventExtensionNames() in > the SQL function be freed? I mean that: > for (int idx = 0; idx < nbextwaitevents; idx++) > pfree(waiteventnames[idx]); > pfree(waiteventnames); > I don't think it's needed. The reason is that they are palloc in a short-lived memory context while executing pg_get_wait_events(). > + /* Handling extension custom wait events */ > Nit warning: s/Handling/Handle/, or "Handling of". Thanks, fixed in v10. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
On Sat, Aug 19, 2023 at 06:30:12PM +0200, Drouvot, Bertrand wrote: > Thanks, fixed in v10. Okay. I have done an extra review of it, simplifying a few things in the function, the comments and the formatting, and applied the patch. Thanks! I have somewhat managed to miss the catalog version in the initial commit, but fixed that quickly. -- Michael
Вложения
Hi, On 8/20/23 10:07 AM, Michael Paquier wrote: > On Sat, Aug 19, 2023 at 06:30:12PM +0200, Drouvot, Bertrand wrote: >> Thanks, fixed in v10. > > Okay. I have done an extra review of it, simplifying a few things in > the function, the comments and the formatting, and applied the patch. Thanks! Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Please, consider this small fix for the query example in the docs[1]:
SELECT a.pid, a.wait_event, w.description FROM pg_stat_activity a JOIN pg_wait_events w ON (a.wait_event_type = w.type AND a.wait_event = w.name) WHERE wait_event is NOT NULL and a.state = 'active'; I propose to add a table alias for the wait_event column in the WHERE clause for consistency. 1. https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-ACTIVITY-VIEW
-- Pavel Luzanov Postgres Professional: https://postgrespro.com
Вложения
On Mon, Oct 23, 2023 at 01:17:42PM +0300, Pavel Luzanov wrote: > I propose to add a table alias for the wait_event column in the > WHERE clause for consistency. Okay by me that it looks like an improvement to understand where this attribute is from, so applied on HEAD. -- Michael