Re: Add new wait event to XactLockTableWait

On 2025/06/09 7:41, Michael Paquier wrote:
> On Sun, Jun 08, 2025 at 10:30:45PM +0800, Xuneng Zhou wrote:
>> This is more problematic in logical replication scenarios where these
>> waits can be very long - for example, when creating a logical
>> replication slot on a busy system. Without a specific wait event, it's
>> hard to distinguish legitimate wait from other issues.
>
> Gotcha.
>
>> Based on suggestions from Fujii and Kevin [1], the patch introduces
>> WAIT_EVENT_XACT_DONE ("Waiting for a transaction to commit or abort")
>> and instructs both functions to report this event during their
>> pg_usleep() calls With patch applied, when backends are waiting in
>> these functions, pg_stat_activity will show what they're waiting for.
>
> + pgstat_report_wait_start(WAIT_EVENT_XACT_DONE);
> [...]
> + pgstat_report_wait_start(WAIT_EVENT_XACT_DONE);
>
> Wouldn't it be better to use two wait events named differently to be
> able to make the difference between the two code paths?

I think it's fine to use the same wait event, since both code paths
are waiting for the same thing, even though they're in different places.

Here are a few review comments on the patch:

WAL_RECEIVER_WAIT_START "Waiting for startup process to send initial data for streaming replication."
WAL_SUMMARY_READY "Waiting for a new WAL summary to be generated."
XACT_GROUP_UPDATE "Waiting for the group leader to update transaction status at transaction end."
+XACT_DONE "Waiting for a transaction to commit or abort."

XACT_DONE should be listed before XACT_GROUP_UPDATE to maintain
alphabetical order.

Also, for the existing description:
transactionid "Waiting for a transaction to finish."

This could be confusing alongside XACT_DONE. Maybe update it to
something like:

"Waiting to acquire a transaction ID lock; see <xref linkend='transaction-id'/>."

This would help users better understand the difference between
the two wait events.

Regards,

--
Fujii Masao
NTT DATA Japan Corporation