Re: Is WAL_DEBUG related code still relevant today?

On Sun, Dec 3, 2023 at 4:00 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> On Sat, Dec 02, 2023 at 07:36:29PM +0530, Bharath Rupireddy wrote:
> > I started to think if this code is needed at all in production. How
> > about we do either of the following?
>
> Well, the fact that this code is hidden behind an off-by-default macro
> seems like a pretty strong indicator that it is not intended for
> production. But that doesn't mean we should remove it.

I think all that the WAL_DEBUG code offers can be achieved with other
means after adjusting a few pieces. Please see the commit message in
the v1 patch posted in this thread at
https://www.postgresql.org/message-id/CALj2ACW5zPMT09eqXvh2Ve7Kz02HShTwyjG%2BxTzkrzeKtQMnQQ%40mail.gmail.com.

> > a) Remove the WAL_DEBUG macro and move all the code under the
> > wal_debug GUC? Since the GUC is already marked as DEVELOPER_OPTION,
> > the users will know the consequences of enabling it in production.
>
> I think the key to this option is verifying there's no measurable
> performance impact.

FWIW, enabling this has a huge impact in production. For instance,
recovery TAP tests are ~10% slower with the WAL_DEBUG macro enabled. I
don't think we go the route of keeping this code.

WAL_DEBUG macro enabled:
All tests successful.
Files=38, Tests=531, 157 wallclock secs ( 0.18 usr 0.05 sys + 14.96
cusr 16.11 csys = 31.30 CPU)
Result: PASS

HEAD:
All tests successful.
Files=38, Tests=531, 143 wallclock secs ( 0.15 usr 0.06 sys + 14.24
cusr 15.62 csys = 30.07 CPU)
Result: PASS

> > b) Remove both the WAL_DEBUG macro and the wal_debug GUC. I don't
> > think (2) is needed to be in core especially when tools like
> > pg_walinspect and pg_waldump can do the same job. And, the messages in
> > (3) and (4) can be turned to some DEBUGX level without being under the
> > WAL_DEBUG macro.
>
> Is there anything provided by wal_debug that can't be found via
> pg_walinspect/pg_waldump?

I don't think so. The WAL record decoding can be achieved with
pg_walinspect or pg_waldump. The page comparison check in
generic_xlog.c can be moved under USE_ASSERT_CHECKING macro. PSA v1
patch posted in this thread.

> > I have no idea if anyone uses WAL_DEBUG macro and wal_debug GUCs in
> > production, if we have somebody using it, I think we need to fix the
> > compilation and test failure issues, and start testing this code
> > (perhaps I can think of setting up a buildfarm member to help here).
>
> +1 for at least fixing the code and tests, provided we decide to keep it.

With no real use of this code in production, instead of fixing
compilation issues and TAP test failures to maintain the code, I think
it's better to adjust a few pieces and remove the other stuff like in
the attached v1 patch.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com