Hi Fujii-san, Thanks for reviewing.
> 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.
+1
> 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.
Done.
>
> 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.
I think this is clearer.
Please find attached Version 2, incorporating the suggested changes.
0002-Add-new-wait-event-to-XactLockTableWait functions.patch
Description: Binary data
