Re: Add wait events for server logging destination writes

Hi Seongjun,

I looked at the two _nested helpers in v9.

The code is fine. Like start()/end(), each is a single store; the
previous value is kept in a local of the caller and put back.

There is an alternative: turn start()/end() themselves into macros
that open and close a block, the way PG_TRY()/PG_END_TRY() do, with
end() putting back the previous value:

#define pgstat_report_wait_start(wait_event_info) \
do { \
uint32 outer_wait_event_info_ = \
*(volatile uint32 *) my_wait_event_info; \
*(volatile uint32 *) my_wait_event_info = (wait_event_info)

#define pgstat_report_wait_end() \
*(volatile uint32 *) my_wait_event_info = \
outer_wait_event_info_; \
} while (0)

The _nested helpers are then not needed, and a mismatched pair fails
to compile.

The compiler also finds the sites to fix: change the definitions,
build, and the errors are the places to fix. There are only two
patterns.

First, end() called twice, on the failure branch and on the normal
path. Take the result into a local and move end() ahead of the
branch.

Before:

pgstat_report_wait_start(WAIT_EVENT_SLRU_READ);
if (pg_pread(fd, shared->page_buffer[slotno], BLCKSZ,
offset) != BLCKSZ)
{
pgstat_report_wait_end();
slru_errcause = SLRU_READ_FAILED;
...
return false;
}
pgstat_report_wait_end();

After:

pgstat_report_wait_start(WAIT_EVENT_SLRU_READ);
rc = pg_pread(fd, shared->page_buffer[slotno], BLCKSZ, offset);
pgstat_report_wait_end();
if (rc != BLCKSZ)
{
slru_errcause = SLRU_READ_FAILED;
...
return false;
}

Second, the LWLockReportWaitStart()/LWLockReportWaitEnd() wrappers,
which span functions. Turn them from inline functions into macros,
and their callers need no change:

#define LWLockReportWaitStart(lock) \
pgstat_report_wait_start(PG_WAIT_LWLOCK | (lock)->tranche)
#define LWLockReportWaitEnd() \
pgstat_report_wait_end()

Besides those, error cleanup paths that call end() alone, with no
start(), to reset the field to 0, such as AbortTransaction() and the
sigsetjmp blocks of the auxiliary processes, switch to a separate
reset macro:

#define pgstat_report_wait_clear() \
(*(volatile uint32 *) my_wait_event_info = 0)

There is one cost. Since end() puts back the previous value instead
of 0, a wait event left by a region that an ERROR jumped out of is
saved and put back by every later region until something clears
it. Today the next end() wipes it. So errfinish() needs to call
the reset once before longjmp (before proc_exit() for FATAL). That
also covers PG_CATCH blocks that swallow the error without a
subtransaction. As a safety net, the sigsetjmp block in
PostgresMain() should reset it too.

Two things the compiler does not catch:

- Platform-specific code (WIN32, io_uring and so on) is checked only
by a build on that platform.
- As with PG_TRY(), a region must not be left by return, goto, break
or continue. The compiler does not stop this, and a break or
continue would bind to the hidden do/while. There are none in the
tree today.

This is not a blocker. If you prefer the function form, it is fine
as it is.

Regards,
Henson