On Mon, May 15, 2023 at 06:45:23PM +0200, Drouvot, Bertrand wrote:
> On 5/6/23 4:37 AM, Michael Paquier wrote:
>> On Sat, May 06, 2023 at 11:23:17AM +0900, Michael Paquier wrote:
>>>> I'll look at v7 when the v17 branch opens and propose the separate patch
>>>> mentioned above at that time too.
>>>
>>> Thanks, again.
>>
>> By the way, while browsing through the patch, I have noticed two
>> things:
>> - The ordering of the items for Lock and LWLock is incorrect.
>
> Oh right, fixed in V8 attached (moved the sort on the third column
> instead of the second which has always the same content "WAIT_EVENT_DOCONLY"
> for Lock and LWLock).
Ah, I didn't notice that. Makes sense.
>> - We are missing some of the LWLock entries, like CommitTsBuffer,
>> XactBuffer or WALInsert, as of all the entries in
>> BuiltinTrancheNames.
>
> Yeah, my bad. Fixed in V8 attached.
BufFileTruncate and BufFileWrite have an incorrect order in HEAD's
monitoring.sgml (will fix in a minute for 16~). So your patch fixes
that.
PgStatsDSA and PgStatsData are reversed in your patch compared to
HEAD, actually, based on the way perl sees fit to do its ordering by
giving priority to upper-case characters. Same for RelCacheInit and
RelationMapping, or even SInvalRead/SInvalWrite being now before the
"Serial" family. Worse, the tables LWLock and Lock are in an
incorrect order as well with the patch. We'd better be a bit more
verbose with the sort step, I think.. perl does not handle well
sorting with collations from what I recall, but we could use uc() with
a block sort to force the operation to be case-insensitive, like "sort
{uc($a) cmp uc($b)}". That needs to be applied here, I guess:
+# Sort the lines based on the third column
+my @lines_sorted =
+ sort { (split(/\t/, $a))[2] cmp(split(/\t/, $b))[2] } @lines;
And it looks like you'd need to apply uc() on each [2] element. I
would add a comment about this detail, as well.
No entries are missing, after comparing what's generated by the patch
with the contents of HEAD.
Small nit-ish question: waiteventnames.sgml or wait_event_types.sgml?
Same for generate-waiteventtypes.pl?
>> My apologies for not noticing that earlier. This exists in v6 as much
>> as v7.
>
> No problem at all and thanks for the call out!
FWIW, I would have posted two patches, one with the refactoring of
done in [1], and a second that switches to the automation, to make
clear the preparatory step.
[1]: https://www.postgresql.org/message-id/c6f35117-4b20-4c78-1df5-d3056010dcf5@gmail.com
--
Michael
