Re: WIP: new system catalog pg_wait_event

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